Conversation
momentum/froude-regime (#103) scores the larger of `dis` and `mrro` over the area, on the ground that the two are one outflow. Every adapter reports them that way, and catchment closure needs it, since `channel` holds the water still in transit. But AGENTS.md never said so, and CMIP's `mrro` is generated runoff. An honest bucket reporting generated runoff as `mrro` and a 5-day linear-reservoir `dis` fails that probe on all three gate seeds (worst Fr 1.53-1.87). - AGENTS.md: `mrro` is the runoff the routing has released; `dis = mrro * area_km2 / 86.4` where both are reported. - docs/writing-a-probe.md: the froude_subcritical row described the exact-zero fallback that #103 replaced with the larger reading. - tests/test_froude.py: one docstring called a runoff above the discharge an honest routing difference; renamed and reworded.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The documentation compares differently dimensioned flow columns without explicitly stating the required conversion.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Clarifies that mrro and dis represent the same routed outflow.
Changes:
- Defines routed runoff and discharge conversion semantics.
- Updates Froude criterion documentation and test wording.
| File | Description |
|---|---|
AGENTS.md |
Defines mrro/dis contract. |
docs/writing-a-probe.md |
Updates Froude scoring documentation. |
tests/test_froude.py |
Aligns test naming and comments. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| | `mrro` | total runoff | mm/day | | ||
| | `dis` | river discharge | m3/s | | ||
| | `mrro` | total runoff: the water that leaves the catchment as flow over the step, after the model's routing has released it. Runoff generated but still in transit is `channel`, not `mrro`, so a model that routes reports the routed outflow here and not the generated runoff that CMIP's `mrro` names; reporting the generated runoff beside a `channel` store counts the water in transit twice, once as gone and once as stored | mm/day | | ||
| | `dis` | river discharge. Where `mrro` is also reported, the two are one outflow in two units, `dis = mrro × area_km2 / 86.4`; `momentum/froude-regime` scores the larger of the two, so a `dis` below `mrro` is read at `mrro` | m3/s | |
| | `rating_loop` | where the gauge loops against the reach's store, the loop must be small enough to be noise or run the right way: at the same storage the rising limb sits lower than the falling one. A single-valued rating, or a loop below `min_loop_m` with an inconsistent sign across bins, is read as "no loop" and passes | one run | | ||
| | `uniform_flow_friction` | on each labelled low, medium and high steady plateau, the reported discharge and stage must make Manning friction slope agree with the declared bed slope for the explicit rectangular section; CV and first-to-last-quarter trend gates reject blocks that have not converged | one run, three labelled plateaus | | ||
| | `froude_subcritical` | the share of scored steps on which the reach went supercritical (`Fr > 1 + tolerance`) stays within `max_exceed_fraction`, with `Fr = abs(Q) / (w * d**1.5 * sqrt(g))` read from the model's own stage and discharge and the case's declared width, the depth being `stage - bed_elevation_m`; a step is excused only when no flow column the model reports shows water moving, so a zero written into `dis` while `mrro` still carries the water is scored on `mrro`, and a shallow depth is scored at the one-centimetre floor rather than skipped and a depth above `max_depth_m` is refused rather than scored; a model reporting a depth where the contract asks for a level is N/A (INCOMPATIBLE) rather than failed, a non-finite reading is failed rather than dropped out of the mask, and below `min_scored_fraction` steps carrying flow the case is degenerate and the criterion refuses to score | one run | | ||
| | `froude_subcritical` | the share of scored steps on which the reach went supercritical (`Fr > 1 + tolerance`) stays within `max_exceed_fraction`, with `Fr = abs(Q) / (w * d**1.5 * sqrt(g))` read from the model's own stage and discharge and the case's declared width, the depth being `stage - bed_elevation_m`; where a model reports both `dis` and `mrro` the larger of the two is scored, so a `dis` written below the runoff, zeroed or scaled down, is read at `mrro`, and a step is excused only when that larger reading shows no water moving; a shallow depth is scored at the one-centimetre floor rather than skipped and a depth above `max_depth_m` is refused rather than scored; a model reporting a depth where the contract asks for a level is N/A (INCOMPATIBLE) rather than failed, a non-finite reading is failed rather than dropped out of the mask, and below `min_scored_fraction` steps carrying flow the case is degenerate and the criterion refuses to score | one run | |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #103, raised in my approving review there.
momentum/froude-regimescores the larger ofdisandmrroover the catchment area, because the two are one outflow. That holds for every adapter in the repo: each one that reports both setsdis = mrro × area_km2 / 86.4exactly (cwatm, dhbv2, flex_lumped, flex_topo, google_flood_forecast, lisflood, wflow_sbm, sacsma_snow17, the bucket and rating references). Catchment closure needs it too, sincechannelholds the water still in transit. ButAGENTS.mdnever said so, and CMIP'smrro, where the name comes from, is generated runoff.What goes wrong without the sentence. I took an honest bucket that reports generated runoff as
mrro, withdisandstagerouted through a linear reservoir. It fails the Froude probe on all three gate seeds when the reservoir holds water for 5 days (worst Fr 1.53–1.87). At 2 days its worst Fr is 0.85–0.99, close to the 1.05 limit. Reportingmrroas the routed outflow, the same model passes at every reservoir length. No model scored today reportsmrrothis way, so no verdict moves.Changes
AGENTS.md: themrrorow says it is the runoff the routing has released, with in-transit water underchannel. Thedisrow gives the conversion and says that the Froude probe reads the larger of the two columns.docs/writing-a-probe.md: thefroude_subcriticalrow still described the exact-zero fallback from a3deccb. It now describes the larger-reading rule that merged.tests/test_froude.py: one docstring called a runoff above the discharge "an honest difference" from routing lag. That contradicts the contract as now written, so the test is renamed and reworded. The assertions are unchanged.pytest -q: 1267 passed. Docs and tests only, so the probe workflow does not run.