-
Notifications
You must be signed in to change notification settings - Fork 29
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] Fix inundation nation pathing errors #1026
Conversation
@RyanSpies-NOAA : Hi Ryan. I took this one to the goal line. See testing notes embedded. If you are comfortable with it, can you approve it? Your call if you want to do more testing on it but it likely is ok. Note: I did not test or understand how to test the change the newer mannings file. |
config/params_template.env
Outdated
@@ -47,7 +47,7 @@ export src_subdiv_toggle="True" | |||
# text to append to output log and hydrotable file names (use for testing/debugging) | |||
export vrough_suffix="" | |||
# input file location with nwm feature_id and channel roughness and overbank roughness attributes | |||
export vmann_input_file="$inputsDir/rating_curve/variable_roughness/mannings_global_06_12.csv" | |||
export vmann_input_file="$inputsDir/rating_curve/variable_roughness/mannings_global_025_05.csv" |
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 don't think we want this changed?
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.
oh sorry. Not sure why that change was made. Anyways. I fixed it and put it back. :)
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.
Reviewed code changes and ran a successful test run with 10 hucs. Also reviewed the 3gb BED mosaic raster and confirmed inundation was generated for full domain.
A couple of directly related issues were fixed in this PR.
The initial problem came from Issue #1025 which was about a pathing issue for the outputs directory. In testing that fix, it exposed a few other pathing and file cleanup issues which are now fixed. We also added more console output to help view variables and pathing.
Closes #1025
Changes
config
/params_template.env
: Updated for a newer mannings global file. Changed and tested by Ryan Spies.tools
inundate_mosiac_wrapper.py
: Took out a misleading and non-required print statement.inundate_nation.py
: As mentioned above.Testing
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 locally/foss_fim/
, run:pytest unit_tests/
)4.x.x.x
Merge Checklist (For Technical Lead use only)