Skip to content

[1pt] PR: Fix CatFIM inundate gms errors #1573

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

Open
wants to merge 31 commits into
base: dev
Choose a base branch
from

Conversation

EmilyDeardorff
Copy link
Contributor

@EmilyDeardorff EmilyDeardorff commented Jun 24, 2025

Closes Issue #1572

In the recent tests of the CatFIM code, the processing errored out during the Inundate_gms() processing. The following error was recorded:

Start Inundating and Mosaicking... 
Running Inundate_gms and mosiacking for 15050302 : cnea3 : action
ERROR Exception: running inundation for 15050302
ERROR Traceback (most recent call last):File "/foss_fim/tools/catfim/generate_categorical_fim_mapping.py", line 554, in run_inundation
map_file = Inundate_gms(TypeError: Inundate_gms() got an unexpected keyword argument 'inundation_polygon'

This error arose due to some updates to the inudnate_gms() script that removed the ability of inundate_gms() to use the branch hydrotables. This update resolves the error by re-implementing the branch hydrotable functionality and updating the input parameters for the inundate_gms() function in the CatFIM code.

Changes

  • tools/inundate_gms.py: Re-implement functionality to use branch hydrotables (rather than HUC hydrotables) inside __inundate_gms_generator().
  • tools/catfim/generate_categorical_fim_mapping.py: Updated output of get_thresholds() function. Removed logic that disregarded LIDs with more or fewer characters than 5. Fixed '---' bug by adding logic to remove that prefix if the site is not on the valid_ahps_ids (in other words, not mapped). Add improved status messaging for when sites are not found on the WRDS API (which uses the new threshold_count variable).
  • tools/catfim/generate_categorical_fim_mapping.py: Fixed HAND gage elevation so they are correctly working in millimeters rather than meters and being saved as uint16 rather than uint8.
  • tools/catfim/generate_categorical_fim_flows.py: Updated output of get_thresholds() function.
  • tools/tools_shared_functions.py: Updated get_thresholds() function to output the number of thresholds found for the site.
  • data/nws/preprocess_ahps_nws.py: Updated output of get_thresholds() function.
  • data/usgs/preprocess_ahps_usgs.py: Updated output of get_thresholds() function.
  • data/usgs/rating_curve_get_usgs_curves.py: Updated output of get_thresholds() function.
  • tools/catfim/ahps_restricted_sites.csv: Added site BOCC2AJM to restricted sites.

Testing

Tested on a flow-based CatFIM run for HUC 01010004 and it worked as expected.

Deployment Plan (For developer use)

How does the changes affect the product?

  • Code only?
  • If applicable, has a deployment plan be created with the deployment person/team?
  • Require new or adjusted data inputs? Does it have start, end and duration code (in UTC)?
  • If new or updated data sets, has the FIM code been updated and tested with the new/adjusted data (subset is fine, but must be a subset of the new data)?
  • Require new pre-clip set?
  • Has new or updated python packages?

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.

  • Informative and human-readable title, using the format: [_pt] PR: <description>
  • Links are provided if this PR resolves an issue, or depends on another other PR
  • If submitting a PR to the 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)
  • Changes are limited to a single goal (no scope creep)
  • The feature branch you're submitting as a PR is up to date (merged) with the latest dev branch
  • pre-commit hooks were run locally
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • CHANGELOG updated with template version number, e.g. 4.x.x.x
  • Add yourself as an assignee in the PR as well as the FIM Technical Lead

Merge Checklist (For Technical Lead use only)

  • Update CHANGELOG with latest version number and merge date
  • Update the Citation.cff file to reflect the latest version number in the CHANGELOG
  • If applicable, update README with major alterations

@EmilyDeardorff EmilyDeardorff self-assigned this Jun 24, 2025
@EmilyDeardorff EmilyDeardorff added bug Something isn't working CatFIM NWS Flood Categorical HAND FIM labels Jun 24, 2025
@EmilyDeardorff EmilyDeardorff changed the title WIP [1pt] Fix CatFIM inundate gms errors [1pt] Fix CatFIM inundate gms errors Jun 25, 2025
@EmilyDeardorff EmilyDeardorff marked this pull request as ready for review June 25, 2025 18:29
Copy link
Contributor

@GregoryPetrochenkov-NOAA GregoryPetrochenkov-NOAA left a comment

Choose a reason for hiding this comment

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

I made a couple of changes to the script and fixed the linting error. Everything looks good to me!

@RobHanna-NOAA
Copy link
Contributor

Greg has approved his part, but Rob still needs to do a CatFIM test in order to fully approve

@RobHanna-NOAA
Copy link
Contributor

Picked up a new error when running this which the most recent changes on the new temp ucs3 OWP server. I ran stage based against 15050302.
2025-06-27 15:18:30 | ERROR || Traceback (most recent call last):
File "/foss_fim/tools/catfim/generate_categorical_fim_mapping.py", line 554, in run_inundation
map_file = Inundate_gms(
File "/foss_fim/tools/inundate_gms.py", line 121, in Inundate_gms
executor_generator = {executor.submit(inundate, **inp): ids for inp, ids in inundate_input_generator}
File "/foss_fim/tools/inundate_gms.py", line 121, in
executor_generator = {executor.submit(inundate, **inp): ids for inp, ids in inundate_input_generator}
File "/foss_fim/tools/inundate_gms.py", line 277, in __inundate_gms_generator
if os.path.isfile(hydro_table_huc): # TODO: Replace with s3_or_local_is_file
File "/usr/lib/python3.10/genericpath.py", line 30, in isfile
st = os.stat(path)
TypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType

HOWEVER... this bug pops up due to it being new DEV git repo code looking for the new hydrotables, but the catfim tests I did had older hand datasets. I will get a new temp copy for that HUC into ucs3 and retest.

Is this really a bug or just a matter of handling it more elegantly. Keep me posted if a fix, or enhancement, is really coming for this one.

@RobHanna-NOAA RobHanna-NOAA dismissed GregoryPetrochenkov-NOAA’s stale review June 27, 2025 16:39

See comment from today, we might have a new fix, or at least an enhancement to handle the scenario better. Sorry.. it is hard to test in EC2s accurately.

@GregoryPetrochenkov-NOAA
Copy link
Contributor

GregoryPetrochenkov-NOAA commented Jun 27, 2025

I went ahead and made a check for None Type, please test again that should be the last quick fix.

@RobHanna-NOAA
Copy link
Contributor

Just tested Greg's fix to handle the error more gracefully and that works. I tried it against the hand 4.6.1.4 hand dataset which we know was not a valid scenario in the first place. My bad. I just pulled down one HUC from the brand new 4.8.6.1 and will retest. That will become a truly valid test.

@RobHanna-NOAA
Copy link
Contributor

Flow based tested out just fine, but stage base has a problem. The error was, running just 15050302:
processing 15050302 : tvca3
15050302 : tvca3: About to process flood categories
15050302 : tvca3: Skipping intervals as there are not any 'non-record' stages
WARNING : 15050302 : tvca3:All stages failed to inundate

Wrapping up processing HUCs for Stage-Based CatFIM...
CRITICAL Traceback (most recent call last):
File "/foss_fim/tools/catfim/generate_categorical_fim.py", line 1953, in
process_generate_categorical_fim(**args)
File "/foss_fim/tools/catfim/generate_categorical_fim.py", line 266, in process_generate_categorical_fim
generate_stage_based_categorical_fim(
File "/foss_fim/tools/catfim/generate_categorical_fim.py", line 1658, in generate_stage_based_categorical_fim
raise Exception(f"no csv files found - missing attribute CSVs in {attributes_dir}")
Exception: no csv files found - missing attribute CSVs in /data/catfim/rob_test/inud_fix_2_stage_based/attributes

All sites failed to inundate for stage based. Might be a missing file from hand maybe?

HOWEVER: it might be right potentially if all of its' sites for that HUC now fail based on new wrds data. This might mean that their is no bug. It is worth checking that huc and maybe running another couple of hucs to see if any of them inundate.

I checked 4.6.1.4 stage based for that huc and it should have 11 sites. Only one of the sites had that mapped.
image

@RobHanna-NOAA RobHanna-NOAA changed the title [1pt] Fix CatFIM inundate gms errors [1pt] PR: Fix CatFIM inundate gms errors Jul 3, 2025
@RobHanna-NOAA
Copy link
Contributor

Ran full scale Flow Base CatFIM for 4.8.6.1 and the comparison tool again it and 4.6.1.4. Both look great. Did some analysis on the sites and the libraries against the datasets for sites / libraries against 4.6.1.4 and they looked good. Did some analysis of the compare tool results against the two and it looks great as well.. Wahoo!!! Flow based is perfect.

Now, we just have to finish getting stage based happy.

@RobHanna-NOAA
Copy link
Contributor

Note; Do not worry about the merge conflict. Once we merge in the navd PR first, this will resolve itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CatFIM NWS Flood Categorical HAND FIM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[13pt] CatFIM inundation errors
4 participants