Skip to content

[PROBE: mass] add aquifer recharge-response ordering - #152

Open
JustinLoiii wants to merge 6 commits into
Flood-Lab:mainfrom
JustinLoiii:probe/135-aquifer-recharge-ordering
Open

JustinLoiii wants to merge 6 commits into
Flood-Lab:mainfrom
JustinLoiii:probe/135-aquifer-recharge-ordering

Conversation

@JustinLoiii

Copy link
Copy Markdown
Contributor

Summary

Implements the accepted aquifer recharge-response ordering probe from #135.

The probe compares:

  • control
  • recharge_added

The only intervention is a nonnegative direct gw_recharge pulse. The new aquifer_recharge_ordering criterion checks:

  • added recharge never lowers groundwater storage;
  • incremental exchange does not reverse direction;
  • added recharge is partitioned between storage and cumulative incremental exchange;
  • the cumulative budget residual remains within the recharge-scaled tolerance.

The PR includes exact, recharge-blind, and nonmonotone overshoot reference models, documentation, site metadata, and archive synchronization.

Validation

  • ht validate passes
  • ht gate --probe mass/aquifer-recharge-ordering passes
  • documentation synchronization tests pass

Closes #135

@chrimerss

Copy link
Copy Markdown
Contributor

/review

@github-actions github-actions 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.

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

Adds mass/aquifer-recharge-ordering: a two-variant probe (control, recharge_added) whose only intervention is a 5 mm/day gw_recharge pulse on the first two scored days, plus a new paired criterion aquifer_recharge_ordering scoring three arms against one tolerance — storage never falls (E_S), incremental exchange never reverses (E_Q), and the paired budget ΔS + ΣΔQ = ΣΔR closes (E_B) — three reference adapters, and the docs/site/archive updates.

The physics checks out: with gw_sw_exchange positive into the aquifer the budget identity and both sign conventions are right, reference_recharge_order_exact closes it to float noise, and reference_recharge_overshoot (storage arm) and reference_recharge_blind (budget arm) separate cleanly.

The most important thing: the probe's acceptance rests entirely on a reference written alongside the criterion. must_pass names only reference_recharge_order_exact, and all ten archived rows are N/A, so no physical model passes it. docs/writing-a-probe.md allows gating on an exact reference alone only when the README says why and a submitted physical model that has the process passes in models/result.csv — as modflow6 does for mass/gw-sw-exchange-consistency. Here modflow6 reports every required variable and is excluded only by supports: perturbation: false.

Second: the ten archive rows carry a detail string the harness never emits, and modflow6's would be INCOMPATIBLE, not INCOMPLETE — they were written by hand, which AGENTS.md item 5 forbids.

Posted by the pr-review workflow. Run

Comment thread models/result.csv Outdated
Comment thread probes/mass/aquifer-recharge-ordering/probe.yaml
Comment thread src/hydroturing/criteria/recharge_ordering.py Outdated
Comment thread src/hydroturing/criteria/recharge_ordering.py Outdated
Comment thread probes/mass/aquifer-recharge-ordering/probe.yaml
Comment thread README.md Outdated
Comment thread src/hydroturing/criteria/recharge_ordering.py Outdated
Comment thread models/reference_recharge_blind/ht_adapter.py Outdated
Comment thread probes/mass/aquifer-recharge-ordering/probe.yaml Outdated
Comment thread src/hydroturing/criteria/recharge_ordering.py
@chrimerss
chrimerss requested a review from Barbhuiya12 October 4, 2026 21:31

@Barbhuiya12 Barbhuiya12 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed at eda8355. ht validate, ht gate --probe mass/aquifer-recharge-ordering and tests/test_recharge_ordering.py pass here. The ordering question is a good one, and the exact reference closes the paired budget to 6e-12 mm. reference_recharge_overshoot trips this criterion alone, so the storage arm is isolated by a baseline. Two things need to change before this can merge, though, and one of them is about what the probe can actually detect.

1. The exchange arm can't see a reversed exchange in this case

The case's river is too weak for the incremental exchange to clear the tolerance. With river_conductance_m2_per_day: 100 over 1 km² and S_y = 0.2, the aquifer drains to the river on a time constant of S_y · A / C ≈ 2000 days. So over the 18 scored days the pulse's 10 mm raises the exchange by about 0.08 mm in total, while ε = max(0.01 · I*, …) = 0.1 mm. Every possible reversal is smaller than the tolerance.

I took reference_recharge_order_exact's output in the runner and reversed its incremental exchange in the perturbed run (q_p' = q_c − (q_p − q_c)). I adjusted gw so each run's own balance still closes, and split the components to match. On the three gate seeds:

river conductance exact reference reversed exchange
100 (this PR) PASS, E_Q 0 PASS, E_Q 0.082 mm against ε 0.1 mm
1000 PASS FAIL, E_Q 0.794 mm (7.9 × ε)
5000 PASS FAIL, E_Q 3.41 mm (34 × ε)

groundwater_balance, exchange_components and state_bounds pass the reversed run at every conductance, as they should, so nothing else in the probe catches it. The gate stays green at 1000 and at 5000 with both must_fail baselines tripping as declared. I'd raise the case's conductance to 1000, which is the value mass/groundwater-datum-invariance uses, and re-archive MODFLOW 6 on the changed case. A reversed-exchange case in tests/test_recharge_ordering.py built on the real generator, or a third must_fail reference, would then pin the arm the gate currently can't reach.

2. The branch carries the MODFLOW 6 adapter #151 replaced

It is 23 commits behind main and conflicts in nine files. models/modflow6/ht_adapter.py and model.yaml are the two that matter. This branch still has the adapter.2 that reads static["aquifer_top_m"] by subscript and lists both keys under needs_static. That is the version that made modflow6 N/A on mass/gw-sw-exchange-consistency, the archived pass that probe's exact-reference exception rests on. #151 merged the fix: uses_static, with 20 m / 0 m as the fallback. When you merge main, please take main's adapter and manifest for both files. This probe's generator supplies both keys, so nothing here depends on them being required. Then re-archive modflow6 on this probe under main's adapter, so adapter.2 means one thing in models/result.csv.

3. Not blocking: the budget arm restates groundwater_balance

E_B = max|ΔS_n + D_n + B_n − I_n| is the perturbed run's own balance minus the control's. If each run closes its own budget, which groundwater_balance with all_variants: true already requires, E_B is zero up to the two tolerances. The gate shows it: reference_recharge_blind trips groundwater_balance as well as this criterion, and no baseline trips E_B alone. That's fine to keep as a paired restatement, but the README should say so, so that the probe's independent content reads as what it is: the two ordering arms. After point 1, both of those are live.

What holds: the per-step nonnegativity check on the added recharge, the explicit MODE constants, the declared statics, and the storage floor capped at 0.05 mm. All of the automated review's points are addressed at this head.

Requesting changes for 1 and 2.

@JustinLoiii

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I have addressed the requested changes.

  • Increased river_conductance_m2_per_day from 100.0 to 1000.0, so the reversed exchange component is large enough for E_Q to detect.
  • Synchronized the branch with the latest main.
  • Adopted the current MODFLOW 6 adapter and static-field declarations from the merged groundwater datum work.
  • Re-ran MODFLOW 6 on mass/aquifer-recharge-ordering; it now passes with:
    aquifer_recharge_ordering: added recharge preserves storage and exchange ordering.
  • Re-generated the archive entries for all evaluated models and updated the README, contributor list, site standings, and documentation.
  • Clarified in the README that E_B is the paired restatement of the groundwater_balance criterion.

Validation completed:

  • ht validate — passed
  • ht gate --probe mass/aquifer-recharge-ordering — passed
  • pytest -q --basetemp=.pytest-tmp — 1280 passed, 8 skipped

The changes are pushed in commit 4f26803.

@chrimerss
chrimerss requested a review from Barbhuiya12 October 5, 2026 21:44
Barbhuiya12
Barbhuiya12 previously approved these changes Oct 6, 2026

@Barbhuiya12 Barbhuiya12 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed at 4f26803, which is current with main. Both blocking points are fixed.

  • The exchange arm is live. At river_conductance_m2_per_day: 1000 the same construction as before, reference_recharge_order_exact with its incremental exchange reversed and gw adjusted so each run still closes its own balance, now fails on the three gate seeds with E_Q 0.794 mm against ε 0.1 mm. The unmodified reference still passes with E_Q 0 and E_B 6e-12 mm. groundwater_balance, exchange_components and state_bounds pass the reversed run, so the catch is this criterion's own.
  • MODFLOW 6 is main's. The adapter and manifest are byte-identical to main, and modflow6 is archived PASS on mass/aquifer-recharge-ordering, mass/groundwater-datum-invariance and mass/gw-sw-exchange-consistency.

ht validate (36 probes, 83 models), the probe gate, the gw-sw-exchange-consistency gate and the full pytest -q (1288 passed) pass here.

One small thing, not blocking: your reply mentions a README note that E_B restates groundwater_balance, but I don't see it at this head. The README lists the three arms without saying which are independent. It's one sentence whenever you next touch the file.

Approving.

…he site

- aquifer_recharge_ordering credited a declared gw_boundary out of the
  aquifer (+B) where groundwater_balance has it in (dS = R + Q + B), so a
  correct model with a head-dependent boundary was off by twice the
  boundary's response: 1.7 times the tolerance already at 0.1 mm/day per m.
  It is now -B and closes to 1e-11 mm. The probe's groundwater_balance
  declares the same source.
- The exchange arm was untested: the "reversal" test sent the increment
  the right way and failed on E_B, and deleting E_Q left the tests and the
  gate green. tests/test_recharge_ordering.py now builds every case on the
  probe's generator and checks that each arm is the one that catches its
  fault: a reversed increment that keeps each budget closed trips E_Q
  alone, and E_B is pinned as a running maximum. Removing any arm, or
  flipping the boundary sign, fails a test.
- The criterion fails a non-finite output itself; it picked the worst arm
  with max() over the ratios, which passes a NaN exchange. The protocol
  already rejects NaN columns, so this was latent.
- The criterion and the three reference adapters are rewritten in the
  repository's style; the exact reference now integrates the cell exactly
  over each day. Same case, same verdicts.
- Probe README: the arms, the tolerance, the boundary, which content is
  independent (E_B largely restates groundwater_balance), which arms each
  reference trips, and what the probe does not catch. The order is non-strict,
  so a model that banks the pulse passes; a recovery check needs the
  aquifer's response time, which for MODFLOW 6's 10 x 10 grid at K = 1 m/day
  is thousands of days and is not derivable from the case's attributes.
  A test pins the gap.
- site/index.html: the modflow6 models row had been replaced by "$13 / 3"
  in all three languages; restored as 3 / 3. The flowchart's new entry
  overprinted "datum"; it now follows it, the three pillars are 6 px
  taller, and the 24 MASS entries are evenly spaced, which also clears the
  spin-up/drift overlap main had.
- README: thirty-six probes, twenty-four mass, three groundwater-exchange
  probes; reference descriptions match what the adapters do.
- models/result.csv: the ten rows re-archived on the gate seeds;
  modflow6 still passes (E_B 7e-10 mm), the other nine are N/A as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@Barbhuiya12 Barbhuiya12 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed at dc363ee. It merges with main without conflicts. ht validate (36 probes, 83 models), ht gate --probe mass/aquifer-recharge-ordering, and tests/test_recharge_ordering.py, test_docs_in_sync.py and test_report_detail.py (304) pass here.

I checked the two changes that matter beyond the rewrite.

The boundary sign. With a declared boundary flux positive into the aquifer, a rise in it adds storage, so the paired identity is ΔS + D − B − I = 0. That is −B, and it is the same convention groundwater_balance uses. The earlier +B was off by twice the boundary's response, as the commit message says. Flipping it back fails two tests.

The arms are pinned. I broke each in turn in recharge_ordering.py: dropping E_Q fails one test, dropping E_S fails one, dropping E_B fails three, flipping the boundary sign fails two, and taking the budget at the last step instead of its running maximum fails one. The restored file passes all 11. That closes the gap I raised earlier, where the exchange arm could be deleted with the gate and the suite still green.

The README now says which content is independent (E_B largely restates groundwater_balance) and what the probe does not catch, including that a model which banks the pulse passes. That is the honest statement of the limit.

Approving.

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] Aquifer recharge-response order preservation

3 participants