-
Notifications
You must be signed in to change notification settings - Fork 34
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
Google Floods Part 1: Creating gauge points and tooltip graphs; COUNTRY=cambodia #1328
base: master
Are you sure you want to change the base?
Conversation
Build succeeded and deployed at https://prism-1328.surge.sh |
api/app/main.py
Outdated
@@ -412,3 +413,10 @@ def post_raster_geotiff(raster_geotiff: RasterGeotiffModel): | |||
return JSONResponse( | |||
content={"download_url": presigned_download_url}, status_code=200 | |||
) | |||
|
|||
|
|||
@app.get("/google-floods-gauges") |
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.
If we're gonna add a bunch of integrations, it will be worthwile to think about a standardized approach and common architecture. And maybe expose them through a sub endpoint of the API to keep things cleaner. wdyt @lowriech ?
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.
I think that's a good question, @ericboucher. Do you see other APIs we're already leveraging fitting into this integration category (kobo, hdc stats, others)? Or are you thinking just for new integrations?
I'm wary of premature abstraction and trying to determine if I think now is the right time to align on this standardized approach.
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.
It seems like low-hanging fruit to abstract once we decide on a pattern. So yes, agree we should think about it, but not sure about the velocity of adding entirely new providers. Would vote to wait until we see more pattern around this.
@lowriech and @ericboucher, there are a couple outstanding issues (noted in the PR description) but this ready for initial review |
if ( | ||
itemProps.visibility === FeatureInfoVisibility.IfDefined && | ||
!properties[item] | ||
) { | ||
return obj; | ||
} |
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.
We have some feature properties that may or may not be defined by the google api (for example: site name, river name). This allows us to hide the property from the pop up if it's not defined
api/app/googleflood.py
Outdated
"river": ( | ||
data["river"] if "river" in data and len(data["river"]) > 1 else None | ||
), |
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.
"river" can come in as a string with just 1 space (" "
). This ensures there's a real value
Note: I'm adding pytest-recording to store our google flood responses in local tests and use them in CI/CD. That will fix the failing API test in this PR |
Looks good, thanks Will! I created an issue for the other (pre-existing) bug I mentioned: #1365. If it's simple to address here, please do so otherwise we can address in a separate PR |
@@ -42,11 +49,13 @@ const useStyles = makeStyles(() => | |||
marginBottom: '4px', | |||
}, | |||
popup: { | |||
// Overrides the default maxWidth of 240px set by react-map-gl | |||
maxWidth: 'none !important', |
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.
Heads up @wadhwamatic, this fixes #1365
@gislawill - I noticed today that the charts module has a new bug on this branch. If I go to charts, select an indicator, and then drag the time slider, the chart is not updating to show the new date range even though the axis labels change. Also, if I choose an indicator that uses a line chart (ex: rainfall anomaly), the thickness of the lines has increased compared to what was previously there. |
Thanks for catching these issues! I'll check it out. For the second issue (thickness of the line increasing), that's a result of me purposefully making the line thicker for the flood graphs (easier to interpret imo). @wadhwamatic could you confirm you prefer the thinner line in the charts module? Do you have a stance on the weight of the floods line? |
@@ -178,7 +193,7 @@ const Chart = memo( | |||
fill: config.fill || false, | |||
backgroundColor: colors[i], | |||
borderColor: colors[i], | |||
borderWidth: 2, | |||
borderWidth: 4, |
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.
@gislawill a value of 3 works well from my perspective. let me know what you think
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 to me, updated
@gislawill - I thought that might be the case. I personally prefer a bit thinner. I added a comment, suggesting borderWidth: 3 instead of 4. Sound / look ok to you? |
@wadhwamatic, line thickness updated and time selection in the charts module updated |
Thanks @gislawill. The line thickness looks good to me and the time slider issue has been resolved |
Thanks @wadhwamatic, is this good to go as far as you're concerned? Some thoughts on potential remaining steps:
I'll note for context that the 502 errors from Google in RBD is not an issue on this branch. It seems to only crop up in #1332 (where I've added a cache-based workaround) |
@ericboucher - can you make a final pass on this PR? It's gtg from my side. Let's please remove the google flood layers from prism.json in the demo countries before merging into main though but bring it back subsequently in #1332 |
], | ||
}, | ||
"properties": { | ||
"gaugeId": data["gaugeId"], |
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.
How confident are we that all the keys will be present? Should ze use a getter with a default None to be safe?
Or make sure that we have proper error handling for missing keys.
@@ -38,3 +40,29 @@ def filter(self, record): | |||
self.warning_count += 1 | |||
|
|||
return False | |||
|
|||
|
|||
def make_request_with_retries( |
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.
Do we need something specific, or could we rely on the Retry util directly?
from requests.adapters import Retry
@@ -6531,5 +6531,77 @@ | |||
} | |||
], | |||
"legend_text": "Joint Food Security & Nutrition Hotspot Analysis conducted by WFP, UNICEF, and partners based on SMART surveys and Cadre Harmonisé (CH) & Integrated Food Security Phase Classification (IPC) analyses" | |||
}, | |||
"google_flood_status_at_gauges": { |
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 this layer be shared ans then just override the necessary param?
@@ -8,7 +8,7 @@ interface FetchWithTimeoutOptions extends RequestInit { | |||
|
|||
export const ANALYSIS_REQUEST_TIMEOUT = 60000; | |||
|
|||
const DEFAULT_REQUEST_TIMEOUT = 15000; | |||
const DEFAULT_REQUEST_TIMEOUT = 30000; |
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.
Still needed? Is it to account for the retries?
@@ -208,8 +223,21 @@ const Chart = memo( | |||
fill: false, | |||
})); | |||
} | |||
if (data.GoogleFloodConfig) { |
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.
This PR is introducing a lot of extra complexity to this file. I'll open an issue recommending to split it by functions/services
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.
Nice work overall. Let's make sure to QA all different tooltips inc. EWS.
I left a few comments in line but this looks gtm overall !
Description
This fulfills step 1 and 2 of 3 for #1317 (see plan).
In this PR, we're fetching flood status by gauge from Google's Floods API for the entire country (10-100 gauges per country). We're displaying these gauges as points on Prism. They're color coded by flood status and provide past and future flooding in a tooltip when clicked.
Forecasts for each gauge were added here: #1330 (merged into this branch for review)
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: