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

Fix NHD issues related to gage byte strings #742

Merged
merged 4 commits into from
Feb 28, 2024

Conversation

shorvath-noaa
Copy link
Contributor

There was inconsistent handling of converting USGS gage IDs from byte strings to character strings. This resulted in link-gage-crosswalk and lake-gage-crosswalk having different character representations of the same gage IDs. The position where _reindex_link_to_lake_id() function was also caused issues as it replaced link IDs with lake IDs in usgs_df before creating reservoir_usgs_df. This resulted in missing lake IDs/gage IDs in reservoir_usgs_df.

Additions

Removals

Changes

DataAssimilation.py

  • Moved location of _reindex_link_to_lake_id()

NHDNetwork.py

nhd_io.py

  • Convert gage ID byte strings to character strings in a consistent way.

nhd_network.py

  • Convert gage ID byte strings to character strings in a consistent way.

preprocess.py

  • Moved location of _reindex_link_to_lake_id()

Testing

  1. Ran nwm_routing v3 and v4 on NHD network with extended_AnA configuration (1-day run).

Screenshots

Notes

Todos

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • 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
  • Visually tested in supported browsers and devices (see checklist below 👇)
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Target Environment support

  • Windows
  • Linux
  • Browser

Accessibility

  • Keyboard friendly
  • Screen reader friendly

Other

  • Is useable without CSS
  • Is useable without JS
  • Flexible from small to large screens
  • No linting errors or warnings
  • JavaScript tests are passing

@shorvath-noaa shorvath-noaa marked this pull request as ready for review February 23, 2024 17:17
@AminTorabi-NOAA
Copy link
Contributor

I got an error in compute.py that says KeyError: "None of [Index([5194634], dtype='int32', name='lake_id')] are in the [index]"
seems like this line below and previous change on L139 won't run because from_file is True
self._usgs_df = _reindex_link_to_lake_id(self._usgs_df, network.link_lake_crosswalk)

Some general improvement : in compute.py it happens that usgs_wbodies_sub is empty so reservoir_usgs_df.loc[usgs_wbodies_sub] would be empty there is no need to calculate those. We should skip the empty ones

@shorvath-noaa
Copy link
Contributor Author

shorvath-noaa commented Feb 26, 2024 via email

@@ -344,6 +342,8 @@ def __init__(self, network, from_files, value_dict, da_run=[]):
set_index('usgs_lake_id')
)

self._usgs_df = _reindex_link_to_lake_id(self._usgs_df, network.link_lake_crosswalk)
Copy link
Contributor

Choose a reason for hiding this comment

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

This part of the code is not running while from_files (L295) is True

usgs_df = network.link_gage_df.reset_index().set_index('gages').join(usgs_df).set_index('link').sort_index()

self._usgs_df = _reindex_link_to_lake_id(usgs_df, network.link_lake_crosswalk)
self._usgs_df = network.link_gage_df.reset_index().set_index('gages').join(usgs_df).set_index('link').sort_index()
Copy link
Contributor

Choose a reason for hiding this comment

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

This part of the code is not running while from_files (L100) is True

@shorvath-noaa shorvath-noaa merged commit 63542ab into NOAA-OWP:master Feb 28, 2024
4 checks passed
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.

3 participants