Skip to content

Migrate TUFLOW culvert demos with selectable hydraulic engine - #100

Open
Chain-Frost wants to merge 198 commits into
mainfrom
feature/issue-99-tuflow-culvert-engines
Open

Chain-Frost wants to merge 198 commits into
mainfrom
feature/issue-99-tuflow-culvert-engines

Conversation

@Chain-Frost

@Chain-Frost Chain-Frost commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner

Current validation and merge blockers (2026-10-08)

Published head: 45e972c695d5669ed3f34a876cb6e70ca77ec9cc.
Dependencies: vendor/run_hy8 at 28e7909afd5ae53c4357380ef70c5f2e482c917d (elliptical support, upstream PR #7) and vendor/ryan_culverts at 2adea6abdb715993ba06c2487a12da9a7207f335.

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:

  • Rebuilt retained wheel locally as 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 published 26.10.8.3 wheel is stale.
  • Ruff lint/format and strict Pyright pass across 15 changed first-party Python files; focused tests: 134 passed, zero skips/deselections.
  • Repeated 15 synthetic Windows HY-8/native scenario pairs using HY864.exe 8.0.1.2 and run_hy8 28e7909: 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.
  • Python CI run 150 at the published head: Policy passed; Package failed at retained-wheel verification; Hosted tests still running at the latest check. The 1037 passing tests from run 149 apply to an earlier head.

Merge blockers:

  1. Commit/publish the rebuilt wheel and metadata, then pass every hosted job on that final head.
  2. Complete representative Maximums/EOF and mixed 1d_nwk workflow verification in Validate migrated TUFLOW culvert engines against actual HY-8 on Windows #102. Supplied tests/test_data/tuflow contains EOF outputs but no Maximums workbook or 1d_nwk layer in its 162 GeoPackages.
  3. Verify source dimension provenance and the active blockage matrix under Verify TUFLOW blockage handling in Maximums/EOF culvert integration #103. The user confirms reduced-area blockage with unadjusted nominal diameters as the intended design method; actual processed-input provenance remains unverified.
  4. Agree numerical acceptance tolerances and complete the remaining real-input/executable checks before manual merge.

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

  • migrates the two TUFLOW-specific culvert computation demos out of run-hy8;
  • adds a shared engine-neutral circular culvert definition with explicit physical shape/material/inlet configuration and separate TUFLOW loss parameters;
  • adds explicit --engine hy8|ryan-culverts selection;
  • delegates native hydraulics to public culvert_solver APIs;
  • delegates HY-8 hydraulics to public run_hy8 APIs;
  • preserves the original Maximums forward/inverse scenarios and 1d_nwk HW:D checks;
  • removes project-specific hard-coded source paths;
  • emits engine-labelled long-form CSV while retaining failed rows/scenarios;
  • catalogues both maintained wrappers for MCP discovery;
  • documents repository ownership and the lack of assumed numerical parity.

Scope note

The migrated workflows remain circular-only because the source demos do not provide enough information to reconstruct rectangular culverts faithfully. TUFLOW Type = C establishes 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 Material and Inlet Configuration through the optional attribute source. The shared typed definition carries CircularCulvertConfiguration separately from TuflowLossParameters. Unsupported or unresolved combinations fail closed rather than borrowing another material/inlet assumption.

Review fixes

Automated review findings were addressed for:

  • canonical Maximums Num_barrels handling;
  • fail-closed Flags checking so box rows are not treated as circular;
  • preserving the complete governing Maximums row instead of synthesising columns across rows;
  • preserving valid zero invert elevations;
  • modelling HY-8 constant tailwater with the actual downstream invert rather than the water-surface elevation;
  • skipping TUFLOW 1d_nwk features marked ignored;
  • resolving negative Len_or_ANA from the digitised feature geometry;
  • refusing result CSV replacement unless --overwrite is supplied;
  • using unique retained HY-8 workspaces per crossing/AEP/scenario and requiring explicit overwrite before reuse;
  • exposing the native wrappers in the packaged MCP workflow catalogue with focused registry coverage and safety metadata;
  • applying numeric TUFLOW pBlockage to circular 1d_nwk geometry and failing closed for unresolved category blockage;
  • rejecting unresolved TUFLOW -99999 invert sentinels before hydraulic evaluation;
  • treating canonical Number_of = 0 as TUFLOW's documented one-barrel default;
  • protecting retained 1d_nwk HY-8 workspaces from implicit reuse;
  • splitting external HY-8 catalogue discovery into privileged, explicit-approval workflow entries.
  • applying Maximums pBlockage consistently with the 1d_nwk circular mapping;
  • using deterministic filesystem-safe internal HY-8 crossing/project/barrel names while preserving original TUFLOW IDs in normalized output;
  • making retained HY-8 workspace keys collision-resistant after sanitization in both migrated wrappers.
  • allowing adverse-slope culverts through the engine-neutral model and failing closed only for the native engine that does not support them;
  • adding wrapper/library identity banners to both maintained wrappers;
  • avoiding HY-8 workspace-root creation for native Maximums runs.
  • preserving Maximums base-run identity (trim_runcode, with internalName fallback) when selecting governing rows and exporting results;
  • retaining nominal diameter separately from blockage-adjusted hydraulic diameter so HW/D targets and reported HW/D remain TUFLOW-consistent in both wrappers.
  • rejecting malformed/non-finite barrel-count values instead of silently defaulting them to one barrel while preserving documented missing/blank defaults;
  • replacing roughness-based material inference with explicit typed physical configuration (shape, material and inlet treatment);
  • supporting per-crossing inlet configuration alongside material in CSV/vector attribute sources and CLI overrides;
  • keeping TUFLOW loss coefficients separate from physical inlet selection;
  • preserving Maximums Entry Loss, Exit Loss and Fixed Loss evidence: native EntryC is attached as a barrel loss override while unsupported loss terms fail closed, and HY-8 rejects incompatible arbitrary loss overrides;
  • correcting wrapper-side attribute lookup and isolating workspace tests from unrelated native mixed-flow convergence.

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:

  • the ASC_to_ASC parity-test conventional executable default and its documentation;
  • the minor uncertainty-test Ruff formatting change;
  • the vendor/ryan_culverts pointer 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

  1. Merge this PR manually.
  2. Then merge Remove TUFLOW-specific demo ownership run-hy8#6, which removes the obsolete upstream TUFLOW demo copies and closes Review ownership of TUFLOW/domain-specific demo scripts run-hy8#4.

No ryan-culverts implementation PR is required for this migration because the required public solver APIs already exist.

Do not merge automatically.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-08T02:25:34.384453Z cbbdfab Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment thread ryan-scripts/tuflow/tuflow_culvert_from_maximums.py Outdated
Comment thread ryan-scripts/tuflow/tuflow_culvert_from_maximums.py Outdated
Comment thread ryan_library/functions/culvert/tuflow_engines.py Outdated
Comment thread ryan-scripts/tuflow/tuflow_culvert_from_maximums.py Outdated
Comment thread ryan-scripts/tuflow/tuflow_culvert_from_maximums.py Outdated

@Chain-Frost Chain-Frost left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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.

@phantom-branch phantom-branch self-assigned this Oct 8, 2026
@Chain-Frost Chain-Frost self-assigned this Oct 13, 2026

This branch has not been deployed

No deployments
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.

Migrate TUFLOW culvert demos and add selectable HY-8 / ryan-culverts engine Review ownership of TUFLOW/domain-specific demo scripts

2 participants