D24 composability: admit build_waveform_digest via a shared digest-family registry (issue #508) #510
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -347,6 +347,160 @@ def _pooled_build(meta, n=8): | |
| return resolve_function(meta["function"])(cell_data["h_ph"], **params) | ||
|
|
||
|
|
||
| _WAVEFORM_FUNCTION = "zagg.stats.waveform.build_waveform_digest" | ||
|
|
||
|
|
||
| def _waveform_variables(temporal=True, delta=64): | ||
| """A ``rx_flux``-shaped field: the gedi01b template's reducer + channel. | ||
|
|
||
| Sources ``h_ph`` and reads the noise-model columns via the params-as-column | ||
| path, exactly as the shipped template wires ``rxwaveform``/``noise_mean``/ | ||
| ``noise_stddev`` (issue #508 phase 1 harness). | ||
| """ | ||
| field = { | ||
| "kind": "ragged", | ||
| "function": _WAVEFORM_FUNCTION, | ||
| "source": "h_ph", | ||
| "inner_shape": [2], | ||
| "dtype": "float32", | ||
| "fill_value": 0, | ||
| "params": { | ||
| "delta": delta, | ||
| "counts": "wf_counts", | ||
| "noise_mean": "wf_noise_mean", | ||
| "noise_stddev": "wf_noise_stddev", | ||
| "gain": 1.0, | ||
| "false_positive_rate": 1.0e-3, | ||
| "samples_per_record": 60, | ||
| }, | ||
| } | ||
| if temporal: | ||
| field["temporal"] = "per-centroid" | ||
| return { | ||
| "count": {"function": "len", "source": "h_ph", "dtype": "int32", "fill_value": 0}, | ||
| "rx_flux": field, | ||
| } | ||
|
|
||
|
|
||
| def _with_waveform_columns(dfs, seed=0): | ||
| """Add the noise-model columns ``_waveform_variables`` declares. | ||
|
|
||
| Counts sit well above the zero-mean noise floor, so every row survives the | ||
| clip and ``sum(weights)`` over a cell is the sum of its rows' counts. | ||
| """ | ||
| rng = np.random.default_rng(seed) | ||
| for df in dfs: | ||
| n = len(df) | ||
| df["wf_counts"] = rng.uniform(5.0, 50.0, n) | ||
| df["wf_noise_mean"] = np.zeros(n, dtype=np.float64) | ||
| df["wf_noise_stddev"] = np.full(n, 0.1, dtype=np.float64) | ||
| return dfs | ||
|
|
||
|
|
||
| class TestWaveformSpillBaseline: | ||
| """How the spill path treats ``build_waveform_digest`` TODAY (issue #508). | ||
|
|
||
| Phase 1 characterization, recorded before any registry changes: the SERC | ||
| GEDI fleet runs (0.47–0.49) completed under ``{mode: spill}``, and the | ||
| mechanism that carried them is the NON-mergeable single-block regime. | ||
| ``validate_spill_fold`` refuses the builder (it is outside | ||
|
Member
Author
There was a problem hiding this comment. 🤖 from Claude (review) The recorded spill baseline is a 2-field synthetic, and the docstring's causal claim does not hold for the config the SERC runs actually used. The docstring attributes the non-mergeable verdict to the builder alone:
For the shipped The template's own comment says the same thing ( Why it matters for a characterization phase: Suggested fix — one extra pin on the real config, plus a docstring correction: def test_the_shipped_template_is_refused_by_more_than_the_builder(self):
# The deployed config's refusal is OVER-DETERMINED: even if the builder
# were admitted, the seven per-shot companions keep it non-mergeable, so
# issue #508 cannot move the GEDI store off single-block spill.
from zagg.config import default_config
with pytest.raises(ValueError) as exc:
validate_spill_fold(default_config("gedi01b_waveform_healpix_hive"))
assert "'rx_flux'" in str(exc.value)
assert "'shot_count'" in str(exc.value) and "single_shot_value" in str(exc.value)and reword the docstring's second sentence to "
Member
Author
There was a problem hiding this comment. 🤖 from Claude Folded in 0de5dbf. New pin — Docstring corrected — it no longer attributes the shipped templates verdict to the builder alone; it now reads that
|
||
| ``_TDIGEST_SPILL_FUNCTIONS``) — and on the SHIPPED template the refusal is | ||
| OVER-determined, naming the per-shot ``single_shot_value``/``shot_*`` | ||
| scalar companions alongside it — so ``SpillAggregator`` records the | ||
| verdict, replays the pooled machinery byte-identically while the shard | ||
| fits in one block, and raises ``SpillOverflowError`` naming the field on | ||
| the first block close. The synthetic below declares only ``count`` + | ||
| ``rx_flux`` so the builder is isolated as the cause; the last test pins | ||
| the deployed config, where it is not the only one. Issue #508 changes | ||
| NONE of this: the shared digest-family registry feeds the D24 pyramid | ||
| classification (stored-payload folds), not the build-time spill fold, | ||
| which would need the noise-model columns re-threaded per block. | ||
| """ | ||
|
|
||
| def test_probe_refuses_the_waveform_builder(self): | ||
| # The per-centroid channel passes the temporal arm (issue #477); the | ||
| # refusal is the ragged function arm — no cross-block fold law. | ||
| cfg = _config(_waveform_variables(), streaming=_SPILL) | ||
| with pytest.raises(ValueError, match="'rx_flux'.*build_waveform_digest.*fold law"): | ||
| validate_spill_fold(cfg) | ||
|
|
||
| def test_spill_accepts_the_config_as_non_mergeable(self): | ||
| # Accepted — single-block exact — with the probe's verdict recorded so | ||
| # a block close can name the field (issue #474 message discipline). | ||
| cfg = _config(_waveform_variables(), streaming=_SPILL) | ||
| agg = SpillAggregator(cfg, _grid(cfg), "pandas", 1) | ||
| assert not agg._mergeable | ||
| assert agg._digest_fields == {} | ||
| assert "rx_flux" in agg._fold_problems and "fold law" in agg._fold_problems | ||
| agg.close() | ||
|
|
||
| def test_single_block_regime_is_byte_identical_to_pooled(self, monkeypatch): | ||
| # The SERC mechanism: one block -> the pooled replay, payload AND the | ||
| # per-centroid temporal channel byte-identical to the pooled path. | ||
| key = _shard_key() | ||
| pooled_cfg = _config(_waveform_variables()) | ||
| spill_cfg = _config(_waveform_variables(), streaming=_SPILL) | ||
| grid = _grid(pooled_cfg) | ||
| dfs = _with_waveform_columns( | ||
| _granule_dfs(grid, key, _CELL_LISTS[:3], obs_per_cell=40, seed=6, times=True) | ||
| ) | ||
| df_p, ragged_p, _ = _run(monkeypatch, pooled_cfg, grid, key, list(dfs)) | ||
| df_s, ragged_s, _ = _run(monkeypatch, spill_cfg, _grid(spill_cfg), key, list(dfs)) | ||
| pd.testing.assert_series_equal(df_p["count"], df_s["count"]) | ||
| assert set(ragged_p) == set(ragged_s) == {"rx_flux"} | ||
| pay_p, idx_p, locs_p, times_p = _channels_of(ragged_p["rx_flux"]) | ||
| pay_s, idx_s, locs_s, times_s = _channels_of(ragged_s["rx_flux"]) | ||
| assert idx_p == idx_s and len(pay_p) > 0 | ||
| assert locs_p is None and locs_s is None | ||
| for a, b in zip(pay_p, pay_s, strict=True): | ||
| np.testing.assert_array_equal(a, b) | ||
| for a, b in zip(times_p, times_s, strict=True): | ||
| np.testing.assert_array_equal(a, b) | ||
|
|
||
| def test_block_close_raises_overflow_naming_the_field(self, monkeypatch): | ||
| # The recorded boundary of the mechanism above: past one block there is | ||
| # no fold law, and the overflow splices the probe's verdict. | ||
| from zagg.processing.spill import SpillOverflowError | ||
|
|
||
| _force_tiny_blocks(monkeypatch) | ||
| key = _shard_key() | ||
| cfg = _config(_waveform_variables(), streaming=_SPILL) | ||
| grid = _grid(cfg) | ||
| dfs = _with_waveform_columns( | ||
| _granule_dfs(grid, key, _CELL_LISTS[:2], obs_per_cell=10, seed=1, times=True) | ||
| ) | ||
| with pytest.raises(SpillOverflowError, match="'rx_flux'.*build_waveform_digest.*fold law"): | ||
| _run(monkeypatch, cfg, grid, key, dfs) | ||
|
|
||
| def test_the_shipped_template_is_refused_by_more_than_the_builder(self): | ||
| # The synthetic above isolates the builder; the DEPLOYED config is | ||
| # over-determined. Seven of its eight fields are per-shot scalars with | ||
| # no cross-block fold either, so even if issue #508 admitted | ||
| # ``build_waveform_digest`` to a shared digest-family registry the GEDI | ||
| # store would stay non-mergeable -- ``spill_blocks_closed: 0`` remains a | ||
| # hard precondition of the shipped template. Its own | ||
| # ``worker.extra_disk`` comment says the same: "validate_spill_fold | ||
| # rejects it (build_waveform_digest and the single_shot_value | ||
| # companions have no cross-block fold law)". | ||
| from zagg.config import default_config | ||
|
|
||
| with pytest.raises(ValueError) as exc: | ||
| validate_spill_fold(default_config("gedi01b_waveform_healpix_hive")) | ||
| message = str(exc.value) | ||
| assert "'rx_flux'" in message and "build_waveform_digest" in message | ||
| for name in ( | ||
| "shot_count", | ||
| "shot_number", | ||
| "noise_mean", | ||
| "noise_stddev", | ||
| "rx_energy", | ||
| "elevation_bin0", | ||
| "elevation_lastbin", | ||
| ): | ||
| assert f"'{name}'" in message | ||
| assert "zagg.stats.waveform.single_shot_value" in message | ||
|
|
||
|
|
||
| class TestSpillFoldProbe: | ||
| """The spill mergeability probe (validate_spill_fold) vs merge mode.""" | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 from Claude (review)
Verified: taking this comment's advice today publishes a flux ladder that declares itself
countsand carries none of the leaf's calibration provenance.Emitting the leaf template and the overview template side by side for this config (
_overview_config(composable_fields(declared_fields(cfg)[0])),output.pyramid = {}):and the overview root records only
{"class": "approximate", "method": "tdigest_kway"}(sweep_overview._field_provenance)._overview_configcarriesgainthrough only whenmeta["weights"] not in (None, "counts")(pyramid.py:222-226→sweep_overview.py:2053-2056), and this template'sweights: fluxis still commented out pending #431 (lines 187-188 above), so nothing is carried.Two things to decide before the ladder is advertised here:
weightspayload. Spec §2.0 says an absent key MUST be read ascounts, and §2.1 then requires integer weights ≥ 1 whose sum is the cell's exact observation count. rx_flux weights are(count − noise_mean)·gainfloats. The leaf already carries that mismatch (pre-existing, GEDI waveforms 1/3: §2 counts/flux weights declaration + δ=8,192 raise #431's job), but D24 composability: build_waveform_digest not admitted as approximate — GEDI pyramid declares class none with pyramid on #508 is what replicates it across every level of a ladder, and this comment is what invites an operator to turn it on.weights: flux, the carry-through that does exist propagates a bareattrs: {"gain": 1.0}— noname/versionin any spelling, which §2.0 makes REQUIRED beside a flux declaration ("againkey carrying at minimum the gain constant'snameandversion"), andgain_name/gain_version/clip/operating_pointstill never reach an overview. So the flux ladder is not self-describing even after the rebase this comment points at.Cheapest fix consistent with the phase scope: sequence the "so
pyramid: {}builds the ladder" half of this comment behind #431 (or behind a carry-through of the field's declaredattrs), rather than asserting it now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 from Claude
Folded as a durable record in the template, 69cf3c7 — not as a src change, because the leaf and the overview agree here and the divergence is #431's.
What changed. The
# weights: fluxpending-rebase comment now says why it is load-bearing for the pyramid and not just the leaf:zagg.pyramid.declared_fieldskeysweights(and thegainbeside it) into the manifest entry only when it is set, andsweep_overview._overview_configcarries that pair into every overview level's template on the same condition — so commented out, leaf and overview agree by ABSENCE, which spec §2.0 makes readers take ascountswhile these weights are(count − noise_mean)·gainfloats. It ends with the operator instruction: enable before building a GEDI ladder anyone will analyze. Thepyramid: falsecomment this thread is anchored to now points at that note, so the "pyramid: {}builds the ladder" invitation does not read as unqualified.Point (2) verified, and standing for #431. Simulating the rebase (
weights: fluxon the shipped template,pyramid = {}) the manifest entry is{..., "weights": "flux", "gain": 1.0}and the overview template is{..., "weights": "flux", "attrs": {"gain": 1.0}}— a bare scalar. Noname/versionin any spelling, which §2.0 makes REQUIRED beside a flux declaration, andgain_name/gain_version/clip/operating_pointnever reach an overview at all. So the flux ladder is not self-describing even after the rebase. Recording it here rather than changing src: the carry-through shape ispyramid.py:222-226/sweep_overview.py:2053-2056, which is exactly the code #431 touches when it turns the declaration on, and fixing it here would fold a §2.0 conformance change into a composability PR.Declined as #431 scope, by design: keying
weightsunconditionally, or widening the carry-through to the field's declaredattrs, or gating the D24 flip behind the weights declaration. Point (1)'s underlying mismatch is pre-existing at the leaf; #508 replicates it across levels only for a store built withpyramid: {}before #431 lands, which the comment now warns against and the CA runbook does not do (pyramids-off on 0.49, ladder retrofit sweep-only).Gates:
ruff check src testsclean except the pre-existingN818inregistry.py;pytest tests/test_read_vlen.py tests/test_config.py tests/test_semantics.py tests/test_stats_toc.py tests/test_spill_crossblock.py617 passed.