Repository navigation
Migrate TUFLOW culvert demos with selectable hydraulic engine - #100
Chain-Frost wants to merge 198 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75892bf316
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Chain-Frost
left a comment
There was a problem hiding this comment.
Review note — distinguish TUFLOW Flags from EOF Type (legacy bridge follow-up)
The Flags field in our consolidated channel results comes from _1d_Chan.csv via ChanProcessor.py. It is source channel metadata; we have not established an authoritative interpretation of its actual values from real _1d_Chan.csv samples. It should not be treated as a substitute for the physical culvert Type.
By contrast, EOFProcessor.py explicitly extracts Type from the CULVERT AND PIPE DATA table. processor_collection.py joins this EOF data into the Maximums output by Chan ID, retaining Type (C = circular, R = rectangular). Material and inlet configuration must be supplied explicitly, not inferred from shape, Flags, Manning's n, or losses.
Existing issue to track separately: ryan_library/functions/hy8/run_hy8_bridge.py still defines shape_by_flag = {C: CIRCLE, R: BOX} and material_by_flag = {R: CONCRETE, C: CORRUGATED_STEEL}, then reads row["Flags"] to infer both. This conflates channel flags with physical properties and can silently create the wrong HY-8 model. Please replace those inferences with EOF Type for shape and explicit physical material configuration when that legacy bridge is next updated; keep Flags as metadata and verify its exact semantics against real channel output before decoding it.
The legacy bridge is not changed in this PR. This is a follow-up review observation, not a request to expand PR #100's implementation scope. The new wrappers' use of EOF Type is correct.
Current validation and merge blockers (2026-10-08)
Published head:
45e972c695d5669ed3f34a876cb6e70ca77ec9cc.Dependencies:
vendor/run_hy8at28e7909afd5ae53c4357380ef70c5f2e482c917d(elliptical support, upstream PR #7) andvendor/ryan_culvertsat2adea6abdb715993ba06c2487a12da9a7207f335.The adapter requires explicit physical inlet configuration and preserves source Manning roughness. Implementation-review comments are addressed; native HDPE remains explicitly deferred.
Validation against the updated dependency:
26.10.8.4; source verification and isolated installed-wheel smoke pass. SHA-256:56d2573ae626267de8433b19fa25dfa050f5e5362d5a5cf850b40b0f08626756. The wheel/version/handoff edits are uncommitted and unpublished; the published26.10.8.3wheel is stale.run_hy828e7909: all 30 evaluations returned positive finite discharge/velocity and valid/success status. Results match the earlier reference. Maximum observed differences: headwater 0.0100001 m, discharge 0.9742%, outlet velocity 0.2271%. These descriptive differences are not agreed engineering acceptance tolerances.Merge blockers:
1d_nwkworkflow verification in Validate migrated TUFLOW culvert engines against actual HY-8 on Windows #102. Suppliedtests/test_data/tuflowcontains EOF outputs but no Maximums workbook or1d_nwklayer in its 162 GeoPackages.Current local evidence and delivery state: validation handoff. Windows HY-8 verification is required before merge, not deferred. Keep this PR open; do not merge automatically.
Closes #99.
Summary
run-hy8;--engine hy8|ryan-culvertsselection;culvert_solverAPIs;run_hy8APIs;1d_nwkHW:D checks;Scope note
The migrated workflows remain circular-only because the source demos do not provide enough information to reconstruct rectangular culverts faithfully. TUFLOW
Type = Cestablishes circular shape only; material and inlet treatment are explicit physical inputs and are never inferred from Manning roughness or loss coefficients.Mixed networks can supply per-crossing
MaterialandInlet Configurationthrough the optional attribute source. The shared typed definition carriesCircularCulvertConfigurationseparately fromTuflowLossParameters. Unsupported or unresolved combinations fail closed rather than borrowing another material/inlet assumption.Review fixes
Automated review findings were addressed for:
Num_barrelshandling;Flagschecking so box rows are not treated as circular;1d_nwkfeatures marked ignored;Len_or_ANAfrom the digitised feature geometry;--overwriteis supplied;pBlockageto circular1d_nwkgeometry and failing closed for unresolved category blockage;-99999invert sentinels before hydraulic evaluation;Number_of = 0as TUFLOW's documented one-barrel default;1d_nwkHY-8 workspaces from implicit reuse;pBlockageconsistently with the1d_nwkcircular mapping;trim_runcode, withinternalNamefallback) when selecting governing rows and exporting results;Entry Loss,Exit LossandFixed Lossevidence: nativeEntryCis attached as a barrel loss override while unsupported loss terms fail closed, and HY-8 rejects incompatible arbitrary loss overrides;Regression tests cover these mappings, blockage/default/sentinel semantics, overwrite/workspace safeguards, native and privileged MCP discovery, native forward/inverse dispatch, and the HY-8 adapter boundary.
Intentionally retained ancillary changes
This branch also intentionally retains:
vendor/ryan_culvertspointer advance currently present on the branch.These are intentionally retained rather than removed from this PR.
Validation
See the current validation and merge blockers above. Earlier green runs do not establish final-head readiness.
Dependency / merge order
No
ryan-culvertsimplementation PR is required for this migration because the required public solver APIs already exist.Do not merge automatically.