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

Fixes a bug with how we load forecast and fire zones from shapefile #1463

Merged
merged 4 commits into from
Jul 25, 2024

Conversation

greg-does-weather
Copy link
Collaborator

What does this PR do? 🛠️

Previously, we'd encountered a situation where the zones shapefile could include multiple entries for a single zone. We mistakenly believed this was simply a duplication. However, zones are sometimes described by multiple polygons. We only kept the first polygon we encountered for a given zone, resulting in a couple dozen zones that had wildly incorrect geometries. Worse, it meant we could sometimes tell a user that there were no alerts for them because we had a misrepresentation of zones in our database. And it showed up visually (see #1409) where we drew the zone on the map, but what we drew was not the entire zone.

To fix this, our zones table is modified to expect a geometry collection for its shape column. When we import zones from the shapefile, we collect all geometries for a single zone and put them into the geometry collection for that zone's database row.

What does the reviewer need to know? 🤔

After merging, we will need to run a spatial load on all of our environments.

@loganmcdonald-noaa
Copy link
Member

After merging, we will need to run a spatial load on all of our environments.

Can we test this in a cloud environment first, or have we already?

Copy link
Member

@loganmcdonald-noaa loganmcdonald-noaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but let's test this in a cloud environment and document exactly what was run before merging? Let's do this in all envs (design, greg, eric) before merging and then doing in staging. Can we just do it for beta without deploying?

@greg-does-weather
Copy link
Collaborator Author

We haven't, but I know someone who can. Hang on a sec!

@greg-does-weather
Copy link
Collaborator Author

And yes, we can run these updates outside of a release/deploy.

@greg-does-weather
Copy link
Collaborator Author

From this branch (or from main after the PR is merged), run this script to load spatial data in cloud environments:

cf login --sso -a api.fr.cloud.gov
./scripts/load-spatial-data.sh <space-name>

Where space-name is one of:

  • beta
  • staging
  • design
  • eric
  • greg

The script handles targeting the right org/space so no worries there. It also handles the wonkiness associated with beta also being called prod. 😜

I have already run it successfully on my environment and can confirm that it does fix #1409. A flood watch as displayed by the Houston WFO:

image

And the same one in my environment:

image

@greg-does-weather greg-does-weather merged commit f7ccf75 into main Jul 25, 2024
17 checks passed
@greg-does-weather greg-does-weather deleted the mgwalker/1409-hazard-counties branch July 25, 2024 15:32
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.

Trouble splitting counties in hazard map/Inland and Coastal Harris County TX example
2 participants