Expand snm coverage for packaged snow models - #153
jinlong-hu wants to merge 11 commits into
Conversation
|
/review |
There was a problem hiding this comment.
Automated review by Claude (changes-suggested), not a maintainer approval.
The PR maps snm (snow-module liquid outflow) for five packaged models so mass/snowpack-mass-closure can score them, bumps each model version, and archives a full 33-probe gate-seed row set per model, updating the top-level README, each model README and the site's three language blocks.
The mappings themselves look sound. CWatM (Rain + SnowMelt + IceMelt), LISFLOOD (Rain + SnowMelt) and Wflow SBM (native snow.runoff) are read from native terms and close the pack budget algebraically; the Julia column re-index at WflowSbmAdapter.jl:714-722 and the LISFLOOD record unpacking are both consistent. The archived standings (12/22, 19/22, 9/25, 18/22, 20/22) match models/result.csv exactly, and every new version has a row for all 33 probes, so tests/test_docs_in_sync.py should pass.
The thing to decide before merging is SUMMA. Its new snm row is a FAIL at 18.85% of precipitation, and the README explains it as a contract limitation: sbl cannot be reported because HydroTuring's sbl must be non-negative. Nothing in AGENTS.md, spec.py or the criteria says that — coherence.py explicitly nets out deposition reported in sbl. So this publishes a snowpack conservation violation against SUMMA that is most likely an adapter omission, not the model.
Two secondary items: the LISFLOOD README now contradicts itself about the two ten-year probes (ERROR vs completed), and δHBV2's reconstructed RAIN term is not exercised by any archived case.
models/lisflood/README.md:389 medium: LISFLOOD README still documents ERROR rows the archive no longer has
The rewritten Result section (line 398) says "FAIL (VIOLATION), 20 of 22 probes passed" and models/result.csv:762-794 shows mass/precipitation-counterfactual PASS and mass/human-abstraction VIOLATION, both on 3650-day windows. But lines 389-394 still read "A ten-year daily record takes 97 s there. That is over the 60 s budget of mass/precipitation-counterfactual and mass/human-abstraction … so the archived ERROR rows record no wall time of their own", and lines 434 and 450 still say those probes are ERROR. The "Native re-run" section that explained how the provisional ERROR rows would be replaced was deleted in this PR, and model.yaml's closing comment (which also carried the "Default flood-event window" note) went with it.
Nothing in the adapter changed its speed and both probes still carry max_runtime_s: 60 (probes/mass/precipitation-counterfactual/probe.yaml:35, probes/mass/human-abstraction/probe.yaml:35), so the verdict moved from FAIL (ERROR) to FAIL (VIOLATION) purely because a different host ran it — and the README no longer records which host produced the archived rows.
Failure scenario: a reader checking why LISFLOOD's verdict changed finds the README asserting the two probes cannot finish inside their budget on the host, and no statement of the host these rows came from; the next person to re-run gets different rows and cannot tell whether that is a regression.
Suggested fix: rewrite lines 378-394 for the host that produced the .6 rows (or say plainly that the archive is now native x86-64 and the emulated timings are historical), and restore the window sentence to model.yaml.
Posted by the pr-review workflow. Run
|
Thanks, Jinlong — exposing the native snow-module outflow makes the existing snow-balance probe usable for more models. δHBV2’s mapping matched the native model in my cold-snow and rain-on-snow runs. One issue to fix before merging: the new Could the check account for native state precision while retaining the rain-reconstruction check? I ran this on Linux with the pinned model dependencies, using Python 3.10 outside Docker, and have included the reproduction input below. That would keep this added coverage usable for deeper snowpacks too. Reproduction input and environmentNative CPU inference on Linux: Python 3.10.19, torch 2.5.1+cpu, numpy 1.26.4, pandas 2.3.3, dmg 1.4.3, hydrodl2 1.3.5, dhbv2 0.5.4; The input generator below is extracted from the diagnostic I ran. Its regenerated forcing and static parameters matched the recorded experiment exactly. It produces two synthetic seasons, with 180 cold days, 120 dry melt days and 65 warm days per season. Total precipitation is 2,973.091151 mm, with a maximum daily value of 81.878663 mm. No explicit elevation is supplied, so the adapter uses its training-mean elevation. Save as import json
from pathlib import Path
import numpy as np
import pandas as pd
STATIC = {
"area_km2": 250.0,
"soil_capacity_mm": 320.0,
"canopy_capacity_mm": 0.0,
"degree_day_factor_mm_per_C_day": 3.2,
"baseflow_coefficient": 0.006,
"snow_threshold_degC": 0.0,
"latitude_deg": 44.0,
}
def frame(time, pr, tas):
doy = pd.to_datetime(pd.Series(time)).dt.dayofyear.to_numpy()
daylength = 1.0 + 0.35 * np.cos(2 * np.pi * (doy - 172) / 365)
pet = np.maximum(0.0, 0.13 * (tas + 5.0)) * daylength # the probe generator's PET form
return pd.DataFrame({"time": list(time), "pr": np.round(pr, 6), "tas": np.round(tas, 6), "pet": np.round(pet, 6)})
def deep_snow_case(scale, years=2, seed=20261002):
"""Single deep winter per year: 180 cold days (all snow), 120 dry melt days, 65 warm days.
Same draws at every scale, so only the amounts (and the SWE magnitude) change."""
rng = np.random.default_rng(seed)
n = 365 * years
time = pd.date_range("1999-10-01", periods=n, freq="D").strftime("%Y-%m-%d")
day = np.arange(n) % 365
cold, melt = day < 180, (day >= 180) & (day < 300)
noise, innov = np.zeros(n), rng.normal(0.0, 0.8, n)
for t in range(1, n):
noise[t] = 0.72 * noise[t - 1] + innov[t]
tas = np.where(cold, np.minimum(-6 + noise, -3.0), np.where(melt, np.maximum(8 + noise, 4.0), 10 + noise))
snow_draw, wet_cold = rng.gamma(0.8, 1.0, n), rng.random(n) < 0.5
rain_draw, wet_late = rng.gamma(0.7, 8.0, n), rng.random(n) < 0.3
pr = np.where(cold & wet_cold, scale * snow_draw, 0.0)
pr = np.where(~cold & ~melt & wet_late, rain_draw, pr)
return frame(time, pr, tas), dict(STATIC)
case = Path(__file__).resolve().parent / "publication_input"
(case / "input").mkdir(parents=True, exist_ok=True)
(case / "output").mkdir(exist_ok=True)
f, static = deep_snow_case(scale=18.0, years=2, seed=20261002)
f.to_csv(case / "input/forcing.csv", index=False)
(case / "input/static.json").write_text(json.dumps(static, indent=2))
request = {
"case_id": "deep-snow-native-control", "seed": 20261002,
"timestep": "PT1D", "n_steps": len(f),
"input": {"forcing": "input/forcing.csv", "static": "input/static.json"},
"output": {"table": "output/result.csv", "run": "output/run.json"},
}
(case / "request.json").write_text(json.dumps(request, indent=2))
print(case / "request.json")The paired model runs used the base adapter, version .3 and the PR adapter, version .4, with the same model directory and input. To repeat the comparison, save these as export DHBV_THREADS=2 OMP_NUM_THREADS=2
cp -R publication_input baseline_case
cp -R publication_input pr_case
python ht_adapter_base.py --request baseline_case/request.json
python ht_adapter_pr.py --request pr_case/request.jsonObserved: base exit 0 and 730 output rows; PR exit 1 before writing the output table or run metadata. The error ends with: At the worst step, float32 spacing is 0.00012207 mm; the full-record signed snow-budget residual is only -1.65×10⁻⁸ of precipitation. The base snow-storage series matches the native states observed in the PR diagnostic exactly. |
21a8308 to
faab065
Compare
|
@cehw Thanks for the detailed reproduction. I reproduced the deep-snow case and updated the δHBV2 cross-check to account for the native float32 precision while retaining the rain-reconstruction validation. The check now keeps the existing On your 730-day reproduction case, the updated adapter completes successfully with:
The full output contains all 730 rows. I also re-ran:
The branch has also been rebased onto the current |
faab065 to
29e53da
Compare
|
Thanks for the fix, Jinlong — the precision-aware tolerance keeps this check useful for deeper snowpacks. I re-ran This resolves my δHBV2 finding. My checks cover that adapter’s native inference outside Docker. |
Closes #137
Summary
This PR expands
snmcoverage for packaged models with an explicit snow module, so thatmass/snowpack-mass-closurecan be evaluated where a model-native snow-module outflow is available.The mappings added are:
snmmappingRain + SnowMelt + IceMeltRAIN + tosoilRain + SnowMeltsnow.runoffscalarRainPlusMeltThe other relevant packaged models were also reviewed.
flex_lumpedandflex_topodo not expose a separate snow-module control volume with a distinct liquid outflow matching thesnmcontract.google_flood_forecasthas no snow module, whilemodflow6represents groundwater and surface-water exchange without snow processes, so they remain unchanged.Implementation
For each supported model, this PR:
snm;model.yamlto declare the new flux;snmfrom a residual;For δHBV2,
snmisRAIN + tosoil.tosoilis read directly from the model, whileRAINis reconstructed using the same temperature-threshold rule as the pinned hydrodl2 version. Each adapter run cross-checks that reconstruction against the model's exposed snow stores usingΔ(SNOWPACK + MELTWATER) = pr - snm. The check retains a1e-4mm absolute tolerance floor and expands it, when necessary, to four float32 ULPs at the native snow-state and flux scale, accounting for δHBV2's native numerical precision at deep snowpacks while retaining the reconstruction check. On the collaborator-provided 730-day deep-snow reproduction, the maximum step residual is0.000195183mm against a precision-aware tolerance of0.00048828125mm, and the adapter writes all 730 output rows.For Wflow SBM,
snmis read directly from the nativesnow.runoffvariable. Its snowpack budget closes to machine precision. In the snowpack-closure probecanopy_capacity_mm = 0, mapped toCmax = 0, so precipitation reaches the snow module without upstream interception.For SUMMA,
snmis mapped toscalarRainPlusMelt. With explicit snow layers this is the basal liquid flux leaving the snowpack; without an explicit snow layer it also carries rain and melt entering the soil. The adapter also reports signedsbl = -scalarSnowSublimation, preserving SUMMA's native sublimation/frost exchange: positive values remove snow by sublimation and negative values represent frost deposition.Results
After rebasing onto the current
main, I reran the adapter checks and the full gate-seed suite.The δHBV2 spin-up failure is the existing
mrsostate-bound issue: the reported soil moisture exceeds the declared capacity under all three tested spin-up lengths.SUMMA now passes
mass/snowpack-mass-closure, with a cumulative residual of 0.0000% of precipitation. The closure uses independent native quantities:scalarRainPlusMeltassnm, signed-scalarSnowSublimationassbl, andscalarSWEassnw; it is not reconstructed from a residual.Reporting signed
sblexposes a semantic difference between its use in the snowpack balance and in the current latent-heat split criterion. The snowpack balance accepts negativesblas net frost deposition, while the latent-heat criterion treats the reported sublimating share as non-negative and therefore flags SUMMA's negative frost-deposition steps. The affected energy probes remain FAIL, while the snowpack mass balance closes to machine precision.Validation
All five modified adapters pass
ht verify-adapter.The repository-level checks also pass:
The archived rows for these model versions were regenerated against the current 35-probe suite, including
mass/groundwater-datum-invariance,mass/spinup-cycle-invariance, andmomentum/froude-regime.