Skip to content
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

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

lowriech
Copy link
Collaborator

@lowriech lowriech commented Aug 8, 2024

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:

  • Open the Mozambique or cambodia deployment
  • Enable the Google Flood layer
  • Click on a gauge point

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
  • Add / update necessary tests?
  • Add / update outdated documentation?

Screenshot/video of feature:

Screenshot 2024-08-12 at 2 03 31 PM 382087325-f448a602-a406-4f64-b6c5-861a8bc25c49

@lowriech lowriech marked this pull request as draft August 8, 2024 22:11
Copy link

github-actions bot commented Aug 8, 2024

Build succeeded and deployed at https://prism-1328.surge.sh
(hash 439a542 deployed at 2024-11-15T22:30:06)

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")
Copy link
Collaborator

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 ?

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

@gislawill gislawill changed the title Adding Google Floods Backend Adding Google Floods Backend + Creating a Gauge Points Aug 12, 2024
@gislawill
Copy link
Collaborator

@lowriech and @ericboucher, there are a couple outstanding issues (noted in the PR description) but this ready for initial review

@gislawill gislawill changed the title Adding Google Floods Backend + Creating a Gauge Points Adding Google Floods Backend + Creating a Gauge Points; COUNTRY=mozambique Aug 12, 2024
Comment on lines +172 to +177
if (
itemProps.visibility === FeatureInfoVisibility.IfDefined &&
!properties[item]
) {
return obj;
}
Copy link
Collaborator

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

Comment on lines 32 to 34
"river": (
data["river"] if "river" in data and len(data["river"]) > 1 else None
),
Copy link
Collaborator

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

@gislawill
Copy link
Collaborator

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

@gislawill gislawill linked an issue Aug 19, 2024 that may be closed by this pull request
@gislawill gislawill marked this pull request as ready for review August 21, 2024 01:21
@gislawill
Copy link
Collaborator

Heads up @wadhwamatic, the today "Date" is now set on these layers
Screenshot 2024-10-09 at 10 00 54 PM

I'll work with @ericboucher to get a new server deployment out for testing

@gislawill
Copy link
Collaborator

Server deployment containing the updates is out (thanks @ericboucher for walking me through it). @wadhwamatic you can resume testing

@wadhwamatic
Copy link
Member

@gislawill - thanks for adding the time dimension here. I tested date intersections a few ways and this is looking good from my side, as does the river gauge info noted in this PR

@gislawill
Copy link
Collaborator

gislawill commented Oct 24, 2024

Remaining todo items:

  • Ensure all parts of tooltip are translate-able

@gislawill gislawill changed the title Adding Google Floods Backend + Creating Gauge Points; COUNTRY=cambodia Google Floods Part 1: Adding backend + creating Gauge Points; COUNTRY=cambodia Oct 24, 2024
* Adding graph for google flood data

* Merge conflict resolution and units

* Updating tests

* formatting

* Using superscript

* Supporting rbd

* Add support for Cambodia

* Updating url for data

* api lint

* Fixing date logic on graph look up

* Updates to chart to denote past vs future

* Adding translation to 'future' label

* Fix chart tests using mock

* Requested changes: descriptive labels
@gislawill gislawill changed the title Google Floods Part 1: Adding backend + creating Gauge Points; COUNTRY=cambodia Google Floods Part 1: Creating gauge points and tooltip graphs; COUNTRY=cambodia Nov 1, 2024
@gislawill
Copy link
Collaborator

@wadhwamatic and @ericboucher, this PR is ready for review. Note: it contains the updates from #1330 to ease the review. There were tooltip features in this PR originally that were overridden by #1330. Now that they're together, we don't need to review code/ui that'll be immediate removed

api/app/googleflood.py Outdated Show resolved Hide resolved
api/app/googleflood.py Outdated Show resolved Hide resolved
@wadhwamatic
Copy link
Member

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

@gislawill gislawill linked an issue Nov 12, 2024 that may be closed by this pull request
@@ -42,11 +49,13 @@ const useStyles = makeStyles(() =>
marginBottom: '4px',
},
popup: {
// Overrides the default maxWidth of 240px set by react-map-gl
maxWidth: 'none !important',
Copy link
Collaborator

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

@wadhwamatic
Copy link
Member

@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.

@gislawill
Copy link
Collaborator

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,
Copy link
Member

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

Copy link
Collaborator

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

@wadhwamatic
Copy link
Member

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?

@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?

It looks like this:
Screenshot 2024-11-15 at 18 53 20

Screenshot 2024-11-15 at 18 53 10

@gislawill
Copy link
Collaborator

@wadhwamatic, line thickness updated and time selection in the charts module updated

@wadhwamatic
Copy link
Member

Thanks @gislawill. The line thickness looks good to me and the time slider issue has been resolved

@gislawill
Copy link
Collaborator

Thanks @wadhwamatic, is this good to go as far as you're concerned?

Some thoughts on potential remaining steps:

  • How does the copy look?
  • Is the layer set up optimal in each deployment (should this be in it's own layer group as currently implemented or put in an existing layer group)
  • We'll need translations once the copy and layer organization is complete

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Chart tooltip window resizes in error [Feature Request]: Google Flood Hub integration
4 participants