-
Notifications
You must be signed in to change notification settings - Fork 35
[8pt] PR: Fix Alaska HydroIDs #1650
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
Conversation
…into dev Mergin remote repo updates
…into dev Latest changes
…into dev Merge dev
…into dev Merge latest changes.
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 tested this PR for flow-based CatFIM and stage-based CatFIM and the over-inundation that was previously present in AK is no longer there. The performance if CatFIM in non-Alaska areas is also unchanged, which is a good thing.
I ran the following commands testing the new precalb option (-p flag) and also without:
The first metrics are identical to testing before merging last round of PRs. |
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 reran stage and flow based CatFIM with the new code changes ad it looks like it still performed as expected.
…into dev-fix-alaska-hydroids
This PR addresses #1629. Upon further review, even aside from the HydroIDs being more than 32766 in number, the gage watershed algorithm output normally creates HydroIDs starting with 8 digits. However, in some HUCs in Alaska there is a mix of 8 digit and 9 digit HydroIDs even though the amount of features are less than 32766 in cardinality. this addresses both cases as well as provides error for processing a catchment if Int16 is used and there are more than 32766 features and or a mix of 8 digit and 9 digit HydroIDs.
I also removed the overlapping_inundation script in favor of rasterio merge which will address the following issues:
#644, and partially addresses #1562.
Changes
Removals
Testing
Created FIM outputs with all of Alpha Test and every Alaska HUC in an output folder named alaska_fix.
I ran synthesize test cases and got identical results for all evaluation types aside from two NWS sites in Alaska HUC 19020302. I am still showing the difference even though the inundation was previously incorrect. While the CSI and PND regressed the MCC and FAR all increased for these two sites.
I also ran inundate nation for the alaska_fix output folder and remedied the over inundation exhibited in the original issue as seen here:
Deployment Plan (For FIM developers use)
Does the change impact inputs, docker or python packages?
If you are not a FIM dev team member: Please let us know what you need and we can help with it.
If you are a FIM Dev team member:
Please work with the DevOps team and do not just go ahead and do it without some co-ordination.
Copy where you can, assign where you can not, and it is your responsibility to ensure it is done. Please ensure it is completed before the PR is merged.
Has new or updated python packages, PipFile, Pipefile.lock or Dockerfile changes? DevOps can help or take care of it if you want. Just need to know if it is required.
Require new or adjusted data inputs? Does it have a way to version (folder or file dates)?
Please use caution in removing older version unless it is at least two versions ago. Confirm with DevOps if cleanup might be involved.
If new or updated data sets, has the FIM code, including running fim_pipeline.sh, been updated and tested with the new/adjusted data? You can dev test against subsets if you like.
Notes to DevOps Team or others:
Please add any notes that are helpful for us to make sure it is all done correctly. Do not put actual server names or full true paths, just shortcut paths like 'efs..../inputs/, or 'dev1....inputs', etc.
Issuer Checklist (For developer use)
You may update this checklist before and/or after creating the PR. If you're unsure about any of them, please ask, we're here to help! These items are what we are going to look for before merging your code.
[_pt] PR: <description>
dev
branch (the default branch), you have a descriptive Feature Branch name using the format:dev-<description-of-change>
(e.g.dev-revise-levee-masking
)dev
branchpre-commit
hooks were run locally4.x.x.x
Merge Checklist (For Technical Lead use only)