GEDI waveforms 1/3: §2 counts/flux weights declaration + δ=8,192 raise - #431
Conversation
| continue | ||
| # Mismatched §2.0 weights declarations refuse to merge (issue #424); | ||
| # the enclosing per-child guard skips the child loudly. | ||
| check_weights_match(dict(arr.attrs), meta, name) |
There was a problem hiding this comment.
🤖 from Claude (review)
_overview_config never stamps weights, so every overview a flux store writes misdeclares its payload — and this gate then refuses every child in the cascade.
_write_overview builds the overview's arrays from _overview_config(fields) (src/zagg/sweep_overview.py:1695-1725), which reconstructs each approximate field as:
variables[name] = {
"kind": "ragged",
"function": "zagg.stats.tdigest.build_tdigest",
"inner_shape": list(meta.get("inner_shape") or [2]),
"dtype": meta.get("dtype", "float32"),
"fill_value": 0,
}No weights, and no attrs (so no gain either). The new stamp in ragged_array_spec only fires when the config carries the declaration, so an overview array comes out with a bare ragged block. Two consequences:
- Spec §2.0 violation on the overview itself. The folded payload holds flux weights but declares none, so §2.0 (
docs/specification.md:253-256) says a reader MUST read it as"counts"— i.e. thatsum(weights)is an exact observation count. Thegainprovenance §2.0 makes REQUIRED is gone too. - The cascade breaks hard.
_fold_child(this line) compares the child's stored declaration against the manifest's. A flux store's overview children read"counts", the manifest says"flux",check_weights_matchraises, and_cascade_node's per-child guard (sweep_overview.py:1525-1530) swallows it asskipping unreadable overview, counts itfailed/unreadable, and leaves the child's span at fill. Cascade is the ratified default fold source, so every level above the finest would come out empty for a flux store, with only a warning per child.
Reproduced against this branch (cba7546):
$ python -c "from zagg.sweep_overview import _overview_config, check_weights_match; ..."
overview array attrs: {'ragged': {'spec': 'zagg-ragged/1', 'element': {'dtype': 'float32', 'shape': [-1, 2]}}}
gate: RAISES -> field 'rx_flux': stored weights declaration 'counts' does not match
the manifest's 'flux' — merges are legal only between matching declarations
Nothing in the suite catches it: the flux/ fixture is built with pyramid=None (tools/generate_spec_fixtures.py:694), and the pyramid/column fixtures are counts-only, so no test ever folds a flux-declared field.
Suggested fix: carry the declaration and its provenance through _overview_config for non-default fields —
variables[name] = {
...,
"fill_value": 0,
**({"weights": meta["weights"]} if meta.get("weights") else {}),
**({"attrs": {"gain": meta["gain"]}} if meta.get("gain") else {}),
}which means pyramid.declared_fields also has to carry the gain mapping onto the manifest field entry (an array stamped weights: "flux" without gain is itself non-conformant — and note PipelineConfig does not run validate_config at construction, so the template will happily emit the half-declared form). Then add a cascade test over a flux-declared field so the fold-of-folds path is pinned; a flux variant of the pyramid/column fixture geometry would do it.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 27a0287 — the declaration (and its provenance) now rides the manifest field entry through to the overview template.
pyramid.declared_fieldscarriesgainalongsideweightson a non-default entry: the manifest is the only thing_overview_configreconstructs a field from, and §2.0 makesgainREQUIRED on a flux-declared array, so recordingweightswithout it would just move the non-conformance one level up._overview_configthreads both through —weightson the variable (whichragged_array_specstamps as the sibling key) and{"attrs": {"gain": ...}}(whichapply_field_attrsmerges). Both are keyed only for a non-default declaration, so a counts store's template bytes are unchanged.docs/specification.md§4.5 now says the pair rides together and why.
The flux/ fixture already held the byte evidence — its all.pyramid.zarr column arrays were written through this path and carried no weights. Regenerated (--only flux), and only the column's rx_flux array attrs, the column root attrs and the manifest field entry moved; every array content hash and flux.expected.json are unchanged, and the leaf is untouched.
Tests:
TestWeightsGate::test_overview_template_stamps_the_flux_declaration— emits a real template from a flux + a counts field entry, assertsweights/gainland on the flux array and nothing lands on the counts one, then runscheck_weights_matchover the emitted attrs, which is the cascade guard this was breaking.test_declared_fields_records_flux_onlygains thegainassertion.TestWeights::test_fold_arrays_re_declare_weights_and_gainin the conformance suite pins it on the committedflux/bytes.
| entry["delta"] = int(meta.get("delta") or 512) | ||
| # The budget the column fold actually compressed at (issue #424): | ||
| # the split overview_delta, not the leaf δ. | ||
| entry["overview_delta"] = overview_fold_delta(meta) |
There was a problem hiding this comment.
🤖 from Claude (review)
A new key in the versioned zagg-column/1 attrs block, with no §4.6 spec update and a now-stale column/ fixture.
This line makes every column written from here on carry zagg_column.fields.<f>.overview_delta. That is a wire-format change to a versioned block, and the two things #340 / CLAUDE.md §4 require alongside it are missing:
- Spec.
docs/specification.md§4.6 still documents the block as{"class", "method", "delta", "dtype", "inner_shape"}— the JSON example at:1019-1021and the prose at:1034-1036("approximate entries additionally carrydtype/inner_shape/delta— enough to decode without the manifest"). The tolerance clause this PR adds ("a reader MUST tolerate entry keys it does not bind",:865-870) landed in §4.5 only, on the manifest's all-fields map. §4.6 has no such clause, so an external reader built from spec + fixtures alone (moczarr — the §7 contract) meets an undocumented key in a block it strict-decodes. - Fixture. The committed
tests/data/spec/column/1/1/2/1/3/all.pyramid.zarr/zarr.jsonandcolumn.expected.jsonboth pinh_tdigestas{"class","method","delta":16,"dtype","inner_shape"}— nooverview_delta. The writer and the committed conformance fixture now disagree.column/is not in the stale-by-design carve-out, which namesminimal/andkitchen_sink/only (tools/generate_spec_fixtures.py:62-68). (The suite passes only because the column tests compare the committed store against the committedexpected.json, never against a freshly written column —tests/test_column.py:435-442is the live-writer assertion, and it was updated.)
Suggested fix: extend §4.6's example + prose with overview_delta and the same tolerate-unknown-keys sentence §4.5 got, then regenerate the column fixture with the --only switch this PR conveniently adds (python tools/generate_spec_fixtures.py --only column) and refresh FROZEN_COLUMN/column.expected.json. Alternatively, key overview_delta only when it differs from delta — but that trades a documented key for a conditional one, and the spec update is needed either way.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 7b2cfac — spec and fixture both updated, per the #340 same-PR rule.
- §4.6. The JSON example's
h_tdigestentry now carries"overview_delta": 16, and the prose says what it is (the budget the column's fold actually compressed at — the split pyramid-fold budget rather than the leafdelta, both fold algebra and informative per §2.3) plus the same tolerate-unknown-keys sentence §4.5 got. - Fixture. Regenerated with
uv run python tools/generate_spec_fixtures.py --only column. The churn is exactlyoverview_delta: 16in three places — the column root attrs'zagg_column.fields.h_tdigest, the manifest's §4.5 pyramid entry, andcolumn.expected.json— plus regeneration timestamps. No array bytes moved, so no content hash changed andFROZEN_COMBINED/FROZEN_COMBINED_COLUMNare untouched; I committed only the three files with real content change and left the leafzarr.json/ stats sidecar (timestamp-only churn) alone, so the leaf stays byte-identical tominimal/.
Test: TestColumnArtifact::test_basename_role_and_attrs_grammar now pins the approximate entry's decode-without-the-manifest key set — {delta, overview_delta, dtype, inner_shape} — off the committed bytes rather than only comparing store against expected.json (both of which regenerate together, so the equality assert alone could not have caught this).
uv run --extra test pytest tests/test_spec_conformance.py tests/test_column.py -q is green.
| constant's `name` and `version` (the operating point of any write-time | ||
| clip rides alongside, writer-defined). | ||
|
|
||
| A reader MUST strict-check the value: an unknown declaration is a future |
There was a problem hiding this comment.
🤖 from Claude (review)
§2.0 ships a normative reader MUST that zagg's own reader does not honor.
grids.base.weights_declaration implements exactly this sentence, but nothing on the read path calls it. readers/tdigest_tensor._open_ragged (src/zagg/readers/tdigest_tensor.py:263-300) strict-checks the ragged block's spec, element dtype and shape — the precedent this sentence is modeled on — and never looks at weights. So on this branch:
- a payload declaring a future revision (
weights: "photons") decodes silently throughread_tensors, which is precisely what "MUST be refused, never read as either defined value" forbids; - a
weights: "flux"payload is rasterized as if its weight column were counts, anddtype="uint32"tensors additionally truncate the fractional weights.
The reader is the party this MUST is addressed to, and the check is two lines next to the existing spec gate:
from zagg.grids.base import weights_declaration
weights_declaration(arr.attrs) # raises on an unknown declarationIf binding flux semantics in the reader is deliberately #426's P5 read-validation phase, that is fine — but then either narrow the sentence (e.g. "a reader that binds the weight column MUST …") or record the deferral under "Questions for review", so the spec is not shipping a MUST ahead of the only in-tree reader. As written, tests/test_spec_conformance.py pins the fixture's attrs but no test asserts that a reader refuses an unknown declaration off a store.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 90b3135 — the reader honors the MUST, so the §2.0 sentence stands as written rather than being narrowed.
readers/tdigest_tensor._open_ragged now calls weights_declaration(dict(arr.attrs)) right beside the existing ragged spec gate, so an undefined value raises at open for every reader that goes through it (read_tensors, read_cell, read_raw_values, read_locations). Both defined values still open: what a reader BINDS of flux semantics is issue #426's read-validation phase, and this only refuses the undefined ones — which is exactly the sentence's scope ("never read as either defined value").
On the uint32 truncation: recorded as a dtype docstring note in read_tensors rather than a behavior change — a flux field's weights are positive reals, so an integer dtype rounds each bin's photoelectron estimate away and float32 is the right choice; the Raises section names the new refusal too. Binding flux at rasterization stays #426.
Tests in TestReadTensors: test_unknown_weights_declaration_raises (a weights: "photons" payload refused through both read_tensors and read_cell) and test_defined_weights_declarations_open (counts and flux both open and yield).
| """ | ||
| from zagg.grids.base import weights_declaration | ||
|
|
||
| stored = weights_declaration(dict(attrs or {})) |
There was a problem hiding this comment.
🤖 from Claude (review)
Missed call site: _field_drift doesn't cover the new declaration, so a weights mismatch surfaces at fold time — the exact failure mode that gate exists to prevent.
check_weights_match turns a store-vs-manifest weights disagreement into a per-leaf fold failure. _field_drift (src/zagg/sweep_overview.py:741-795) is the pre-flight that catches this class for the other ragged declarations, and its own docstring (:747-753) states the rationale verbatim: "without the gate the retrofit installs a declaration promising overviews the sweep then fails to read at FOLD time, later and with a worse error." weights is now one more thing a leaf can falsify, it is stamped on the array the probe already opens, and the approximate branch checks only spec / element dtype / inner_shape.
Concretely: declare_pyramid run against a counts store with a weights: flux config returns a clean validated summary and PUTs the declaration; the next sweep then logs skipping unreadable leaf for every leaf (sweep_overview.py:1379-1383) and writes fill — no error surfaced to the operator beyond a warning per leaf.
Suggested: in the approximate branch, after the inner_shape comparison —
stored = weights_declaration(arr.attrs) # already raises on an unknown value
declared = meta.get("weights") or "counts"
if stored != declared:
return (
f"field {name!r}: stored weights declaration {stored!r} != declared "
f"{declared!r} — merges are legal only between matching declarations (§2.0)"
)There was a problem hiding this comment.
🤖 from Claude
Fixed in c04d91f — _field_drift's approximate branch now compares the declaration, essentially as suggested (absent ⇒ counts on both sides, matching how the fold gate reads it), so the retrofit refuses instead of PUTting a declaration the next sweep can only fail per-leaf on.
One deliberate detail: weights_declaration raising on an undefined stored value propagates out of the probe rather than being reported as drift. That store is unreadable, not merely mis-declared — same posture as the ragged spec-revision gate two checks up — and the docstring now says so.
Tests in TestDeclarePyramid:
test_flux_declaration_over_a_counts_store_refuses— the operator-facing case from the finding:declare_pyramidagainst a counts store with aweights: fluxconfig raises and leaves nopyramidkey on the manifest.test_store_side_weights_drift_refuses— the other direction (flux-stamped leaf, counts config).test_undefined_stored_weights_raises.
|
🤖 from Claude Status: all phases complete,
Generated by Claude Code |
|
🤖 from Claude espg ruled on all four open review questions (in-session, 2026-08-16); the body's "Questions for review" section now records each resolution inline. Summary:
All phases complete, all findings folded, CI green on c04d91f — marking ready for review. |
Closes #424
Closes #414
First of the three GEDI-waveform implementation PRs (plan + rulings: #422). Two halves, per the issue scope:
§2 weights declaration.
docs/specification.md§2 gains a §2.0weightsdeclaration: digest payloads declare their weight-column semantics via a sibling spec-owned attrs key on the payload array (ruled: not inside theraggedblock, so thezagg-ragged/2migration stays metadata-only — ruling on #422 citing zarr-developers/zarr-extensions#71)."counts"= integers ≥ 1, sum exact (today's contract; absent key ⇒ counts, every existing store conformant verbatim);"flux"= positive reals, sum estimates detected photoelectrons,gainname/version provenance required in attrs. Merges are legal only between matching declarations — enforced at the one place zagg merges digests from stored arrays (sweep_overview.check_weights_match, wired into the leaf and cascade folds; a mismatch raises and the fold's guard skips the source loudly). The writer stamps the key at template time (ragged_array_spec→ both grid backends), config gainsweights:validation, and the newflux/conformance fixture lands in the same PR per the #340 rule.weights: countsnormalizes out of the semantic core (explicit ≡ absent ≡ the pre-#424 hash);weights: fluxhashes — it is output-defining.δ = 8,192 raise +
overview_delta. The four packaged ATL03 configs raisedelta: 256 → 8192(the measured loss-free leaf bound — #422's statewide CA scan: δ=8,192 leaves exactly one storm-artifact cell lossy) and declare the new field-leveloverview_delta: 512, the pyramid/overview fold budget (accuracy bound ~1/δ) that also caps the sweep's k-way fold buffers (~33 MB vs ~1 GB saturated at 8,192). All four overview-fold sites (sweep_overview._fold_node/_fold_child,column.fold_column,sweep_stage._stage_fold) resolve the budget through one helper,overview_fold_delta: a declared value wins; absent (every pre-#424 manifest) falls back to the leaf δ capped at 512 — byte-identical to the historical fold-at-leaf-δ behavior for every manifest ever written (all carried δ ≤ 512), while a raised leaf δ can no longer saturate fold buffers. The manifest records the resolvedoverview_deltaper approximate field so stores are self-describing._DEFAULT_DELTAstays 512 (raising it silently changes output under an unchanged semantic hash — the packaged configs are explicit instead), andoverview_deltais normalized out of the semantic core (overview artifacts are packaging, like the pyramid knob itself).Semantic-hash consequence, spelled out:
params.deltais hashed, so these configs get a NEWsemantic_hash— reusing an existing product name against a δ=256 store refuses up front. Per the #422 transition ruling: same product names, fresh store roots; espg sweeps old outputs after landing.Phases
flux/fixture text (0fab8eb)WEIGHTS_ATTRstamp inragged_array_spec+ both grid templates;weights:/overview_deltavalidation; output-signature plumbing; tests (95e2492)weights: counts≡ absent;overview_deltais packaging);check_weights_matchwired into the leaf + cascade folds; manifest records non-defaultweights; tests (c80da29)flux/conformance fixture viatools/generate_spec_fixtures.py --only flux→tests/data/spec/flux/+ conformance tests; frozen combined-hash literal; committedminimal/is the absent-key ⇒ counts pin (676b35d)delta: 8192+overview_delta: 512;overview_fold_deltawired at all four fold sites + manifest/column provenance; singleton-preservation tests at combined n = δ (build, pairwise, k-way); declared-δ propagation pins for streaming and spill state;_DEFAULT_DELTA == 512pinned (55e588a, cba7546)Preallocation audit (issue requirement)
Nothing allocates δ×cells.
_compressallocates O(n) working arrays (cumw,k_right) plus O(k ≤ δ) outputs per populated cell only; per-cell digest state everywhere (streaming dicts, spill_digests, sweep accumulation lists) is dynamic at Σ min(n, δ). Audited via grep oversrc/zaggfor allocation calls involvingdelta— the only hits are the two O(1)/O(n) lines instats/tdigest.py(np.zeros(1)for the k-left seed,np.ones(n)for unit weights). The sweep's k-way fold buffer is bounded byoverview_deltaper this PR, not leaf δ.How tested
uv run --extra test pytest -v: full suite green locally excepttests/test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds, which fails identically on the base commit (local pip index cannot resolvezarr>=3.1.5inside the deployment build script — environmental, pre-existing, unrelated).ruff check src tests: clean for every touched file; one pre-existingN818insrc/zagg/registry.py(also fails on base — not touched here).ruff format --check src tests: clean for every touched file; one pre-existing failure undertests/data/benchmark/(also fails on base).pre-commit run --all-files: remaining ruff/mypy/codespell findings are pre-existing (bench/demo notebooks, missingtypes-PyYAMLstubs locally,DataSourceDicttyping debt) and identical on base; no finding points at a line this PR adds.gainprovenance,overview_deltabounds, attrs reservation), template stamping (sibling key, absent ⇒ no key, located sibling clean), hash stability (counts ≡ absent,overview_deltapackaging, flux hashes), merge gate (mismatch refuses both ways, unknown declaration refuses), fixture conformance (§2.0 attrs, fractional flux sums, frozen O11 literals), singleton preservation at n = δ across build/pairwise/k-way, streaming/spill declared-δ pins, packaged-config budget pins.minimal/,kitchen_sink/,column/,pyramid/) untouched — byte-identical, andminimal/now doubles as the committed absent-key ⇒ counts pin.overview_delta).Questions for review
_compressrequires strictly positive weights). Flag if "nonnegative" (zero-weight rows storable) was the intent.Resolved (espg ruling, 2026-08-16): confirmed as written — zero-weight observations are not stored; "positive" stands, no change.
gain: {name, version, …}as the required provenance shape (config-validated). The exact operating-point keys are left writer-defined until the GEDI waveforms 2/3: generic vlen reader primitives, paired-asset shardmap, flux transform + gedi01b template #425 transform lands. If you want the clip operating point normative now, say the word and I'll extend §2.0.Resolved (espg ruling, 2026-08-16): deferred as proposed, with a standing constraint on the eventual grammar — the spec stays instrument-agnostic: operating-point provenance describes the measurement (thresholds, an FP/FN operating point), never a specific sensor's conventions; nothing GEDI-specific lands in §2.0. The same declaration must carry LVIS/ATL03/future waveform sensors config-side.
weightsstamp yet, so a flux-declared manifest would refuse to cascade from them (leaf folds are fine — leaves are stamped). Stamping overview/column arrays is deferred to the PR that first makes a flux field composable (today's plan keeps the waveform field classnone, so this path is unreachable). Flag if you want the stamp now.Resolved — this question was stale when asked: the adversarial-review fold already landed the stamp in this PR (27a0287): the manifest field entry carries
weights+gain,_overview_configstamps every derived-array template (pyramid levels, the §4.6 column store, the staged sweep — all three route through it), and the committedflux/fixture pins the declaration on both pyramid levels, so the cascade gate accepts overview sources. Counts stores are byte-unchanged. espg ratified wanting the stamp in-PR (2026-08-16); no further change needed.Resolved (espg ruling, 2026-08-16): confirmed fleet-side; ablation numbers go to GEDI waveform aggregation template: photon-currency digests (plan) #422 when the run happens. (Distinct from the
overview_deltasplit, which is implemented in this PR.)N818inregistry.py, benchmark README format, mypy stub debt) were left alone per §4 — none are touched by this change.