make titiler tile endpoints consistent with tile cache service #169
Conversation
…r endpoints consistent with tile cache service
…m for dist/integrated alerts as we surface in api doc
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #169 +/- ##
==========================================
+ Coverage 80.24% 80.29% +0.05%
==========================================
Files 59 61 +2
Lines 1954 2005 +51
==========================================
+ Hits 1568 1610 +42
- Misses 386 395 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
solomon-negusse
requested review from
gtempus,
danscales,
dmannarino and
manukala6
and removed request for
gtempus
| # tags=["Raster Tiles"], | ||
| # response_description="PNG Raster Tile", | ||
| # ) | ||
| @router.get( |
There was a problem hiding this comment.
the /titiler endpoint for integrated alerts here is temporary to deploy this and test it without replacing the existing dynamic implementation. there is an item in the roadmap to fully implement this later.
There was a problem hiding this comment.
@solomon-negusse and I walked through this together yesterday. I'm fine with the temporary titiler implementation. Moving away from the auto-magic titiler endpoints is worth it, even though it's a little more work. The URLs are much cleaner 🧹
Thanks, Solomon! 💪
| start_date: Optional[str] = None | ||
| end_date: Optional[str] = None | ||
| alert_confidence: Optional[str] = None | ||
| render_type: Optional[RenderType] = RenderType.true_color |
There was a problem hiding this comment.
Shouldn't render_type just be RenderType - not Optional[RenderType]. You've given it a default value, and it really should be true_color or encoded, not None, right?
There was a problem hiding this comment.
Good catch, thanks.
app/main.py
Outdated
| @@ -42,6 +42,8 @@ | |||
| from .routes import preview | |||
|
|
|||
| from .routes.titiler import routes as titiler_routes | |||
| from .routes.titiler.integrated_alerts import router as alerts_router | |||
There was a problem hiding this comment.
Very minor: Maybe rename "alerts_router" to "integrated_alerts_router" for consistency with next line?
|
|
||
| _versions = get_versions(dataset, TileCacheType.cog) | ||
| for _version in _versions: | ||
| extend_enum(GfwIntegratdAlertsVersions, _version, _version) |
There was a problem hiding this comment.
Can you add comments on what GfwIntegratedAlertsVersions() is for? I don't see it used anywhere, so then it seems like a lot of work to create an enum with all the integrated alerts versions going back for several years.
There was a problem hiding this comment.
I have added back the use of that enum in the view function which will show to the user the available valid versions to use.
| tags=["Raster Tiles"], | ||
| response_description="PNG Raster Tile", | ||
| ) | ||
| async def glad_dist_alerts_raster_tile( |
There was a problem hiding this comment.
I wonder if we should get rid of glad_ here and in the name of the file? So file name is just umd_dist_alerts.py and name here is dist_alerts_raster_tile. Would be more parallel with integrated_alerts. Also, including glad may confuse with the glad-L and glad-S2 alerts.
There was a problem hiding this comment.
i added gfw_ prefix to integrated alerts. {org}_optional{group} prefix is used consistently in other route files. GLAD is just the name of the UMD research group.
|
Your new changes in response to some of my comments look good. I guess you just have to figure out the test failure, then you can merge the PR. |
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the new behavior?
Titiler endpoints use the same pattern as tile cache service
Does this introduce a breaking change?
Other information