feat(optimization): share the calibration-period slice with in-memory workers - #392
Merged
Merged
Conversation
… workers The differentiable losses in the JAX model packages (jHBV, jTOPMODEL, jSAC-SMA, jXAJ, jHEC-HMS) each need to score the same window the optimizer is reported against. Only jHBV had that logic, and the rest scored everything after warmup — training gradient-based optimizers on the held-out evaluation period. Add get_calibration_slice() to InMemoryModelWorker so every in-memory worker inherits one implementation, plus a warmup_steps() hook that models on a sub-daily timestep override to convert days to timesteps. Ported from 6e6d986 on the repro clone, where this module still lives at symfluence/optimization/workers/; here it has moved to symfluence/core/calibration/workers/ behind a back-compat shim, so the change is applied at the new canonical location. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DarriEy
pushed a commit
to symfluence-org/jhbv
that referenced
this pull request
Jul 28, 2026
…e base method CI resolves symfluence from PyPI (0.9.2), which predates InMemoryModelWorker.get_calibration_slice — added in symfluence-org/SYMFLUENCE#392 alongside this change — so these five cases failed with AttributeError rather than exercising anything. Guard them on the capability instead of pinning a version. They skip until a symfluence carrying the method is released, then activate on their own with no follow-up edit. Verified both directions: 5 pass against the new base, 5 skip when the method is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DarriEy
pushed a commit
to symfluence-org/jXAJ
that referenced
this pull request
Jul 28, 2026
jXAJ was the only model plugin with no .github/workflows at all, so its PRs got no automated verification. Ports jTOPMODEL's tests.yml (pytest on 3.11 and 3.12) unchanged, and declares symfluence in the dev extra to match, since the calibration worker and plugin-registration tests import it. Turning CI on immediately surfaced a broken test: test_registration imported XinanjiangPostprocessor, but the class — and everything register() actually wires up — is XinanjiangPostProcessor. The sibling assertion for XinanjiangPreProcessor had the capitalisation right. The test was wrong, not the code, so this is a one-character fix rather than a rename. Full suite: 53 pass. Also verified green against a symfluence predating get_calibration_slice, which is what CI will resolve from PyPI until symfluence-org/SYMFLUENCE#392 ships. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DarriEy
added a commit
to symfluence-org/jhbv
that referenced
this pull request
Jul 28, 2026
* fix(calibration): enforce k0 > k1 > k2 on every simulation path The recession-ordering constraint was applied only in HBVWorker._run_simulation, which sorts the coefficients before simulating. The JAX loss that ADAM and L-BFGS differentiate called simulate_jax directly and skipped it, so a gradient run optimized an unconstrained model, converged on an unordered parameter set, and reported that model's score — while the final evaluation re-ran the same parameters through the sorting path and scored a different model. On the P3 Bow-at-Banff ensemble this showed up as ADAM reporting best 0.9373 against Calib_KGE 0.5860, the only mismatch among the 17 algorithms; the converged set had k1 = 0.0100 < k2 = 0.0327. Move the constraint into create_params_from_dict, the one function every simulation path already goes through, so the two paths cannot drift apart again. Sorting is differentiable almost everywhere — it is a permutation, and the gradient reaches whichever slot each value lands in — so gradient-based optimizers still descend on the constrained objective. Reparameterizing instead (k1 as a fraction of k0, etc.) would have kept the ordering by construction, but it changes the effective search space for every algorithm and would invalidate the completed population-based runs, which already search the sorted space. Also adopt the shared InMemoryModelWorker.get_calibration_slice() in place of the local copy, supplying the timestep-aware warmup length via warmup_steps(). Verified: ADAM now reports best 0.9331 == Calib_KGE 0.9331 (gap 5e-07). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(calibration): skip worker-slice tests on a symfluence without the base method CI resolves symfluence from PyPI (0.9.2), which predates InMemoryModelWorker.get_calibration_slice — added in symfluence-org/SYMFLUENCE#392 alongside this change — so these five cases failed with AttributeError rather than exercising anything. Guard them on the capability instead of pinning a version. They skip until a symfluence carrying the method is released, then activate on their own with no follow-up edit. Verified both directions: 5 pass against the new base, 5 skip when the method is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: DarriEy <darri.eythorsson@ucalgary.ca> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
DarriEy
added a commit
to symfluence-org/jXAJ
that referenced
this pull request
Jul 28, 2026
…riod (#6) * fix(calibration): score the differentiable loss on the calibration period kge_loss and nse_loss dropped warmup and then scored everything that remained. With a calibration/evaluation split configured, that span covers both windows, so gradient-based optimizers were trained on the held-out evaluation period — the split-sample test was not honest — and the score they reported was computed over a different window than the Calib_* metrics written at final evaluation. This is the same defect found and fixed in jTOPMODEL; Xinanjiang has no completed ADAM run in the P3 ensemble yet, so it is fixed before it can produce misleading numbers rather than in response to one. Thread a calibration slice through the plain and Snow-17-coupled losses, both backends, and the gradient-function factories; the worker supplies it from the shared InMemoryModelWorker.get_calibration_slice(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(calibration): pin that the loss honours the calibration window Guards the leakage fix directly: a cal_slice covering the whole post-warmup record must be a no-op, a narrower slice must change the score, and two disjoint windows must not collapse to the same number. Verified these fail against the pre-fix behaviour — disabling the slice in _eval_window fails 4 of the 6 cases, so an accidental revert cannot pass silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: add the test workflow, and fix the registration test it exposes jXAJ was the only model plugin with no .github/workflows at all, so its PRs got no automated verification. Ports jTOPMODEL's tests.yml (pytest on 3.11 and 3.12) unchanged, and declares symfluence in the dev extra to match, since the calibration worker and plugin-registration tests import it. Turning CI on immediately surfaced a broken test: test_registration imported XinanjiangPostprocessor, but the class — and everything register() actually wires up — is XinanjiangPostProcessor. The sibling assertion for XinanjiangPreProcessor had the capitalisation right. The test was wrong, not the code, so this is a one-character fix rather than a rename. Full suite: 53 pass. Also verified green against a symfluence predating get_calibration_slice, which is what CI will resolve from PyPI until symfluence-org/SYMFLUENCE#392 ships. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: DarriEy <darri.eythorsson@ucalgary.ca> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Jul 28, 2026
Merged
Merged
fix(calibration): tolerate a symfluence without get_calibration_slice (0.6.2)
symfluence-org/cFUSE#2
Merged
DarriEy
pushed a commit
that referenced
this pull request
Jul 30, 2026
…d area fixes 468 runs, 196 rows changed. 137 are on the corrected basis — regenerated after the catchment-area fix landed at 2026-07-28T18:03 — and 77 predate it. The corrected rows carry the five fixes from this cycle: the shared calibration window (#392), HYPE reading its period from a field that does not exist (#393), catchment area resolved from two different shapefiles (#394), point-domain GRU_area written in square degrees (#396), and the seven model-package leakage fixes now on PyPI. Every affected model's calibration objective now agrees with its own final evaluation. Before this cycle the two disagreed by up to 0.35 KGE; across 118 verified runs the worst residual is 5.8e-05 and most sit near 1e-06: HBV 17/17 2.6e-07 HYPE 15/15 3.9e-06 FUSE 15/15 1.4e-06 SACSMA 17/17 5.9e-06 TOPMODEL 17/17 4.2e-06 HECHMS 17/17 5.8e-05 SUMMA 3/15 0.0e+00 The 77 pre-fix rows are identifiable by run_completed < 2026-07-28T18:03 and are dominated by SUMMA (32), which needs ~4.8 h per config and could not be regenerated before this machine was returned. GR (21), GSFLOW (8), JFUSE (6) and RAVEN (4) were outside this cycle's scope. Re-running any of them refreshes the rows in place, as the collector is designed for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DarriEy
added a commit
that referenced
this pull request
Jul 30, 2026
…d area fixes (#397) * repro(macos_p3repro): refresh metrics after the calibration-window and area fixes 468 runs, 196 rows changed. 137 are on the corrected basis — regenerated after the catchment-area fix landed at 2026-07-28T18:03 — and 77 predate it. The corrected rows carry the five fixes from this cycle: the shared calibration window (#392), HYPE reading its period from a field that does not exist (#393), catchment area resolved from two different shapefiles (#394), point-domain GRU_area written in square degrees (#396), and the seven model-package leakage fixes now on PyPI. Every affected model's calibration objective now agrees with its own final evaluation. Before this cycle the two disagreed by up to 0.35 KGE; across 118 verified runs the worst residual is 5.8e-05 and most sit near 1e-06: HBV 17/17 2.6e-07 HYPE 15/15 3.9e-06 FUSE 15/15 1.4e-06 SACSMA 17/17 5.9e-06 TOPMODEL 17/17 4.2e-06 HECHMS 17/17 5.8e-05 SUMMA 3/15 0.0e+00 The 77 pre-fix rows are identifiable by run_completed < 2026-07-28T18:03 and are dominated by SUMMA (32), which needs ~4.8 h per config and could not be regenerated before this machine was returned. GR (21), GSFLOW (8), JFUSE (6) and RAVEN (4) were outside this cycle's scope. Re-running any of them refreshes the rows in place, as the collector is designed for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * repro(macos_p3repro): final collection before the machine was returned Refreshes 7 rows completed after the previous collection. The corrected / pre-fix split is unchanged at 137 / 77. This is the last collection from this box: the reproduction environment was torn down while SUMMA was still working through its re-run. SUMMA's cma-es config was ~40% through generation 20 of 50 and did not complete, so 32 SUMMA rows remain on the pre-fix basis, identifiable as before by run_completed < 2026-07-28T18:03Z. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * repro(macos_p3repro): add the last runs before the machine was returned 469 runs. Adds gr4j, gsflow and a re-run of rhessys, plus the benchmark config that finished during the final window. The rhessys row is the end-to-end confirmation of the basin-area fix: it now reports best_score 0.851883 against Calib_KGE 0.851883 (0.00e+00), where before the fix the same run reported 0.851883 against 0.835738 (1.61e-02) — the same simulation measured against the worldfile's stale 2248.0606 km2 rather than the delineation's 2207.5038 km2. Corrected / pre-fix now 138 / 77. The 77 remain identifiable by run_completed < 2026-07-28T18:03Z and are dominated by SUMMA (32), which needs ~4.8 h per config; its cma-es run reached generation 40 of 50 before the machine went back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: DarriEy <darri.eythorsson@ucalgary.ca> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Why
The differentiable losses in the JAX/Torch model packages (jHBV, jTOPMODEL, jSAC-SMA, jXAJ, jHEC-HMS, jFUSE, cFUSE) each need to score the same window the optimizer is reported against.
Only jHBV had that logic. Every other package dropped warmup and then scored everything that remained — which, with a calibration/evaluation split configured, spans both windows. Gradient-based optimizers were therefore trained on the held-out evaluation period, and the score they reported was computed over a different window than the
Calib_*metrics written at final evaluation.On the P3 Bow-at-Banff ensemble this surfaced as TOPMODEL+ADAM reporting best 0.7260 against final Calib_KGE 0.7829, while all 16 other algorithms agreed exactly.
What
Adds two methods to
InMemoryModelWorkerso every in-memory worker inherits one implementation instead of each package growing its own copy:get_calibration_slice()— start/end indices of the calibration period within post-warmup arrays, orNonewhen unconfigured or non-overlapping.warmup_steps()— hook returning the warmup length in timesteps; defaults towarmup_days, overridden by models running on a sub-daily timestep (jHBV).No behaviour change on its own — this is the shared primitive the seven companion package PRs consume.
Companion PRs
This PR should merge first — the package PRs call
get_calibration_slice()and willAttributeErroragainst a SYMFLUENCE without it.Tests
441 optimization unit tests pass. The behavioural coverage lives in the package PRs, where the slice is actually applied.
🤖 Generated with Claude Code