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

[1pt] PR: Updating modules with new AEP (NWM V3) #1198

Merged
merged 11 commits into from
Aug 2, 2024
Merged

Conversation

ZahraGhahremani
Copy link
Contributor

@ZahraGhahremani ZahraGhahremani commented Jun 24, 2024

This PR updates scripts that use the recurrence flow files and high water thresholds. The new flow files (NWM V3) are in /inputs/rating_curve/nwm_recur_flows/ and the new high water threshold is in /inputs/rating_curve/bankfull_flows/nwm3_high_water_threshold_cms.csv.

This PR also incorporates Manning numbers for Alaska feature ids, needed for calibration. The new Manning dataset is available on dev1 here: /dev_fim_share/foss_fim/outputs/ali_alaska_calib/mannings_global_nwm3.csv

Addresses #1189

Changes

  • src/bash_variables.env: high water threshold and recurrence flows CSV files were updated into new NWM v3 flow files. Also, a new Manning numbers file created from the new NWM v3 dataset was used.
  • src/src_adjust_ras2fim_rating.py: 100 year recurrence was removed since it is not included in the new AEP.
  • src/src_adjust_usgs_rating_trace.py: 100 year recurrence was removed since it is not included in the new AEP.
  • tools/rating_curve_comparison.py: 100 year recurrence was removed since it is not included in the new AEP. Also, the name of recurrence flow CSV file was updated.
  • tools/composite_inundation.py
  • tools/inundate_nation.py

Testing

  • Tested for some HUCs (01010002, 10290103, 10290105, 21010002).
  • Needs to run for alpha domain.

Also, made a successful FIM run on Alaska HUC 19020302 using the new Manning number file (based on NWM V3 feature ids which now include Alaska feature ids). The run successfully applied the calibration from a USGS gage within the HUC. Plot below shows a typical change in SRC for one of HydroIDs:

image

Alpha test results below confirm validity of the above calibration:
image

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
  • Passes all unit tests locally (inside interactive Docker container, at /foss_fim/, run: pytest unit_tests/)
  • N/A 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
  • Reviewers requested
  • 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

@ZahraGhahremani ZahraGhahremani changed the title updated modules with new AEP [1pt] PR: Updating modules with new AEP (NWM V3) Jun 24, 2024
@ZahraGhahremani ZahraGhahremani added the testing Evaluation and testing related label Jun 24, 2024
@ZahraGhahremani ZahraGhahremani self-assigned this Jun 24, 2024
@ZahraGhahremani ZahraGhahremani marked this pull request as ready for review June 24, 2024 22:41
@CarsonPruitt-NOAA CarsonPruitt-NOAA linked an issue Jul 9, 2024 that may be closed by this pull request
Copy link
Contributor

@mluck mluck left a comment

Choose a reason for hiding this comment

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

The code changes here look good. A couple of things:

  1. There are 3 other files in the repo that reference "nwm21" that should probably also get updated to "v3": two are usage (tools/composite_inundation.py and tools/inundate_nation.py) and one is a pytest (unit_tests/inundate_gms_params.json).
  2. Please merge with the current dev branch.

@RobHanna-NOAA
Copy link
Contributor

The code changes here look good. A couple of things:

  1. There are 3 other files in the repo that reference "nwm21" that should probably also get updated to "v3": two are usage (tools/composite_inundation.py and tools/inundate_nation.py) and one is a pytest (unit_tests/inundate_gms_params.json).
  2. Please merge with the current dev branch.

Pytest is going away shortly. The whole unit tests system will disappear with the new docker image update PR coming soon.

@ZahraGhahremani
Copy link
Contributor Author

The code changes here look good. A couple of things:

  1. There are 3 other files in the repo that reference "nwm21" that should probably also get updated to "v3": two are usage (tools/composite_inundation.py and tools/inundate_nation.py) and one is a pytest (unit_tests/inundate_gms_params.json).
  2. Please merge with the current dev branch.

Thanks Matt! I updated those files.

@mluck
Copy link
Contributor

mluck commented Jul 19, 2024

Sorry, one more thing. There are a few files that have the old path (/data/inundation_review/inundation_nwm_recurr/) that need to be updated to the new path (/data/inputs/rating_curve/nwm_recur_flows/).

@ZahraGhahremani
Copy link
Contributor Author

ZahraGhahremani commented Jul 19, 2024

Sorry, one more thing. There are a few files that have the old path (/data/inundation_review/inundation_nwm_recurr/) that need to be updated to the new path (/data/inputs/rating_curve/nwm_recur_flows/).

I found three files with the old path. Two of them are in the unit tests, which Rob mentioned will be removed next week. The other file is tools/inundation_wrapper_nwm_flows.py, which generates inundation outputs using the NWM Recurrence Interval flow data for 1.5-year, 5-year, and 10-year events. In the new version, we don't have flow data for 1.5-year events in HI and Pr/Vi. Should I still update this and mention that the new data is only for CONUS and AK, or should I leave it as is?

mluck
mluck previously approved these changes Jul 19, 2024
@RyanSpies-NOAA
Copy link
Collaborator

Hey Zahra - can you paste in a few of your alpha eval comparison plots? It's helpful to have them in the PR in case we ever need to look back.

@ZahraGhahremani
Copy link
Contributor Author

Hey Zahra - can you paste in a few of your alpha eval comparison plots? It's helpful to have them in the PR in case we ever need to look back.

Here are some eval plots:

csi_nws_comp
ble_comp_stackedbar
nws_comp_stackedbar
usgs_comp_stackedbar

@CarsonPruitt-NOAA
Copy link
Collaborator

Do we have the new high water mark flow files as well? I would like to include that file to this PR addition to the recurrence intervals.

@ZahraGhahremani
Copy link
Contributor Author

Do we have the new high water mark flow files as well? I would like to include that file to this PR addition to the recurrence intervals.

Yes, they are in /inputs/rating_curve/bankfull_flows/nwm3_high_water_threshold_cms.csv. I updated modules based on this new high water threshold.

@RobHanna-NOAA
Copy link
Contributor

Hi Zahra, can you please merge dev in again and check in. I can see via the change log that it is fairly out of sync.

Merge remote-tracking branch 'origin/dev' into dev-update-aep
Rob made a quick fix to the changelog file
ZahraGhahremani-NOAA and others added 2 commits July 24, 2024 16:45
Merge remote-tracking branch 'origin/dev' into dev-update-aep
mluck
mluck previously approved these changes Jul 26, 2024
Copy link
Contributor

@mluck mluck left a comment

Choose a reason for hiding this comment

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

Looks good

@CarsonPruitt-NOAA CarsonPruitt-NOAA merged commit 9cb4a63 into dev Aug 2, 2024
1 check passed
@CarsonPruitt-NOAA CarsonPruitt-NOAA deleted the dev-update-aep branch August 2, 2024 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request FIM4 testing Evaluation and testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[13pt] Update AEP files to NWM v3
6 participants