Skip to content

[PROBE: energy] Hold meltwater in a pack that still reports cold content - #150

Open
Barbhuiya12 wants to merge 6 commits into
Flood-Lab:mainfrom
Barbhuiya12:probe/snowpack-ripening
Open

Barbhuiya12 wants to merge 6 commits into
Flood-Lab:mainfrom
Barbhuiya12:probe/snowpack-ripening

Conversation

@Barbhuiya12

@Barbhuiya12 Barbhuiya12 commented Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #147

What this probe asserts

A pack below freezing refreezes the water its surface produces, and nothing drains until its cold content is paid off and its pore space has filled. So the water a model reports leaving its snow module, snm, must not flow while the cold content it reports, csnow, is still positive:

R   = sum over melt-block steps that END with csnow_t > 1 J m-2  of  snm_t * dt
eps = R / W_peak  <=  0.05                         W_peak the peak snw

A failure means water left a pack that the model itself says had not ripened. Melt is not forbidden. In a layered pack, surface melt percolates into colder snow and refreezes (Marsh and Woo, 1984), which releases no water and is invisible here. That is why this replaces the ROADMAP's energy/snowpack-cold-content wording ("no melt while below freezing"), which would fail a correct layered model.

The case is energy/snowmelt-energy-water's: generate() is byte-identical. The probe needs no energy fluxes and no forcing entry, so it reaches temperature-index models that probe cannot score. Snow-17 is now scored on an energy probe for the first time.

Discrimination

ht gate --probe energy/snowpack-ripening:

Reference model Expected Criterion that catches it
reference_snow_energy PASS n/a
sacsma_snow17 PASS n/a
reference_degree_day FAIL snowpack_ripening: drains 37–47 % of its pack while cold on the gate seeds
reference_always_ripe (new) FAIL snowpack_ripening: opens the melt block ripe (opens_ripe)

Over seeds 0–199, all 1000 runs land on the declared side:

model share of peak pack drained while cold pack opens at
reference_snow_energy 0.000 % −24.0 to −18.3 °C
sacsma_snow17 0.39 to 0.68 % (all of it DAYGM ground melt) −22.5 to −15.9 °C
reference_warming_free 0.000 % (not a baseline; its fault is in the energy head) −24.0 to −18.3 °C
reference_degree_day 27.2 to 77.6 % −20.6 to −15.3 °C
reference_always_ripe refused as opens_ripe, 200/200 0 °C

What changed from the accepted proposal

The review on #147 and building it moved four things. I'd rather name them than have them found:

  1. The energy-supply guard includes sensible heat, computed against the pack's own temperature. The proposal bounded the fall in cold content by radiation, plus sensible heat only above 0 °C. That fails Snow-17 on 111 of 1459 warming steps (seeds 0–39), because Snow-17 warms from the air, not from radiation. The bound is now [max(0, rn) + K·max(0, tas − T_pack)]·dt, with T_pack = −csnow / (c_ice · ice) and K = 10 W m-2 K-1. That is a bulk ρ c_p C_H U at about 4 m/s. Snow-17 needs at most 4.28, and a model that drops its true cold content to zero in one step needs 5.2 to 73 times the bound. No fall is credited to ice leaving (ice that melts is at 0 °C and carries no cold content), and the pack temperature the bound reads is floored at the coldest air in the record less 2 K. Both close one-row constructions found in review. The bound stops a single step's fall, not a gradual write-down, which is the stated limit below.
  2. The pack floor is 300 mm, derived rather than picked, and it refuses only what ground melt could explain. The 10 mm in the proposal was wrong, as the automated review said. Ground melt is fixed per day, not proportional to the pack. At NWS's upper DAYGM of 0.3 mm/day over the longest cold stretch in the case (47 days), a pack releases 14.1 mm while cold, which is inside 5 % only above 282 mm. The case's smallest peak over 200 seeds is 621.6 mm. Below the floor, a pack that lost more while cold than 0.3 mm/day over its cold days is still failed, so a refusal can cost a pass but never buy one.
  3. requires has no forcing. The criterion reads rn, tas and pr from the case as bounds on what the weather allows. Listing them would mean "consume or be N/A", which would drop Snow-17 (needs_forcing: [pr, tas, pet]), the model the probe exists to reach.
  4. A step counts when the pack is cold at its END, not its start. A step that ends ripe may have paid its deficit and drained within the step, which is what Snow-17 does. Counting it would fail a model for ripening on the day it ripened.

Applicability: the rule is the bulk, matrix-flow picture of ripening. A model that represents preferential flow is outside it, and scoring such a model is a maintainer decision taken on its proposal issue. No model that reports snm and csnow does today; see the probe README's Applicability.

Stated limit, not guarded: this is a consistency check between a model's outflow and its own cold content, not an energy balance. A model that under-reports its cold content escapes. No bound built from the forcing alone can close that: over this winter the pack could absorb 127–276 MJ m-2 of radiant energy against 24–41 MJ m-2 of cold brought in by snowfall. Only a surface energy balance pins it, which energy/snowmelt-energy-water does for the models that report one. This is in the probe README's Scope, together with preferential flow (Wever et al., 2016) and rain-on-snow.

Model changes

  • reference_snow_energy, reference_degree_day, reference_warming_free now emit snm. The three adapters stay one body with MODE lines. mass/snowpack-mass-closure and energy/snowmelt-energy-water still gate as before; the full ht gate passes.
  • reference_always_ripe (new, trusted subprocess): the degree-day body with csnow = 0 throughout.
  • sacsma_snow17 now reports csnow = NEGHS × λ_f under diagnostics, and moves from 1.1.0 to 1.2.0 with every probe re-archived, as docs/writing-a-probe.md asks. It passes 26 of 26, including momentum/froude-regime after the merge of main.

Tests

tests/test_snowpack_ripening.py, 34 tests, including daily vs hourly invariance of the share (the #102 dt lesson), the 1.9 K / 2.1 K opening boundary, and an end-to-end check that each baseline is decided by the rule it exists for. I broke each of the criterion's guards in turn (the dt scaling, the end-of-step mask, the opening-ice and opening-temperature checks, the supply bound and its temperature floor, the absence of any mass-loss credit, both sign checks, the floor and its ground-melt exception, the dry-block check and both parameter checks, and the validation of a reported lwsnl), and every one fails at least one test.

Checklist

  • There is an accepted proposal issue and this PR closes it ([PROBE] energy/snowpack-ripening: meltwater must not leave a pack that still holds cold content #147)
  • authors in probe.yaml names every author with name, affiliation and orcid; CONTRIBUTORS.md has the row, and CITATION.cff already lists me
  • ht validate passes (36 probes, 81 models, after merging main)
  • ht gate --probe energy/snowpack-ripening passes, and so does the full ht gate
  • The generator is deterministic given a seed and commits no data
  • The probe gate runs in about 1.5 s
  • Tolerance, floor and every guard are justified in probe.yaml
  • Constructed cheats: reference_always_ripe (report ripe, empty the sum) is caught and is a baseline; dropping true cold content in one step fails the supply bound by 5.2–73×

Docs moved with it: README (probe row, reference rows, counts), CONTRIBUTORS, ROADMAP (the snowpack-cold-content entry is replaced and marked merged), site/index.html in all three languages with a flowchart entry, docs/writing-a-probe.md, and models/result.csv rows from ht run --gate-seeds --csv for every evaluated model. pytest -q: 1320 passed.

energy/snowpack-ripening: over the snowmelt-energy-water case, water a model
reports leaving its snow module (snm) while the cold content it reports
(csnow) is still positive at the end of the step must stay under 5% of the
peak pack. The pack must open the block cold, its cold content cannot fall
faster than net radiation and sensible heat could supply, and a pack under
300 mm is refused because ground melt drains however cold the pack is.

Needs no energy fluxes and no forcing entry, so it scores Snow-17, which now
reports NEGHS as csnow. The snow references report snm; reference_always_ripe
is the degree-day pack reporting itself ripe throughout.

Closes Flood-Lab#147.
@chrimerss

Copy link
Copy Markdown
Contributor

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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 energy/snowpack-ripening: on the same case energy/snowmelt-energy-water generates (byte-identical generate()), the new snowpack_ripening criterion sums the outflow snm over melt-block steps that end with the model's own csnow > 1 J, and requires it to stay under 5 % of the peak snw. Because it reads rn/tas from the case rather than declaring them under requires.forcing, it reaches temperature-index models — sacsma_snow17 now reports csnow = NEGHS × λ_f and is scored on an energy probe for the first time (25 of 25). Three reference snow packs gain snm, and a fourth, reference_always_ripe, is added as a must-fail control.

The physics and the guard design hold up: the end-of-step mask, the dt scaling, the mass-loss credit in the supply bound (which makes reference_degree_day's proportional deficit scaling net to zero warming) and the 300 mm floor are all justified and covered by tests, and I could not construct a false failure for either must_pass baseline.

Two things to look at. reference_always_ripe is caught only by the opening-ripeness check, and that check is skipped outright when the reported ice snw - lwsnl is ≤ 1e-6 — so the same cheat plus one extra column passes. And sacsma_snow17's adapter gained an output without a version bump or a re-archive, which docs/writing-a-probe.md asks for and which leaves one archived row no longer reproducible.

Posted by the pr-review workflow. Run

Comment thread src/hydroturing/criteria/ripening.py Outdated
# reported within a few kelvin of melting on the opening row is a state
# the forcing did not produce -- the plainest way to empty the sum.
opening_temperature = -c0 / (C_ICE * ice0) if ice0 > 1e-6 else 0.0
if ice0 > 1e-6 and opening_temperature > -min_depression:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium: The opening-ripeness check, the only guard against a pack reported ripe, is skipped when reported ice is zero

opening_temperature is only evaluated and only compared when ice0 = snw[start-1] - lwsnl[start-1] > 1e-6. A model that reports lwsnl == snw on the row before the melt block makes ice0 == 0, and the opens_ripe branch is skipped entirely.

That matters because opens_ripe is the only thing standing between this criterion and a model that reports csnow = 0. With csnow identically zero the mask cold = c_end > c_eps is empty, so leaked = 0 and share = 0; warming = c_open - c_end - carried is zero on every step, so the supply bound never trips either; and the peak-pack floor is satisfied by the real pack. Concretely: take reference_always_ripe exactly as submitted — the degree-day pack that drains 37–47 % of its pack while genuinely cold — and add lwsnl = snw to its rows. It now returns PASS with a 0.00 % share instead of FAIL (opens_ripe), and the probe's must_fail baseline no longer demonstrates anything. Nothing else constrains lwsnl: this probe does not require it, and a temperature-index model (the audience the probe exists to reach) is N/A on energy/snowmelt-energy-water, so that probe's max_opening_liquid_mm guard never sees it.

The sibling criterion on the same case does guard this (max_opening_liquid_mm, default 1 mm, in coherence.melt_energy). Suggest either refusing/failing when ice0 <= 1e-6 while snw on the opening row is above min_peak_pack_mm — an all-liquid pack after 150 days below −12 °C is not a state this forcing produces — or adding an equivalent max_opening_liquid_mm bound on the opening row. A test alongside test_a_pack_reported_ripe_on_the_opening_row_fails that sets lwsnl = snw would pin it.

fluxes: [pr, snm, evspsbl, mrro, dis, gwex]
states: [mrso, snw, canopy, gw, channel]
diagnostics: [stage]
diagnostics: [stage, csnow]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium: sacsma_snow17's adapter gained an output without a version bump or re-archive, and one archived row no longer reproduces

docs/writing-a-probe.md:318 says that when a physical model's adapter is extended so a new probe can score it, you "bump the model's version and re-archive its rows". This PR extends sacsma_snow17 (new csnow output, diagnostics: [stage, csnow]) and keeps version: "1.1.0", archiving only the new probe's row.

The PR description argues that "every archived row reproduces" because this is a new column only, but that is not true of the N/A reasons, which report.py:223 builds from probe.missing — i.e. from the model's emits. models/result.csv:665 records sacsma_snow17,1.1.0,...,energy/snowmelt-energy-water,N/A,INCOMPLETE,...,"does not report sbl, hfls, hfss, hfg, lwsnl, csnow; model does not declare that it consumes forcing rn". Re-running ht run --model sacsma_snow17 --probe energy/snowmelt-energy-water at the same declared version 1.1.0 now emits csnow, so the detail becomes does not report sbl, hfls, hfss, hfg, lwsnl; .... The verdict is unchanged (still N/A), so no test catches it, but the archive now attributes two different outputs to one version string.

Suggest bumping to 1.2.0 and re-archiving sacsma_snow17's rows with ht run --gate-seeds --csv, or — if the maintainers prefer the no-bump path — at minimum re-archiving the energy/snowmelt-energy-water row so the stale reason is not the newest one, and dropping the "every archived row reproduces" claim from the PR body. Note that _archived_standings in tests/test_docs_in_sync.py takes each model's version from its last row and requires that version to cover every probe, so a bump means re-archiving all 34 rows, not just this one.

Comment thread docs/writing-a-probe.md Outdated
| `radiative_identity` | upward longwave equals what the reported surface temperature emits plus the reflected downward longwave, at every step, within the larger of a relative tolerance and an absolute floor; emissivity comes from `static.json` | one run, instantaneous values |
| `soil_heat_storage` | interval boundary heat input agrees with fixed-layer temperature change and prescribed heat capacity | one run, separate heating/recovery phases |
| `melt_energy` | over a labelled melt block, the surface energy residual equals the fusion the reported ice change demanded plus what warming the pack cost; the ice is `snw - lwsnl` and the warming is `-d(csnow)`, both self-reported, so the two rows it differences carry seven contract checks, one of them read on every step | one run, labelled blocks |
| `snowpack_ripening` | over a labelled dry melt block, the outflow `snm` that leaves while the model's own cold content `csnow` is still positive at the step's end, as a share of the peak pack; the pack must open the block colder than `min_opening_depression_k`, its cold content cannot fall faster than net radiation plus sensible heat from air warmer than the pack could supply, and a pack under `min_peak_pack_mm` is refused (N/A) because ground melt drains however cold the pack is. A consistency check on self-reported cold content, not an energy balance: it cannot catch a model that under-reports its cold content |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low: The new criteria-table row is missing its third column

Every other row of the criteria table in docs/writing-a-probe.md has three cells — name, what it asserts, and the shape of the evidence (melt_energy on line 123 ends | one run, labelled blocks |). The snowpack_ripening row ends at ...under-reports its cold content |, so it has only two cells and renders with an empty third column in the table.

Suggest appending the shape cell, e.g. | one run, labelled blocks |, matching the sibling row.

- snowpack_ripening refuses a pack whose ice on the row before the block is
  under half its peak (min_opening_ice_share: 0.5). Declared liquid in lwsnl,
  or snw dipping on that row, would skip the opening check at zero ice or let a
  token cold content read as a cold pack. Honest packs open with 99.99-100% of
  their peak as ice (seeds 0-19). Two tests pin it.
- sacsma_snow17 1.1.0 -> 1.2.0 for the new csnow output, re-archived on every
  probe; the 1.1.0 row this branch had added is dropped. PASS, 25 of 25.
- docs/writing-a-probe.md: the criteria row gains its third column.
@Barbhuiya12

Copy link
Copy Markdown
Collaborator Author

All three were right. Fixed in 2c9fee1.

1. The opening check, skipped at zero ice. Confirmed: reference_always_ripe with lwsnl = snw added passed. I closed it one step wider than the missing liquid cap, because a liquid cap alone leaves the same door open through snw. Dip snw to 1 mm on the row before the block, give it csnow = 2100 × 1 × 20 so it reads −20 °C, and the pack opens "cold" on a token cold content. The criterion now refuses a pack whose ice (snw − lwsnl) on that row is under half its peak (min_opening_ice_share: 0.5, outcome opens_without_ice). Nothing melts before the block, and the four snow models open with 99.99 to 100 % of their peak as ice and 0 mm of liquid over seeds 0–19, so the bound sits a factor of two from any honest pack. Both constructions now fail, and each has a test that fails without the guard. Your scenario on the gate seeds now reads opens_without_ice: 0.0 mm of ice against a 947 mm peak. With at least 150 mm of ice guaranteed, the ice0 > 1e-6 skip is gone.

2. Snow-17's version. You're right that the N/A reasons are built from emits, so "every archived row reproduces" was wrong for the snowmelt-energy-water row. sacsma_snow17 is now 1.2.0, re-archived on all 34 probes with ht run --gate-seeds --csv: PASS, 25 of 25, 9 N/A. I dropped the 1.1.0 row this branch had added, since 1.1.0 never emitted csnow, and corrected the PR description.

3. The criteria row has its third column: one run, labelled blocks.

ht validate (34 probes, 77 models), the full ht gate and pytest -q (1247 passed) all pass at 2c9fee1.

Conflicts in the three tail files every probe PR touches. README counts are
re-derived for both probes: thirty-five, twenty-two mass, eight energy, five
momentum, three still wanted. The flex models take main's standings, and
sacsma_snow17 1.2.0 is archived on momentum/froude-regime (PASS), so it now
stands at 26 of 26. models/result.csv is the union of both sides' rows.

@chrimerss chrimerss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, @Barbhuiya12. I checked 852facd locally. pytest (1293), ht validate (35 probes, 78 models), the probe gate and the full gate all pass, and CI is green. All 35 sacsma_snow17 1.2.0 rows reproduce byte for byte with ht run --gate-seeds, the nine N/A reasons match missing_for, and generate() is identical to the one in energy/snowmelt-energy-water. All three findings from the first review are fixed.

There is one blocker. It is the hole 2c9fee1 closed on the opening row, moved one row later.

1. The mass-loss credit lets the degree-day pack pass with a single edited row (ripening.py:246). carried = c_open * clip((ice_open - ice_end) / ice_open, 0, 1) credits any fall in reported ice. I took reference_degree_day's output on the real generator and changed one thing: on the first row of the melt block I set lwsnl = snw, and from that row on I set csnow = 0. That row credits the whole cold content as carried, so warming is 0 and no step after it counts. It passes on 200 of 200 seeds. A one-row snw dip does the same, with or without an lwsnl column (200/200), and so does lwsnl = snw held through the whole block (200/200). Ice that melts is at 0 °C and takes no cold content with it. Only ice that leaves as ice can, which in practice means sublimation.

Suggested fix: drop the credit. Over seeds 0–199 at PT1D and 0–19 at PT1H, all four baselines land exactly where they do now (degree_day 27.2–77.6 % on the share, Snow-17 0.39–0.68 %, snow_energy 0 %, always_ripe opens_ripe). The three constructions then fail as ripens_too_fast on 200/200. If you want to keep a credit, limit it to reported sbl and charge its latent heat against the same supply. test_ice_that_leaves_takes_its_cold_content_without_counting_as_warming pins the credit with 250 mm sublimating in one day, which is about 700 MJ m-2. Please replace it with regression tests for the lwsnl and snw constructions above.

2. The 300 mm floor turns clear failures into N/A (ripening.py:169). The floor exists so that ground melt is not scored as a leak. It is derived from 0.3 mm/day × 47 cold days = 14.1 mm. Yet it refuses every pack under 300 mm, whatever drained from it. I scaled reference_degree_day to 0.3× (pack, liquid, cold content and outflow, so the same share at the same temperature). It still drains 27–78 % of its pack while cold, 1.4–2.5 mm/day averaged over the block, which is 5–8× the ceiling. It is N/A on 46 of 50 seeds. Suggested fix: below the floor, refuse only when the water that left while cold fits inside 0.3 mm/day × cold steps × dt. Let a failure beyond that stand, as uniform_flow keeps a failure measured on a steady plateau however many others were skipped.

3. The supply bound stops only the one-step drop. Please say so where the bound is described (README:102, probe.yaml:116, reference_always_ripe/model.yaml:9). Suppose a degree-day pack's csnow falls at the bound's own rate from the opening row. It reaches zero on block day 11–23, and the pack first drains on day 37–43. So it passes 200/200. Scope item 1 already covers under-reporting, but "needs 5.2 to 73 times the bound" and always_ripe's "does not rest on a model's own report of its cold content alone" read as if the bound protects more than one row. One sentence in Scope with the slack in this case (about three weeks) would make that accurate.

4. Wording.

  • README:124 says water leaves "on all 60 steps of the block". It leaves on 17–23 of them, the last three weeks.
  • probe.yaml:97 gives the honest opening temperatures as −16.3 to −24.0 °C. Snow-17 opens as warm as −15.9 °C, which is what the README says.

Still to come: two approvals from the energy pool.

…only excuses ground melt

- The supply bound no longer credits a fall in reported ice. Ice that melts is
  at 0 C and carries no cold content, and the credit let one row declared
  liquid, or one dip in snw, write the whole deficit off (200/200 seeds).
- The pack temperature the bound reads is floored at the coldest air in the
  record less 2 K (pack_temperature_margin_k). Without the credit, a row with
  the pack nearly all liquid would otherwise read as impossibly cold and buy
  unlimited sensible heat. Honest packs stay above that air.
- Below min_peak_pack_mm a pack is refused only while what left it while cold
  fits inside 0.3 mm/day of ground melt over its cold days
  (max_ground_melt_mm_per_day); a larger leak is measured and stands.
- README, probe.yaml and reference_always_ripe now say the bound stops a single
  step's fall, not a gradual write-down (two to four weeks of slack here), and
  the degree-day pack drains on 17-23 of the 60 block steps.

Over seeds 0-199 every baseline is unchanged; the three review constructions
and the two-row one fail as ripens_too_fast on 200/200; the 0.3x degree-day
pack fails on 200/200. Snow-17's archived row reproduces byte for byte.
@Barbhuiya12

Copy link
Copy Markdown
Collaborator Author

Thank you, all four were right. Fixed in 461a95e.

1. The credit is gone. You're right that ice which melts is at 0 °C and takes no cold content with it, so a credit for a fall in reported ice only ever served the construction. Removing it opened the same door one step later, so I closed that too. The bound reads the pack's temperature from csnow and the ice, so a row with the pack declared all but liquid (lwsnl = snw − 1e-7, csnow kept) reads millions of kelvin below freezing, and the sensible heat on the next step covers any fall. The temperature is now floored at the coldest air in the record less 2 K (pack_temperature_margin_k). Over seeds 0–19 the honest packs never go below that air: reference_snow_energy comes within 0.08 K of it and Snow-17 within 1.06 K. The sublimation test is replaced by regression tests for your three constructions and for this two-row one. Each fails when its fix is reverted.

2. The floor now excuses only ground melt. Below 300 mm, a pack is refused only while what left it while cold fits within max_ground_melt_mm_per_day (0.3) over its cold days. Beyond that the failure is measured and stands, as you suggested. Your 0.3× degree-day pack now fails on 200/200 seeds, at the same 27.2–77.6 % share.

3. The bound's reach is stated. README, probe.yaml and reference_always_ripe's description now say the bound stops a single step's fall and nothing more. Scope item 1 gives the slack, measured on reference_degree_day with csnow written down at 99 % of the bound's own rate: zero on block day 12–24, first drainage on day 38–44, so two to four weeks here. That construction passes 200/200, as documented.

4. Wording. The degree-day pack drains on 17–23 of the 60 block steps (seeds 0–199), and probe.yaml now gives −15.9 to −24.0 °C.

Over seeds 0–199 at 461a95e:

run verdict
reference_snow_energy, sacsma_snow17, reference_warming_free PASS 200/200 (0 %, 0.39–0.68 %, 0 %)
reference_degree_day FAIL 200/200, 27.2–77.6 %
reference_always_ripe FAIL 200/200, opens_ripe
degree-day + lwsnl = snw on the first block row, csnow = 0 after FAIL 200/200, ripens_too_fast
degree-day + one-row snw dip, csnow = 0 after FAIL 200/200, ripens_too_fast
degree-day + lwsnl = snw through the block, csnow = 0 FAIL 200/200, ripens_too_fast
degree-day + near-zero ice on one row, csnow = 0 from the next FAIL 200/200, ripens_too_fast
degree-day scaled 0.3× FAIL 200/200 (was N/A)
degree-day with csnow written down at the bound's rate PASS 200/200, the stated limit

ht validate (35 probes, 78 models), the full ht gate and pytest -q (1297) pass, and Snow-17's archived row on this probe reproduces byte for byte.

…nowpack-ripening

Conflicts in README.md, models/result.csv and site/index.html. Counts are
re-derived for both probes: thirty-six, twenty-three mass, eight energy, five
momentum. The flowchart keeps main's taller pillars with the ripening entry
in the energy one. sacsma_snow17 1.2.0 is archived on
mass/groundwater-datum-invariance (N/A) after main's 1.1.0 row, and
modflow6 6.7.0-adapter.2 on this probe (N/A); the site chart data is
regenerated with scripts/site_standings.py.

@licm13 licm13 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.

Thank you, @Barbhuiya12 , for this contribution and for working through the earlier reviews. I have been catching up with the wider HydroTuring suite, and this probe fills a useful gap: it links snow-module outflow to reported cold content without requiring the surface energy fluxes that exclude temperature-index models from snowmelt-energy-water. The distinction between melt production and water leaving the pack is also a welcome correction to the original roadmap wording.

I reviewed the current head, 6e9c5f2, together with the scoring contract, the related snow probes, and the earlier discussion. Locally, the 316 tests covering this probe, documentation consistency and report details passed, as did ht validate, the probe gate and the full acceptance gate. The previous fixes are present. I have one reproducible implementation issue and a few suggestions about how this probe should be interpreted within the wider benchmark.

1. Please validate the optional lwsnl column before using it in the supply guard.

The finite-value check in ripening.py covers outflow, pack water and cold content, but not lw. Since lwsnl is optional, the protocol reader does not validate it as one of this probe’s required variables either. A missing value can therefore propagate into ice_open, t_pack, supply and excess. np.argmax(excess) selects the NaN entry, and the comparison against c_eps is false, so the warming guard is skipped.

I reproduced this on all five gate seeds using the real generator and reference_degree_day output, including a CSV round trip through read_result: set csnow to zero from the first melt-block row onward, and leave only that first row’s lwsnl blank. The original model fails; the cold-content write-off alone fails as ripens_too_fast; adding that one blank cell changes the result to PASS, with a NaN in worst_warming_share_of_supply.

Could you reject non-finite values whenever lwsnl is supplied and add this construction as a regression test? Checking 0 <= lwsnl <= snw, with a small numerical tolerance, would also help. Keeping zero as the fallback when the column is genuinely absent is a separate, documented choice. I would address this before merging, since malformed output should not buy a pass.

2. Please connect the preferential-flow limitation to an explicit applicability decision.

The Scope section already acknowledges that preferential flow can produce drainage while bulk cold content remains. However, the opening statement reads as a general snowpack rule, and eligibility currently depends on the reported variables rather than the snow-transport formulation. A physically valid preferential-flow model exceeding the 5% allowance would therefore be recorded as FAIL/VIOLATION, affecting its overall HydroTuring verdict.

I suggest describing the rule upfront as a bulk/matrix-flow ripening consistency check and explaining how models outside that assumption will be handled. An explicit compatibility condition or a documented maintainer decision before scoring such models would be preferable to treating 5% as an established bound for preferential flow. This does not require adding a new SNOWPACK implementation here; it requires distinguishing an unsupported process assumption from a conservation violation.

3. It would be useful to preserve the stated limits as reproducible examples.

The gradual cold-content write-down is now clearly documented, and I agree it should not be presented as something the supply guard prevents. A small regression example showing that this construction still passes would keep that limitation visible as the criterion evolves.

Similarly, the timestep test correctly places the leak away from the ripening transition. It establishes correct rate integration, but not verdict invariance around the transition itself. A paired daily/hourly example near ripening would help readers assess the resolution-dependent blind interval. I see these as useful follow-ups, rather than a request to turn this probe into a full energy-budget test.

4. A small reference correction.

The Wever et al. reference in the new README has the wrong title for The Cryosphere, 10, 2731–2744. The matching paper is Simulating ice layer formation under the presence of preferential flow in layered snowpacks, https://doi.org/10.5194/tc-10-2731-2016. Please correct the entry and include the DOI, since it supports the applicability caveat above.

I support adding this complementary check to the suite. My main requests before approval are to close the optional-column NaN bypass and clarify model applicability; the additional examples would strengthen the documented limits without broadening the intended scope.

…n the limits

- A reported lwsnl is now checked: finite on every row and within [0, snw] to
  liquid_tolerance_mm. It is optional, so nothing upstream validated it, and
  one blank cell made the supply bound select a NaN in argmax and skip itself,
  turning a csnow write-off that fails as ripens_too_fast into a pass. The
  bound's own result is also checked for NaN, so a bad forcing value cannot do
  the same. Absent still means no liquid.
- README and probe.yaml describe the rule as the bulk, matrix-flow picture of
  ripening and add an Applicability section: a model that represents
  preferential flow is outside it, and scoring such a model is a maintainer
  decision taken on its proposal issue.
- Two stated limits are kept as examples: a cold content written down inside
  the energy supply still passes, and a daily and an hourly record differ by at
  most one step's outflow at the ripening step (0% daily, 5.8% hourly for 30
  mm/day on that day).
- The Wever et al. (2016) entry is corrected to its actual title, with the DOI.

Over seeds 0-99 every baseline lands where it did.
@Barbhuiya12

Copy link
Copy Markdown
Collaborator Author

Thank you, @licm13. All four points were right, and the first was a real bypass. Fixed in 8105186.

1. The blank lwsnl cell. Reproduced on the real generator's synthetic equivalent: csnow written to zero from the first block row fails as ripens_too_fast, and leaving one lwsnl cell blank on that row turned it into a PASS with a NaN in worst_warming_share_of_supply. The cause is as you describe, np.argmax selects the NaN and NaN > c_eps is false. A reported lwsnl now has to be finite on every row and between 0 and snw within liquid_tolerance_mm (10⁻⁶ mm), with outcomes non_finite and invalid_liquid. Absent still means no liquid, as before. I also check the energy bound's own result for NaN, so a bad value in the case's forcing can't skip it by the same route. The tests cover a NaN and an inf on the opening row and in the interior, a negative liquid, one above the pack, a NaN in rn and in tas, and a valid column that changes nothing. Removing either check fails its tests. Over seeds 0–99 no honest model trips the range check.

2. Applicability. The opening and probe.yaml now describe the rule as the bulk, matrix-flow picture of ripening, and there is an Applicability section. A model that represents preferential flow is outside that assumption, and a FAIL there would be a difference in the process assumed, not a conservation violation. No model that reports snm and csnow represents it today, so nothing in the suite is affected. If one is submitted, whether it is scored here is a maintainer decision to take on its proposal issue before its rows are archived. I deliberately did not add a declaration a model could set for itself. A flag the model sets is a way out of the check, and the decision should be what sets it. If you'd rather have a manifest key for it, I'm happy to add one once a model needs it.

3. The limits as examples. test_a_cold_content_written_down_inside_the_energy_supply_still_passes keeps the gradual write-down visible. The synthetic pack pays off its deficit in four days at 3.9 MJ/m²/day against 5.2 or more of supply, drains from then on, and passes, where an honest pack with that deficit needs about twelve days. For the transition, test_the_step_that_ripens_the_pack_is_excused_whole pairs a daily and an hourly record of 30 mm/day on the ripening day: 0 % of the peak pack daily and 5.8 % hourly, because the daily step is excused whole while the hourly one excuses only its last hour. The blind interval is one step's outflow, and the README says so. Away from the transition the two agree, as in the existing test.

4. The reference. You're right. I checked it against the DOI registry: Wever, Würzer, Fierz and Lehning (2016), Simulating ice layer formation under the presence of preferential flow in layered snowpacks, The Cryosphere 10, 2731–2744, https://doi.org/10.5194/tc-10-2731-2016. The entry now carries it.

ht validate (36 probes, 81 models), the full ht gate and pytest -q (1320) pass, and over seeds 0–99 every baseline lands as before: reference_snow_energy and reference_warming_free at 0 %, Snow-17 at 0.39–0.68 %, reference_degree_day at 27.2–68.0 %, reference_always_ripe refused as opens_ripe.

@licm13

licm13 commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Thank you, @Barbhuiya12 , for the careful revision. I checked 8105186 and reran the original CSV-based construction on all five gate seeds: the blank lwsnl cell now correctly fails with non_finite. The 327 relevant tests, specification validation and probe gate also passed locally.

The applicability wording and the two reproducible limitation examples address my other concerns. I am comfortable with the documented maintainer decision before archiving any future preferential-flow model; there is no need to add a model-controlled exemption flag at this stage.

This addresses the points I raised in this review. Thank you also for correcting the reference.

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] energy/snowpack-ripening: meltwater must not leave a pack that still holds cold content

3 participants