Skip to content

Use NWM reach slope for normal depth boundary condition#3

Merged
ar-siddiqui merged 5 commits into
NGWPC:mainfrom
biplovbhandari:normal-depth-slope
May 13, 2026
Merged

Use NWM reach slope for normal depth boundary condition#3
ar-siddiqui merged 5 commits into
NGWPC:mainfrom
biplovbhandari:normal-depth-slope

Conversation

@biplovbhandari

@biplovbhandari biplovbhandari commented Apr 3, 2026

Copy link
Copy Markdown
Member

Summary

Replace the hardcoded 0.001 downstream normal depth slope with the actual NWM reach slope from the hydrofabric. Discussion in NGWPC/ripple1d-archive#220 confirmed that using the actual slope reduced water surface elevation discrepancies by several feet.

Addressing review feedback

  • Constant rename: NORMAL_DEPTHDEFAULT_ND_SLOPE — the constant stores a slope value, not a boundary condition type.
  • Slope source reach: the slope now comes from the NWM reach the submodel's final downstream cross-section actually intersects — not blindly from the current reach. Implemented via spatial intersection + network walk.

Changes

  • New helpers in rasfim.py:
    • _ds_xs_geometry() — resolves the ds_xs dict back to its RAS geometry
    • _select_ds_slope_reach() — when the ds_xs intersects multiple NWM reaches, walks the to_id chain from the current reach and picks the furthest downstream candidate
    • get_ds_boundary_slope() — orchestrates the full lookup with fallback chain: walk result → current reach slope → DEFAULT_ND_SLOPE
  • Conflation JSON now includes ds_slope and ds_slope_source_nwm_id (audit field showing which reach provided the slope)
  • create_model_run_normal_depth() and run_incremental_normal_depth() (ras_run.py) pass ds_slope to normal_depth_run(), falling back to DEFAULT_ND_SLOPE
  • FAQ (docs/source/faq.rst) and docstrings updated

Incidental cleanups

While in these files, also:

  • Fixed spelling typos (extenstion→extension, Detemine→Determine, junciton→junction, etc.)
  • Removed unused imports in ras.py, ras_run.py, ras_conflate.py
  • Replaced hardcoded False defaults with SHOW_RAS constant from consts.py for show_ras parameter

Conflation file changes

  • tests/ras-data/*/{Baxter,MissFldwy,PatuxentRiver,winooski}.conflation.json: Regenerated against tests/nwm-data/flows.parquet to pick up new ds_slope and ds_slope_source_nwm_id values

Testing

  • pytest tests/conflation_test.py -v — 16 unit tests pass (5 new tests covering the slope selection logic and fallback chain)
  • pytest tests/conflation_tests/conflation_test.py -v — all 14 integration tests pass

@biplovbhandari

Copy link
Copy Markdown
Member Author

@ar-siddiqui @mdeshotel for your review

@ar-siddiqui ar-siddiqui left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My main comment here is that the nd_slope may not always be the slope of reach, it should be the slope of the reach that intersect with the last XS, that could be the reach in consideration itself or a reach downstream of it.

Comment thread ripple1d/conflate/rasfim.py Outdated
Comment thread docs/source/faq.rst Outdated
Comment thread ripple1d/conflate/rasfim.py Outdated
… with the actual nwm reach slope from the hydrofabric
@biplovbhandari
biplovbhandari requested a review from ar-siddiqui May 4, 2026 18:14

@ar-siddiqui ar-siddiqui left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks really good.

Minor changes requested only.

Comment thread ripple1d/ops/ras_run.py Outdated
Comment thread ripple1d/ops/ras_run.py Outdated
the model with a normal depth downstream boundary condition using the NWM
reach slope for the submodel's final downstream cross section (falls back to
DEFAULT_ND_SLOPE if unavailable). The conflation step bounds the value to
[MIN_ND_SLOPE, MAX_ND_SLOPE] (currently [1e-6, 0.1]).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same comment as above, also see if we really need to document this at two places. Feels redundant.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

On the redundancy: I'd lean toward keeping the note in both. create_model_run_normal_depth and run_incremental_normal_depth are two separate user-facing endpoints, and someone reading either one's docstring shouldn't have to chase a cross-reference to learn the boundary condition behavior. Happy to revisit if you feel strongly, though.

Comment thread tests/ras-data/Baxter/Baxter.conflation.json Outdated
Comment thread ripple1d/conflate/rasfim.py Outdated

@ar-siddiqui ar-siddiqui left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you.

@ar-siddiqui
ar-siddiqui merged commit 48e27c6 into NGWPC:main May 13, 2026
3 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.

2 participants