-
Notifications
You must be signed in to change notification settings - Fork 35
[1pt] PR: Update update_hydrotable_src
#1654
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
base: dev
Are you sure you want to change the base?
Conversation
…dev-fix-update-hydrotable
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.
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.
@RyanSpies-NOAA and @hhs732 Can you confirm that slope is not being changed during any of the post-processing scripts. If it is changing during post-processing, this may be problematic?
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.
Can we replace this block with just below two lines:
recalc_df =input_src_base.copy()
recalc_df = recalc_df.merge(input_src_full[['HydroID', 'Stage', 'SLOPE']], on=['HydroID', 'Stage'], how='left' )
If needed, you can then drop any unwanted columns carried over from input_src_base.
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.
This is my understanding of the workflow to reset
src_full and hydrotable using this code:
-
make an empty recalc_df:
- get slope and manning from current src_full
- get the rest from src_base (number of cells,...)
- calculate discharge
-
update src_full with recalc_df values (discharge and all other fields)
-
update
discharge_cms
anddefault_discharge_cms
columns of hydro_table with recalc_df discharge column.
@ZahraGhahremani If this is correct, please include it in the code’s docstring.
Adjusted the script to source columns from
src_base
instead ofsrc_full
.Notes Regarding Ghost Bug
The
src_base
files are generated directly by TauDEM, not by our post-processing scripts.The slope values in
src_base
represent TauDEM’s rise-over-run slopes, written in its standard double-precision format.Because these slope values—and the subsequent slopes propagated through HFAB and SWORD—are extremely small (e.g.,
9.99999974737875E-06
), it is essential to preserve their numerical precision across all read/write operations in downstream scripts. Even minor floating-point rounding or formatting differences can lead to inconsistencies in later hydraulic computations.Adopted Approach
When writing slope values to derived files (e.g.,
src_full
,hydrotables
):Example:
9.99999974737875E-06 → 10000
Maintain this integer fixed-point representation in all subsequent files and processes.
Whenever slope values are required for computation (e.g., in Manning’s equation to compute discharge), divide the stored integers by 1e9 to recover the original floating-point slope, e.g.,
10000 / 1e9 = 1e-5
.This method ensures consistent results across all post-processing stages.
Changes
dev-fix-update-hydrotable
: as described.Testing
Generally, you do not copy this part into the ChangeLog. These are some quick notes on what you did test and/or notes for the reviewer to help with their review testing.
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)