Repository navigation
Measure the gridded benchmark with the forcing training reads - #197
Merged
taddyb merged 4 commits intoSep 11, 2026
Merged
Conversation
The regridding script emitted Qr(time, divide_id), the transpose of the contract the dHBV2 UH store follows. That is silently readable rather than an error: an out-of-range read returns fill values, so a reader assuming the contract gets an all-NaN lateral inflow and a zero baseline instead of a failure. Regenerated the CONUS store and the shipped bundle. Values are unchanged - volume on a sample day matches the source exactly, the Juniata four-cell mean is 163.0 m3/s as before, and the example still trains to NSE 0.792 / KGE 0.735 against the summed baseline's 0.594 / 0.671. Every reader here already transposed by dimension name rather than position, so none needed changing; documented that requirement alongside the contract so the failure mode is visible to the next writer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZBLnrygRbcnyQvu2KL4vz
_aggregate_qprime indexed Qr positionally, using shape[1] as the time length. That is correct only because the source store happens to be in contract order, and it is the failure mode that silently produces an all-NaN inflow: zarr answers an out-of-range read with fill values rather than raising, so a transposed store yields a zero baseline instead of an error. qprime_matrix now selects and transposes by dimension name and refuses a variable whose dimensions match neither layout. Two tests cover it: a store written each way round must aggregate identically, and a three-dimensional variable must raise. Also removed the _t0 parameter threaded into _route_and_score, which was never read because the routing timer is started inside it. Behaviour is unchanged: the CONUS benchmark reproduces median NSE 0.5872 against the summed baseline's 0.4375, beating it at 84.7% of 321 gauges. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZBLnrygRbcnyQvu2KL4vz
The benchmark built its forcing by flowline-midpoint assignment while training reads the area-weighted store, so the two measured different inflow. About 40% of catchments straddle a cell edge, and midpoint assignment puts all of such a catchment's water in one cell. GriddedPaths gains an optional qprime_icechunk. When set, forced cells and their inflow come from the regridded store, matching training; when unset the midpoint path still runs, so the two can be compared rather than one silently replacing the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZBLnrygRbcnyQvu2KL4vz
The acceptance test measured midpoint-assigned inflow while training reads the area-weighted store, so the gate was not checking what the model uses. It now passes qprime_icechunk and skips if that store is absent. All five gates pass: 321 gauges, neg_solve 0.0488%, median NSE 0.614 against the 0.55 threshold, routing beating the baseline at 92.5% against 75%. Records in the methods doc that the two forcings agree in aggregate, a paired median difference of 0.0003 across 321 gauges, but disagree by more than 0.05 at 71 of them, so they are not interchangeable per basin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZBLnrygRbcnyQvu2KL4vz
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.
The gridded benchmark built its forcing by flowline-midpoint assignment while training reads the area-weighted store, so the two measured different inflow. This makes them agree, and recovers two commits that the #195 merge left behind.
The inconsistency
Runoff arrives as one value per MERIT unit catchment and has to be placed into cells. Midpoint assignment drops a catchment's entire discharge into whichever cell contains its flowline midpoint. About 40% of catchments straddle a cell edge, so that misplaces water. Training moved to area weighting in #194, which splits each catchment's discharge across the cells its polygon overlaps in proportion to area; the benchmark did not.
GriddedPathsnow takes an optionalqprime_icechunk. When set, both the forced-cell set and the per-node inflow come from the regridded store. The midpoint path still runs when it is unset, so the two can be compared rather than one silently replacing the other.What the switch actually changes
Same 321 gauges, same two-year window, fixed parameters:
Paired per gauge: median ΔNSE +0.0003, 51.4% improved, and 71 of 321 gauges move by more than 0.05.
So the two are indistinguishable in aggregate but not interchangeable for an individual basin. The medians move in opposite directions for NSE and KGE, which is what noise looks like; the per-gauge spread is the real finding.
The node count is the substantive difference. Midpoint marked 7,994 nodes as carrying water against 4,428, because it assigned cells from all 346,327 MERIT flowlines while only 197,088 have runoff. Roughly 3,600 of those nodes were routing zero inflow, inflating the apparent network without contributing water.
Acceptance test
Now gated on the forcing training reads, and skips if that store is absent. All five gates pass: 321 gauges, neg_solve 0.0488%, median NSE 0.614 against a 0.55 threshold, routing beating the baseline at 92.5% against 75%.
Two commits recovered
The #195 merge took only up to
316d19a, so two commits pushed after approval were orphaned and are cherry-picked here:Qr(divide_id, time)— the regridding script emitted the transpose of the contract. Regenerated the CONUS store and the shipped bundle; values unchanged, and the example still trains to NSE 0.795._aggregate_qprimeindexed axes positionally, correct only for a contract-order store. Reading goes throughqprime_matrix, which transposes by name and refuses dimensions matching neither layout.Same pattern as the skills commit and #194. Worth checking nothing else from those pushes is missing.
Still open
The 71 gauges that moved by more than 0.05 are unexplained. If they cluster on basins whose catchments straddle cell edges that would confirm the mechanism, which is currently inferred rather than demonstrated.
63 benchmark tests pass, plus the 5 integration gates.