-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support pmtile based admin layers; COUNTRY=global #1417
base: master
Are you sure you want to change the base?
Conversation
Build succeeded and deployed at https://prism-1417.surge.sh |
Heads up @ericboucher and @wadhwamatic, this is ready for review. I believe all features but the Analysis Layers are functional. I did run into some performance issues with the loadBoundaryRelations functionality that is required for the Alerts module. We've have over 20000 admin level 2 regions in the dataset and that much data caused a slowdown in start up time. Are any deployments still using the alerts module?? |
…ithub.com/WFP-VAM/prism-app into feature/support-pmtile-based-admin-layers
<Source | ||
id={`source-${layer.id}`} | ||
type="vector" | ||
url={`pmtiles://${layer.path}`} | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: we might get some performance benefits by rendering one source per PMTiles even when we're pulling multiple layers from the same file.
@gislawill great! Look forward to digging into this later. No, there are no active deployments that are using alerts. But it's a feature that we need functioning in the next month for upcoming deployments. |
@wadhwamatic sounds good. @ericboucher it'd be helpful to get your thoughts on loadBoundaryRelations (in frontend/src/components/Common/BoundaryDropdown/utils.ts) and if there's any way it could be simplified while still supporting alerts. That function alone is taking about 22 seconds to run on my computer 😅 |
Hi @gislawill. The chart feature accessed through map interaction only works admin 0 in the surge deployment. I see the request is missing the admin code for admin level 1 and 2 requests. For example: To reproduce:
![]() |
@wadhwamatic this is should now be fixed via the layer.json updates in 83b324c and 5153708 |
Hi @gislawill. Sorry, I missed this before. I see the request is correctly passing the admin codes now for each level, thanks! Something is currently broken on the chart API though and we're getting null results. I'm following up with Valentin on that. |
Thanks @wadhwamatic. I am also seeing cors errors from the charts api (https://api.earthobservation.vam.wfp.org/stats/admin) both in surge and local environments |
@gislawill - Valentin fixed the chart API issue and that functionality is now working as expected for this PR. 🙌🏼 The outstanding issues I see are:
|
@wadhwamatic, could you point me to the screenshot?
|
Issues 1 and 2 have been addressed and issue 3 is being addressed in this PR #1437 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my side Will. Over to @ericboucher for his review
@gislawill - there's an issue I've noticed while testing that I'm hoping we can improve. When a layer is loaded (10-day rainfall for example), boundaries temporarily disappear but then are reloaded and brought to the front, making the experience of loading layers feel clunky. Do you see any opportunities to improve this? |
"type": "boundary", | ||
"path": "data/global/adm0_simplified.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we keep this one for now? (until we have the analysis part figured out?)
@gislawill really nice work, I'm quite impressed / pleased. Once thing I would recommend is to explicitely lock the analysis feature when pmtiles are used, a few options for @wadhwamatic
|
layerDataSelector(boundaryLayer.id), | ||
) as LayerData<BoundaryLayerProps> | undefined; | ||
const { data } = boundaryLayerData || {}; | ||
const baseBoundaryLayer = useSelector( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const baseBoundaryLayer = useSelector( | ||
layerDataSelector(boundaryLayers[0].id), | ||
); | ||
const { data: baseBoundaryLayerData } = baseBoundaryLayer || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth adding a few commends detailing what's related to pmtiles?
@@ -65,7 +65,7 @@ export function getAdminBoundaryTree( | |||
partialTree.children[fp[currentLevelCode]] ?? { | |||
adminCode: fp[currentLevelCode], | |||
key: fp[layer.adminLevelNames[level]], | |||
label: fp[locationLevelNames[level]], | |||
label: fp[locationLevelNames[level]] ?? '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we process out empty lables?
return undefined; | ||
}, [layer.format]); | ||
|
||
// This is causing a pretty massive performance hit. It seems to only be necessary for alerts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO, let's put this PR in a mergeable state, which means no breaking changes for existing deployments. Similarly to analysis maybe we could disable alerts for pmtiles based deployments for now?
admProps => { | ||
if (!multiCountry) { | ||
return country; | ||
} | ||
|
||
if (!admProps) { | ||
return ''; | ||
} | ||
|
||
if (adminLookup) { | ||
return admProps[adminLookup] as string; | ||
} | ||
|
||
return (admProps.admin0Name as string) || ''; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a few doctrings?
@@ -0,0 +1,24 @@ | |||
import { Protocol, PMTiles } from 'pmtiles'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a longer docstring explaining what this is used for and also update the README to explain how to add pmtiles layers
Description
This addresses #1404 by adding support PMTiles (vector tiles) for admin boundaries. This branch does not yet support running analysis (zonal stats) on these PMTiles. We will add support in a following PR that involves querying a S3-hosted geoparquet.
How to test the feature:
Checklist - did you ...
Test your changes with
REACT_APP_COUNTRY=rbd yarn start
REACT_APP_COUNTRY=cambodia yarn start
REACT_APP_COUNTRY=mozambique yarn start
Screenshot/video of feature:
https://www.loom.com/share/a7c80f04dbbb43a5b4cd68334886d088?sid=cda64d68-f6b3-4307-8b9e-3bf827a3faac