Skip to content

[PROBE: mass] interception-response: a larger canopy must return the water as evaporation - #162

Open
kawh1111 wants to merge 1 commit into
Flood-Lab:mainfrom
kawh1111:probe/mass-interception-response
Open

kawh1111 wants to merge 1 commit into
Flood-Lab:mainfrom
kawh1111:probe/mass-interception-response

Conversation

@kawh1111

Copy link
Copy Markdown
Collaborator

mass/interception-response: a larger canopy must return the water as evaporation

Closes #161.

What this adds

A paired static counterfactual. The same three-year temperate record is run
twice, with byte-identical weather and only static.canopy_capacity_mm
changed from 2 mm to 10 mm (Delta cap = 8 mm). A larger canopy intercepts
more water, and the probe asks where it went.

Four checks on Delta X = X(capacity_high) - X(capacity_low):

check requirement
runoff direction max(0, Delta mrro) / Delta cap <= 0.05
evaporation direction max(0, -Delta evspsbl) / Delta cap <= 0.05
paired allocation abs(R) / Delta cap <= 0.05, with R = Delta evspsbl + Delta mrro + Delta gwex + Delta(stores)
response exists Delta evspsbl / Delta cap >= 0.5

An absolute floor of 1 mm on the residual makes the effective limit 12.5% at
Delta cap = 8 mm; the exact bucket closes to better than 0.01 mm. The floor
is on evaporation rather than the canopy-store peak so a model that routes
interception loss directly to evaporation is not failed for its bookkeeping.

The capacity pair is 2 -> 10 mm rather than 2 -> 20 mm because LISFLOOD's
lai_for_canopy_capacity conversion tops out near 11.7 mm; 10 mm keeps every
model that currently declares the static representable. The change is still a
factor of five.

Implementation

  • New paired criterion interception_response in
    src/hydroturing/criteria/interception.py, registered in criteria/__init__.py.
    Delta cap is read from the two runs' case.static, not from a number in the
    probe file, so the criterion cannot drift from the generator.
  • probes/mass/interception-response/{probe.yaml,generate.py,README.md}.
    The generator draws the weather once, before the variant branch, and ends
    with a 90-day dry spell that drains both canopies.
  • Four synthetic negative controls, one per check:
    reference_interception_blind (4), reference_interception_wetter (1),
    reference_interception_drier (2), reference_interception_leaky (3).
    Each is in baselines.must_fail_only, so the gate requires it to trip this
    criterion alone.
  • spec.py: the four controls added to TRUSTED_SUBPROCESS_MODELS.
  • tests/test_interception.py; tests/test_report_detail.py headline updated.

Gate and tests

ht validate -> 38 probe(s), 88 model(s)                    (exit 0)
pytest -q   -> 1376 passed, 10 skipped                     (exit 0)
ht gate     -> GATE PASSED, every probe separates its
               reference models as declared                (exit 0)

Probe gate:

must_pass  reference_bucket             interception_response=pass
must_pass  flex_lumped                  interception_response=pass
must_pass  flex_topo                    interception_response=pass
must_pass  lisflood                     interception_response=pass
must_pass  summa                        interception_response=pass
must_fail  reference_interception_blind  tripped ['interception_response']
must_fail  reference_interception_wetter tripped ['interception_response']
must_fail  reference_interception_drier  tripped ['interception_response']
must_fail  reference_interception_leaky  tripped ['interception_response']

Archive rows (all evaluated models)

Generated with ht run --probe mass/interception-response --gate-seeds --csv,
including the Docker models:

model verdict detail
flex_lumped PASS evaporation +154.76 mm, runoff -143.44 mm
flex_topo PASS evaporation +188.87 mm, runoff -179.03 mm
lisflood PASS evaporation +148.76 mm, runoff -157.24 mm
summa PASS evaporation +157.80 mm, runoff -177.11 mm
cwatm FAIL (VIOLATION) paired residual -1.54 mm, 19.2% of 8 mm (limit 12.5%)
wflow_sbm FAIL (VIOLATION) evaporation response +0.07 mm, 0.8% (floor 50%)
dhbv2, google_flood_forecast, modflow6, sacsma_snow17 N/A (INCOMPATIBLE) do not declare canopy_capacity_mm

Two findings to note rather than hide:

  • CWatM closes the paired budget to 1.54 mm on an 8 mm capacity change, 19.2%
    against the 12.5% effective limit. Its native 1.11 run at 2 -> 20 mm passed
    the same check at 0.784 mm of 0.9 mm (reported by @cehw in [PROBE] mass/interception-response: the water a larger canopy intercepts has to leave as evaporation #161), so this is
    a property of the 2 -> 10 mm case, not a gross leak.
  • Wflow declares canopy_capacity_mm but its evaporation moves by 0.07 mm on
    an 8 mm capacity change. The adapter is the first place to look; the probe
    is reporting that the declared parameter is not reaching interception.

Prose and site

README.md (probe row, counts, reference-model rows), CONTRIBUTORS.md,
ROADMAP.md, docs/writing-a-probe.md (criterion table), site/index.html
(three languages, flowchart entry, standings regenerated with
scripts/site_standings.py), and models/result.csv.

Credit

Probe author: Yuanhang Liu (Independent Researcher, ORCID
0009-0005-9633-6004).

Checklist

  • ht validate and ht gate
  • full pytest
  • test_docs_in_sync (archive, README, CONTRIBUTORS, ROADMAP, site, flowchart, charts)
  • archive rows for every evaluated model, including Docker
  • no unrelated file changes

… as evaporation

The probe runs the same three-year weather with canopy capacity 2 mm and
10 mm, and scores where the intercepted water went: runoff may not rise,
evaporation may not fall, the paired budget must account for the difference,
and the evaporation response must exist. A new paired criterion reads the
capacity change from the two runs' case static. Four synthetic controls pin
one sub-check each.

Closes Flood-Lab#161.
@chrimerss

Copy link
Copy Markdown
Contributor

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated review by Claude (changes-suggested), not a maintainer approval.

Adds mass/interception-response: the same three-year temperate record run twice with static.canopy_capacity_mm at 2 mm and 10 mm, scored by a new paired criterion interception_response (runoff may not rise, evaporation may not fall, the paired budget must close, and the evaporation response must exist). Four synthetic controls, one per check, plus README/CONTRIBUTORS/ROADMAP/site/archive updates. The design is sound and the docs-in-sync checklist looks complete (CITATION.cff already carries the author; the site counts reconcile with the new archive rows).

Two things a maintainer should decide before merging.

First, the paired residual adds Delta gwex with the sign of a loss (interception.py:140). Everywhere else in the harness — closure.py:116, event_water_closure.py:136, AGENTS.md:223 — gwex is positive into the catchment, so it must be subtracted. It is latent today only because every model this probe can score reports gwex = 0 without an abstr column; it is wrong the moment one does not, and the probe README publishes the wrong formula.

Second, baselines.must_pass includes lisflood and summa, both runner: docker. No other probe gates on a submitted or containerised model. ht gate builds and runs every must_pass model, so the probe workflow's 15-minute gate job — which has no Docker setup and is described as fast because baselines are subprocess — will now build two heavy images on every probe, harness, models, site, scripts or tests pull request.

Posted by the pr-review workflow. Run

else:
d_storage = 0.0

residual = d_evspsbl + d_mrro + d_gwex + d_storage

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high: Delta gwex enters the paired residual with the wrong sign

residual = d_evspsbl + d_mrro + d_gwex + d_storage treats a declared exchange as a loss. AGENTS.md:223 defines gwex as "positive into the catchment", and every other budget in the harness uses it as a source: closure.py:116 computes drive - outflow - dS with gwex added to drive, and event_water_closure.py:136 computes p + g - e - q - change.

From dS = P + gwex - ET - Q with identical weather (dP = 0), the paired identity is Delta evspsbl + Delta mrro + Delta(dS) - Delta gwex = 0. The criterion therefore double-counts: its residual is off by 2 * Delta gwex.

Failure scenario: a model that moves 3 mm of its response through a declared regional-groundwater term over the window (Delta gwex = -3 mm) and otherwise conserves exactly reports residual = -6 mm instead of 0, i.e. 75% of the 8 mm capacity change against a 12.5% limit, and is archived as FAIL (VIOLATION). Symmetrically, a model that leaks 3 mm while Delta gwex = +1.5 mm reports residual = 0 and passes.

This is latent only because reference_bucket, flex_lumped, flex_topo, lisflood, cwatm and wflow_sbm all report gwex = 0 on a case with no abstr column (see each model.yaml's gwex note) — so none of the archived rows in this PR is affected. But dhbv2 declares a nonzero learned gwex and is INCOMPATIBLE here only for want of the static declaration.

Fix: residual = d_evspsbl + d_mrro - d_gwex + d_storage, and correct the formula in probes/mass/interception-response/README.md:14 to R = Delta evspsbl + Delta mrro - Delta gwex + Delta(stores).

floor_mm: 1.0

baselines:
must_pass: [reference_bucket, flex_lumped, flex_topo, lisflood, summa]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high: must_pass gates the probe on two Docker models, which the CI gate job cannot build

must_pass: [reference_bucket, flex_lumped, flex_topo, lisflood, summa]. Both lisflood and summa declare runner: docker (models/lisflood/model.yaml:27, models/summa/model.yaml:21). No other probe in the suite names a submitted or containerised model in must_pass.

cmd_gate (cli.py:363) calls run_probe(registry.find_model(name), ...) for every must_pass entry with no runner filter, and docker_runner.build builds the image locally — there is no registry pull (BUILD_TIMEOUT_S = 1800, with a comment that a first build of a heavy image has taken fifteen minutes).

Failure scenario: .github/workflows/probe-pr.yml runs bare ht gate in the gate job, which has timeout-minutes: 15 and no Docker setup step; its own comment says "The reference models run via subprocess so the gate stays fast". After this merge that job must build the LISFLOOD and SUMMA images and then run each over 3 seeds x 2 variants x 1460 steps, on top of ht validate and 1376 pytest cases. model-eval.yml:9 already records that "a heavy model does not fit the 45-minute budget on a four-core runner". The job will time out on every pull request touching probes/, src/, models/, site/, scripts/ or tests/ — not just this one. The same applies to the ht gate --probe mass/interception-response line the probe README offers as the reproduction recipe.

Separately, this inverts the gate's meaning: docs/writing-a-probe.md:319 frames must_pass as the physical baselines that calibrate a tolerance, so a later revision of a submitted adapter would make the probe "wrong until shown otherwise".

Suggested fix: keep must_pass to reference_bucket, flex_lumped, flex_topo, and let the passing lisflood and summa rows in models/result.csv stand as the independent evidence (the pattern docs/writing-a-probe.md:358-365 describes for modflow6).

d_storage = 0.0

residual = d_evspsbl + d_mrro + d_gwex + d_storage
limit = max(tolerance, floor_mm / dcap)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium: The 1 mm floor is applied to the direction checks, which the docs limit to 5%

limit = max(tolerance, floor_mm / dcap) is computed once and then used as the threshold for all three share checks (lines 149, 154, 159). At dcap = 8 that is 12.5% for every one of them.

Every description of the probe says the floor is on the paired residual alone. The probe README lists checks 1 and 2 as <= 0.05 (README.md:21 and :23) and introduces the floor only after them ("The absolute residual floor is 1 mm", :30); probe.yaml:50 says "A 1 mm absolute floor on the paired residual"; the PR description's table says the same.

Failure scenario: a model whose runoff rises by 0.9 mm over the scored window with a larger canopy (runoff_rise = 11.3%) passes check 1, where the documented 5% rule would fail it at anything over 0.4 mm. A maintainer calibrating a future tolerance from the README would compute the wrong headroom.

Fix either way, but pick one: apply floor_mm only to residual_share and compare runoff_rise/evap_fall against tolerance, or state in the README, probe.yaml and the module docstring that the floor covers all three shares.


## Baselines

`must_pass`: `reference_bucket`, `flex_lumped`, `flex_topo`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium: The README's Baselines section contradicts probe.yaml

README.md:69 states "must_pass: reference_bucket, flex_lumped, flex_topo", and :83-84 says "cwatm and summa ... are candidates for must_pass once their Docker rows pass the gate". probe.yaml:57 already lists summa and lisflood in must_pass; lisflood is not mentioned anywhere in the README's baselines section, and cwatm FAILs the probe in the archive row this PR adds (models/result.csv).

Failure scenario: a maintainer reviewing a later tolerance change reads the README, concludes the gate rests on three subprocess references, and does not understand why ht gate is asking for Docker — or why a SUMMA adapter bump broke an unrelated probe. The probe README is the document docs/writing-a-probe.md points contributors at for exactly this.

Fix: whichever must_pass the maintainers settle on (see the finding on probe.yaml:57), make README.md:69 and :83-87 say it, and give lisflood a line.

return CriterionResult(
name="interception_response",
status=PASS if ok else FAIL,
value=residual_share,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium: value/threshold always report the residual share, even when another check failed

value=residual_share, threshold=limit are set unconditionally, but the criterion can fail on any of four checks. For a model that fails only the evaporation-response floor, the result carries value = 0.000 against threshold = 0.125 with status = fail.

This matters twice. The JSON report writes both fields verbatim (report.py:132-133), so a consumer reading value-against-threshold sees a number comfortably inside its limit on a failing criterion — the inverse of the property human.py:115-118 goes out of its way to preserve ("so a pass never reads as value above threshold").

Worse, harness.py:660 picks the reported seed with min(pairs, key=lambda pair: (pair[1].passed, -abs(pair[1].value or 0.0))). Among failing seeds it selects the largest |residual_share|. Failure scenario: wflow_sbm fails this probe on the response floor across three gate seeds; the archived row (models/result.csv, "+0.07 mm, 0.8%") is whichever seed had the biggest paired-budget residual, not the seed with the smallest evaporation response. The published number is not the worst case of the check that actually failed.

Fix: when a check fails, set value/threshold to that check's own measured share and limit (e.g. evap_response against response_floor), so the worst-seed rule and the report describe the failing quantity.



@pytest.mark.parametrize("model", NEGATIVE)
def test_each_negative_control_fails_the_paired_criterion(model):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium: No unit coverage of the criterion's algebra; the gwex branch is never exercised

The new tests only run seven models end to end on one gate seed and assert PASS/FAIL. Nothing calls interception_response with constructed runs, so none of its arithmetic is pinned.

Untested paths: the d_gwex branch (interception.py:123-128) — every model in POSITIVE and NEGATIVE reports gwex = 0 or omits it, which is precisely why the sign error above survives the suite; the floor_mm interaction at limit = max(tolerance, floor_mm / dcap); each of the four checks in isolation; and the three raise ValueError paths (_capacity on a missing or non-numeric static, _same_weather on divergent forcing, dcap <= 0).

Failure scenario: the gwex sign is corrected (or broken again) and the full suite still passes, because no test has a run with a nonzero exchange. The same holds for a future change to the floor.

Fix: add a table-driven test that builds two synthetic RunResults with known evspsbl, mrro, gwex and storage, and asserts the residual, the four verdicts and the error messages — one case with Delta gwex != 0 where the conserving answer is residual == 0.

reference_interception_leaky: interception_response
# Each control is meant to fail through `interception_response` alone, not
# through a contract accident.
must_fail_only:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low: must_fail_only cannot verify the per-check isolation the README claims

The probe declares exactly one criterion, interception_response. cmd_gate (cli.py:379-382) enforces must_fail_only by checking set(tripped) != {expected}, and tripped can only ever be {'interception_response'} here. The entry is therefore vacuous, yet README.md:80-81 and the PR description both present it as proof that each control "trips this criterion alone".

What is actually unverified is the one-control-per-check mapping in the README table at :75-78, and it does not hold:

  • Check 2 is strictly subsumed by check 4. Check 2 fires at Delta evspsbl < -1 mm, check 4 at Delta evspsbl < 4 mm. reference_interception_drier (Delta evspsbl = -4.8 mm) trips both, so it does not isolate check 2 as the table says.
  • reference_interception_wetter and reference_interception_leaky both land at evap_response = 4.8 / 8 = 0.60 against response_floor: 0.5 — 20% of headroom. Raising the floor to 0.65 would make all three remaining controls trip check 4, the gate would still report GATE PASSED, and the probe would have quietly stopped demonstrating that checks 1 and 3 catch anything.

Fix: either add an assertion in tests/test_interception.py that pins which check each control trips (the diagnostics dict already carries every share), or widen the controls' margins against response_floor so the mapping is robust, and drop or justify the must_fail_only block.

# produces no physical response on its own. The paired difference is then
# exactly the synthetic allocation below, which is what the control is
# built to get wrong. `blind` never reads the declared value at all.
cap = 2.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low: Copy-pasted comment about blind appears in all four controls and is wrong in each

The comment "blind never reads the declared value at all" sits in reference_interception_drier, _wetter and _leaky as well as _blind, at line 27 of each.

In the three non-blind adapters it describes a different model and tells the reader nothing about the file they are in. In reference_interception_blind/ht_adapter.py:27 it is also false: line 23 does declared = float(static["canopy_capacity_mm"]), and the value is then simply never used (no MODE == "blind" branch exists), leaving a dead local. The repo's existing shared-file controls handle this better — reference_recharge_blind/ht_adapter.py:3-12 carries one docstring listing all three modes and no per-mode comment claiming to be about a sibling.

Failure scenario: a maintainer debugging why reference_interception_drier produces a given Delta evspsbl reads line 27, believes the declared capacity is unused in that file, and misses that lines 340-345 scale the adjustment by it.

Fix: move the mode descriptions into one module docstring listing all four, as the recharge controls do, and in _blind either drop the unused declared or note explicitly that reading and discarding it is the fault being modelled.

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.

[PROBE] mass/interception-response: the water a larger canopy intercepts has to leave as evaporation

2 participants