Repository navigation
Gridded routing docs, skills, and two Q' reader fixes - #195
Merged
Merged
Conversation
ddr-training covers the standard CLI and Hydra path: picking a config, dotted overrides for smoke tests, what the fields mean, how to read neg_solve and parameter spread, the common failure modes, and the rule that a run has proved nothing until it beats the summed-Q' baseline. ddr-gridded covers the DDM30 path: the self-contained example, CONUS training, building the stores, and the facts that are easy to get wrong - cell and node id schemes, the 2,350 km2 resolution floor on basins, attributes from native rasters only, forcing aggregated rather than interpolated, why negative Muskingum coefficients are expected here, and how sub-reach sizing targets Courant 1. It also records the two unresolved findings so learned parameters are not presented as calibrated. .claude/ was ignored wholesale, so skills could not be committed as the project instructions require. Narrowed to .claude/* with an exception for the skills library; settings.local.json and worktrees stay ignored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZBLnrygRbcnyQvu2KL4vz
docs/gridded_methods.md describes how gridded routing works and where every input comes from, written for a collaborator who knows hydrology but has not seen the code. Covers the DDM30 network and its three undocumented deviations, sub-reach discretization with the Courant-band derivation, the attribute sources and the native-raster aggregation rule, mass-conserving transfer of lateral inflow, the negative-coefficient regime and why it does not bias the water balance, parameter learning, evaluation against the summed-inflow control, the verification suite, and the three limitations that bear on interpreting learned parameters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZBLnrygRbcnyQvu2KL4vz
Six figures move from the gitignored run directories into docs/images so the methods section shows its results rather than asserting them: the learned parameter maps, the Juniata hydrograph against the summed-inflow control, continental metrics, parameter convergence, the channel-geometry check, and parameter relations. Each carries a caption stating what it shows. The convergence and geometry figures sit in the limitations section, where they are evidence for the two unresolved problems. *.png was ignored wholesale, so documentation figures could not be committed; added an exception for docs/images. README gains a gridded routing section and credits Song, Bindas et al. (2025): the lateral inflow and the attribute set both come from that dataset, now cited with its Zenodo archive alongside the existing paper citation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZBLnrygRbcnyQvu2KL4vz
Grid resolution decides which gauges are usable, so selection is a filter chain rather than a preference: 3,211 in gages_3000, 890 above 2,000 km2, 667 snapping within drainage-area tolerance, 620 with sufficient observation coverage. Both the README and the data guide now show the funnel with the control for each step. Records why nearest-centre snapping was rejected, since it is the obvious first thing to try: it misplaced about half of the large gauges, by up to a factor of 201 in drainage area, because a gauge near a cell edge is often closest to a cell its river never enters. Matching on accumulated area tests the hydrologic connection instead of assuming proximity implies it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZBLnrygRbcnyQvu2KL4vz
DA_TOLERANCE = (0.7, 1.4) bounds the ratio of a cell's accumulated area to the gauge's reported drainage area. Records why the bounds are asymmetric in linear terms but symmetric in log terms, since drainage area is multiplicative and the matching minimises absolute log error; that it rejects rather than constrains the search; and that it binds near the resolution floor rather than for large basins. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZBLnrygRbcnyQvu2KL4vz
This was referenced Sep 10, 2026
taddyb
added a commit
that referenced
this pull request
Sep 11, 2026
scripts/summed_q_prime.py indexed Qr positionally in two places, taking axis 0 as divides and axis 1 as time. That is correct only for a store written in contract order, and the failure is silent rather than loud: zarr answers an out-of-range read with fill values instead of raising, so a transposed store yields an all-NaN lateral inflow and a baseline of zero. Both reads now go through ddr.io.readers.qr_as_divide_time, which selects by dimension name and refuses a variable whose dimensions match neither layout. The helper lives in the reader module rather than the script because the script imports cupy at module scope and so cannot be imported without a GPU, which is why the existing tests reimplement its logic inline. Same class of defect as the gridded fixes in #195, on the MERIT path. 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.
Follow-up to #194: the documentation that work needed, plus two bug fixes it exposed.
#194 landed the gridded routing path but left a gap — every acquisition instruction lived in
wiki/, which is gitignored, so none of it reached a user. This PR closes that and fixes two defects found while writing it up.Documentation
docs/gridded_methods.md— a methods section written for a collaborator who knows hydrology but has not seen the code. Nine sections take a reader from the DDM30 network to the limitations, with six figures from the shipped runs. It covers the three undocumented deviations in the ISIMIP product, the Courant-band derivation behind sub-reach sizing, an attribute table with the reason each source deviates from Song et al. (2025) where it does, why moving lateral inflow onto cells is mass-conserving aggregation rather than interpolation, and why the negative-coefficient regime cannot bias the water balance.docs/gridded_data.mdgains a full gauge-selection section, and the README gains a gridded section with the same in brief.Two skills,
.claude/skills/ddr-trainingand.claude/skills/ddr-gridded, covering the standard CLI path and the gridded path..claude/was ignored wholesale, so the skills library could not be committed as the project instructions require; narrowed to.claude/*with an exception forskills/.Dataset credit — the README now names Song, Bindas et al. (2025) as the source of both the lateral inflow and the attribute set, with the Zenodo archive alongside the paper DOI.
How the gauges are selected
Documented in both the README and the data guide, because grid resolution decides the answer rather than preference. A 0.5° cell is about 2,350 km², so a basin smaller than one routing element cannot be represented.
gages_3000.csvSnapping searches the 3×3 neighbourhood and keeps the cell minimising
|log(cell_upstream_area / gauge_drainage_area)|, withDA_TOLERANCE = (0.7, 1.4)rejecting the match if even the closest is too far off. Those bounds are asymmetric in linear terms but symmetric in log terms, which is the right symmetry for a multiplicative quantity. Nearest-centre snapping was tried first and rejected: it misplaced about half of the large gauges, by up to a factor of 201 in drainage area, because a gauge near a cell edge is often closest to a cell its river never enters.Bug fixes
Both are instances of one failure mode: zarr answers an out-of-range read with fill values rather than raising, so a mismatched reader produces an all-NaN lateral inflow and a silently zero baseline instead of an error.
The Q′ store was written transposed.
build_gridded_qprime.pyemittedQr(time, divide_id), the transpose of the contract the dHBV2 UH source follows. Regenerated the CONUS store and the shipped bundle in contract order. Values are unchanged: volume on a sample day matches the source exactly, the Juniata four-cell mean is 163.0 m³/s as before, and the example still trains to NSE 0.795 against the summed baseline's 0.594.The benchmark read
Qrpositionally._aggregate_qprimeusedshape[1]as the time length, correct only because the source store happens to be in contract order. Reading now goes throughqprime_matrix, which selects and transposes by dimension name and refuses a variable whose dimensions match neither layout. Two tests cover it. Also removed a_t0parameter threaded into_route_and_scorethat was never read.Verified behaviour-preserving by re-running the CONUS benchmark end to end: median NSE 0.5872 against the baseline's 0.4375, beating it at 84.7% of 321 gauges, negative solves 0.0488% — identical to the numbers in #194.
Not changed, worth a decision
scripts/summed_q_prime.pyreadsQrpositionally in two places. It is the production MERIT path rather than gridded work, and it is correct for contract-order stores, but it would hit the same trap on a transposed one.811 tests pass, 2 skipped.