-
Notifications
You must be signed in to change notification settings - Fork 1
D24 composability: admit build_waveform_digest via a shared digest-family registry (issue #508) #510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
D24 composability: admit build_waveform_digest via a shared digest-family registry (issue #508) #510
Changes from 1 commit
eb17805
76468a0
0de5dbf
c1fc01a
1123cd2
69cf3c7
218c9f3
c328280
a660274
eef92f2
d472c23
159d8b3
b022346
13b26ff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -259,6 +259,18 @@ def test_a_temporal_waveform_field_is_still_none(self): | |
| == "approximate" | ||
| ) | ||
|
|
||
| def test_gedi_waveform_template_classifies_none_today(self): | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude (review) Phase 1 missed the one gate pair that is already inconsistent — The plan comment sets the exit condition for this phase: "The existing tuples keep their exact current members and semantics unless phase 1 proves a gate is already inconsistent, in which case the finding is raised on the issue rather than silently unified." That case exists today and nothing in this commit pins it.
#: Ragged reducers the SPILL fold additionally accepts (issue #370): ...
#: It folds k-way, like the ``build_tdigest`` it delegates to.
_TDIGEST_WHERE_FUNCTION = "zagg.stats.tdigest.build_tdigest_where"
_TDIGEST_SPILL_FUNCTIONS = (*_TDIGEST_FUNCTIONS, _TDIGEST_WHERE_FUNCTION)D24 disagrees — it tests That is the same latent bug shape as #508, in a second template, and phase 2 will walk into it: a registry named "folds by the k-way t-digest law" built from the spill-side membership flips both strata fields without a decision being recorded. Simulated the phase-2 change to confirm it is not hypothetical: Concretely:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude Folded, both halves. (a) Pinned — 76468a0 adds (b) Raised — #508 (comment) states the Phase 2 stays scoped to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude Correction to the count in my previous reply: |
||
| # Issue #508 phase 1 baseline: the SHIPPED template's rx_flux — | ||
| # build_waveform_digest with a per-centroid clock — is D24 class | ||
| # ``none`` today, which is exactly what the SERC probe observed | ||
| # (manifest ``{"class": "none"}``, no ladder, even with pyramid on). | ||
| # The meta-level pin above records the mechanism; this one records | ||
| # that the template hits it. | ||
| from zagg.config import default_config | ||
|
|
||
| classes = composability_classes(default_config("gedi01b_waveform_healpix_hive")) | ||
| assert classes["rx_flux"] == "none" | ||
|
|
||
| def test_located_declaration_rides_the_manifest_entry(self): | ||
| # The manifest is the only description the overview WRITER has of a | ||
| # field (``_overview_config``), so the channel has to be recorded there | ||
|
|
||
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)
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
gedi01b_waveform_healpix_hivethe refusal is over-determined — the builder is one of eight problems, and the other seven are the per-shot companions:The template's own comment says the same thing (
worker.extra_disk): "validate_spill_fold rejects it (build_waveform_digestand thesingle_shot_valuecompanions have no cross-block fold law)".Why it matters for a characterization phase:
_waveform_variables()declares onlycount+rx_flux, so the pinned baseline is a config where the builder is the sole cause. A later reader — or phase 2 — could conclude from this class that admittingbuild_waveform_digestto a shared digest-family registry would change the GEDI store's spill posture. It would not: the seven scalar companions keep the shipped config non-mergeable regardless, sospill_blocks_closed: 0stays a hard precondition of the deployed template. That is the load-bearing baseline fact and it is currently unrecorded.Suggested fix — one extra pin on the real config, plus a docstring correction:
and reword the docstring's second sentence to "
validate_spill_foldrefuses the builder and the per-shotsingle_shot_value/shot_*companions".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 in 0de5dbf.
New pin —
test_the_shipped_template_is_refused_by_more_than_the_builderrunsvalidate_spill_fold(default_config("gedi01b_waveform_healpix_hive"))and asserts the refusal namesrx_flux/build_waveform_digestand all seven per-shot companions (shot_count,shot_number,noise_mean,noise_stddev,rx_energy,elevation_bin0,elevation_lastbin) pluszagg.stats.waveform.single_shot_value. So the over-determination is recorded field-by-field: even if issue #508 admitted the builder, the deployed store stays non-mergeable andspill_blocks_closed: 0remains a hard precondition.Docstring corrected — it no longer attributes the shipped templates verdict to the builder alone; it now reads that
validate_spill_foldrefuses the builder "and on the SHIPPED template the refusal is OVER-determined, naming the per-shotsingle_shot_value/shot_*scalar companions alongside it", and says explicitly that the 2-field synthetic isolates the builder while the last test pins the deployed config where it is not the only cause. The new tests comment quotes the templates ownworker.extra_diskline for provenance.tests/test_spill_crossblock.py::TestWaveformSpillBaselinegreen (5 passed; 29 across both targeted classes, 302 across the three touched suites).