D24: admit build_tdigest_where to the digest family; honest composition declaration (issue #515) - #516
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
| manifest = json.loads(CA_MANIFEST.read_text()) | ||
| pyramid = manifest["pyramid"] | ||
| assert pyramid["spec"] == "zagg-pyramid/2" | ||
| assert [e["node"] for e in pyramid["overviews"]] == list(range(9, -1, -1)) |
There was a problem hiding this comment.
🤖 from Claude
(The review bodies on all six threads posted as the literal text @f1.md…@f6.md — gh api -f does not expand @file, -F does. Restating each finding in one line so the threads stay readable; the folds below are against the findings as written.)
Finding 1 — the known-answer stops one assertion short: the ladder/fold_source/exact_levels are asserted against the static fixture, never against code. Folded in fd23346.
TestLiveCaManifestFixture.test_declaration_writer_rebuilds_the_published_block now runs the writer and diffs it against the published block:
config = default_config("atl03_tdigest_strata_healpix")
config.output["pyramid"] = {} # the template ships pyramid: false
config.output["grid"]["parent_order"] = 9 # the CA store's shard order
block = build_pyramid_block(config, 9, chunk_order=13)spec, overviews (the 9..0 ladder with its per-node cells) and the whole overview block minus fields compare equal to the fixture; the fields map is then asserted to differ ONLY in the two ruled entries — count byte-identical, composition still {"class": "none"} until phase 3, h_tdigest_signal/h_tdigest_noise the phase-2 admission (class: approximate). Phrased that way because phase 2 (2218b82) already flipped the declared classes, so a flat equality against the fixture is no longer the right pin.
| assert ( | ||
| manifest["semantic_hash"] | ||
| == "b9b15fdde78f147c15c929da8ca93de21930ad5c552ae082c5d8998fb83ada21" | ||
| ) |
There was a problem hiding this comment.
🤖 from Claude
Finding 2 — the identity story overclaims: no in-tree config reproduces the live store's semantic core, and semantic_hash is a frozen manifest key. Folded as a comment correction in e596245 (no attempt to reproduce the hash).
Re-measured on the current tree, matching the finding exactly: fixture b9b15fdd…, default_config("atl03_tdigest_strata_healpix") → 0ac7d33b…, tests/data/benchmark/configs/atl03_tdigest_healpix_o9_hive_strata.yaml → 5ebf740f…. The comment above the semantic_hash assert now says the hash names the record, not a config; that no config in the tree reproduces it (both divergent hashes named by prefix); that the before/after is pinned against reality for the per-field declaration only; and that it is a frozen key (hive._FROZEN_MANIFEST_KEYS), so the retrofit write the PR body plans against that root needs overwrite=True or ensure_manifest refuses it.
The specific sentence the finding quoted ("the manifest fixture above is what this code writes…") no longer exists — phase 2 (2218b82) rewrote that comment when the test became test_declared_fields_no_longer_reproduce_the_live_ca_declaration, so the correction landed on the surviving identity claim instead.
On the provenance nit: the fixture's top-level keys are exactly build_manifest's /1 output (spec, dataset, semantic_hash, cell_order, shard_order, split_schedule, path_grouping, pyramid, generated_at) with nothing elided, so the module docstring's "byte-for-byte" is right and the PR body's "fragment" is the wrong word. Left standing for the PR body — out of scope for a test-file fold.
| # admits the reducer by name in its scalar branch. | ||
| from zagg.processing.streaming import _COMPOSITION_FUNCTION | ||
|
|
||
| assert _COMPOSITION_FUNCTION == "zagg.stats.composition.pack_composition" |
There was a problem hiding this comment.
🤖 from Claude
Finding 3 — test_composition_is_spill_admitted pins a string constant, so the option (b) flip (removing the name from the validate_spill_fold scalar branch) leaves it green. Folded in 0d9cde1.
Kept the test rather than dropping it, and made it assert the branch as a pass/raise pair on one config:
config = default_config("atl03_tdigest_strata_healpix")
fields = get_agg_fields(config)
assert fields["composition"]["function"] == _COMPOSITION_FUNCTION
validate_spill_fold(config)
fields["composition"]["function"] = "zagg.stats.composition.pack_composition_v2"
with pytest.raises(ValueError, match="scalar function .* has no cross-block fold"):
validate_spill_fold(config)The positive call now fails under option (b) (the shipped composition field is scalar pack_composition), and the negative control shows the pass is the by-name admission rather than a branch that waves every scalar through. test_strata_template_passes_the_spill_gate keeps carrying the shipped shape, and tests/test_spill_crossblock.py::test_strata_and_composition_config_is_mergeable still carries the deep path — this file's job is only that its characterization cannot stay green through the flip.
| classes = composability_classes(default_config("atl03_tdigest_strata_healpix")) | ||
| assert classes["count"] == "exact" | ||
| assert classes["h_tdigest_signal"] == "none" | ||
| assert classes["h_tdigest_noise"] == "none" |
There was a problem hiding this comment.
🤖 from Claude
Finding 4 — nothing records WHY the strata fields classify none, so phase 2's flip would not be provably the registry admission. Left standing as superseded by phase 2 (2218b82) — no residual gap I can find worth another assert.
Both halves the finding asked for landed with the admission itself:
- the exact line phase 2 flips —
tests/test_sweep_overview.py::TestComposabilityClasses::test_digest_family_registry_members_pinned_by_valuepins_DIGEST_FAMILY_FUNCTIONSby value, now(*_TDIGEST_FUNCTIONS, "zagg.stats.waveform.build_waveform_digest", "zagg.stats.tdigest.build_tdigest_where"), with the other two tuples asserted unchanged. Registry membership is the asserted mechanism, not a side effect. - the classifier preconditions —
test_where_strata_template_classifies_approximate(renamed from…_classifies_none_today) now drivesfield_composabilityon a meta directly and asserts the guard still bites:inner_shape: [2]→approximate,inner_shape: [3]→noneon the same function. And the template's own shape is pinned by value intests/test_strata_composability.py::TestD24Classification::test_declared_fields_no_longer_reproduce_the_live_ca_declaration, which asserts the full declared entry for both strata fields —inner_shape: [2],delta: 8192,overview_delta: 512,location: "leaf_id". A template edit droppinglocation: leaf_idor mis-declaringinner_shapefails there, so the "green for the wrong reason" path the finding describes is closed.
Adding _TDIGEST_WHERE_FUNCTION in _DIGEST_FAMILY_FUNCTIONS here as well would be a third copy of the registry pin, so leaving it.
| "h_tdigest_signal": {"class": "none"}, | ||
| "h_tdigest_noise": {"class": "none"}, | ||
| "composition": {"class": "none"}, | ||
| } |
There was a problem hiding this comment.
🤖 from Claude
Finding 5 — the class map gates a write path (column.composable_fields → leaf_column_plan/fold_column), and that half is unpinned even though the retrofit plan rests on it. Folded in c32fa62.
TestD24Classification.test_class_map_gates_the_leaf_column_write_path pins both sides of the flip:
assert composable_fields(CA_FIELDS_BEFORE) == {"count": CA_FIELDS_BEFORE["count"]}
fields, _ = declared_fields(default_config("atl03_tdigest_strata_healpix"))
assert sorted(composable_fields(fields)) == ["count", "h_tdigest_noise", "h_tdigest_signal"]The first line is the published store's before — only count survives the filter, which is why the CA store has no strata leaf columns to fold and the retrofit must re-read leaves. The second is the behavior change phase 2 carries beyond the manifest text: the admission turns worker-side strata column writes on for new runs, and a later change that widens or narrows what the column path carries now shows up as a red test rather than silently on the fleet.
| "dtype": "uint64", | ||
| "fill_value": 0, | ||
| "params": {"threshold": 2}, | ||
| "attrs": {"composition": {"of": "h_tdigest_signal", "threshold": 2}}, |
There was a problem hiding this comment.
🤖 from Claude
Finding 6 — the composition meta is hand-built and does not match the shipped field, and it re-pins what the template test already covers. Folded in d758a53 — driven off the template, as suggested.
test_composition_field_classifies_none_today (phase 2 renamed the class to TestD24Classification) now reads:
meta = get_agg_fields(default_config("atl03_tdigest_strata_healpix"))["composition"]
assert meta["function"] == "zagg.stats.composition.pack_composition"
assert field_composability(meta) == "none"So the fabricated params: {"threshold": 2} / two-key attrs are gone and the verdict follows the shipped field: a declaration the template gains (temporal, location, a params change) moves this test rather than leaving a green pin over a shape no store writes. Kept rather than dropped because it names the reducer that drives the arm, which test_strata_template_classes does not.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
…ssue #515) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
…(issue #515) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
…sue #515) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
… meta (issue #515) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
| elif cls == "packed": | ||
| block = (meta.get("attrs") or {}).get("composition") or {} | ||
| of = block.get("of") | ||
| if classes.get(of) != "approximate": |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] The packed guard rail checks the of digest's class but not its weights declaration, and the law only holds for weights: counts.
merge_composition_kway divides by n, and payload_weight (sweep_overview.py:226) rounds the of digest's summed centroid weights to an int — its docstring justifies that with "strata weights are exact stratum photon counts (spec §2)". Nothing enforces it. A config whose attrs.composition.of names a §2.0 weights: flux digest passes every gate here: field_composability returns approximate for it (weights are not consulted), classes.get(of) == "approximate" here, and _validate_composition_attrs (config.py:894-908) only checks of != name and kind == "ragged". The result is a packed declaration whose divisor is a flux sum silently truncated to an integer at every fold site — wrong-but-plausible lanes, exactly the failure mode §2.0/issue #424 introduced the weights gate to stop (check_weights_match is not reachable on this pairing: the packed arms in _fold_node/_fold_child never call it).
Suggested fix: tighten the demotion predicate to the pairing the law actually needs, e.g.
of_meta = agg.get(of) or {}
if classes.get(of) != "approximate" or (of_meta.get("weights") or "counts") != "counts":and/or add the same rule to _validate_composition_attrs so it fails at template time with a named error rather than demoting silently.
There was a problem hiding this comment.
🤖 from Claude
Folded in 3367b7f — the demotion predicate now carries the weights half.
declared_fields' packed arm (src/zagg/pyramid.py):
of_weights = (agg.get(of) or {}).get("weights") if of else None
if classes.get(of) != "approximate" or of_weights not in (None, "counts"):Took the weights off the SOURCE meta (agg, i.e. get_agg_fields) rather than off the built fields entry, because the manifest entry keys weights only when non-default — reading the source is the same test without depending on iteration order. Absent-or-"counts" is the pairing the law admits, matching how the fold gate itself reads the key.
Test beside the existing one: test_declaration_demotes_packed_over_a_flux_weighted_divisor drives one config twice — the counts control still declares packed/of: h_sig, and the flux variant (weights: flux + the §2.0 gain provenance the config gate requires) leaves h_sig folding as approximate while composition becomes {"class": "none"} and lands in excluded.
Left the _validate_composition_attrs half of the suggestion standing: refusing the pairing at TEMPLATE time would make a config that is legal today (a flux digest plus a composition field, no pyramid declared) fail to load, which is a scope change rather than a fix — the declaration is where this PR's honesty contract lives, and it now refuses. Flagging it here for @espg rather than acting on it.
| of = block.get("of") | ||
| if classes.get(of) != "approximate": | ||
| fields[name] = {"class": "none"} | ||
| excluded.append(name) |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] The demotion has no diagnostic of its own — the operator gets a message that names the wrong cause.
When this arm fires, the only output is warn_excluded's generic line (pyramid.py:273-277): "fields ['composition'] are non-composable (D24 class 'none') and will exist ONLY at native resolution … declare a derived summary to opt in, issue #201". That is false for this path: the field is classified packed by field_composability; it was demoted because its divisor is not approximate. An operator reading that warning will go looking at the composition reducer, not at attrs.composition.of's target — and in the common case the of digest is itself in excluded right beside it, so the actual root cause is one line away but never stated.
This matters most for exactly the run the PR is aimed at: the CA retrofit's value is the strata pyramid plus its composition companion. If a future template change knocks h_tdigest_signal out of approximate (a non-(2,) inner_shape, a per-cell temporal companion, an expression), composition drops out silently with a misattributed warning.
Suggested fix: log at the demotion site before continue, e.g.
logger.warning(
f"pyramid: field {name!r} classifies 'packed' but its attrs.composition.of "
f"{of!r} is declared {classes.get(of)!r}, not 'approximate' — the fold divides "
f"by that digest's weight at every level, so composition is demoted to "
f"'none' (issue #515)"
)There was a problem hiding this comment.
🤖 from Claude
Folded in 89f7776 — the demotion now logs its own cause before continue:
pyramid: field 'composition' classifies 'packed' but its attrs.composition.of
'h_sig' is declared 'approximate'/weights='flux', not approximate/counts — the
fold divides by that digest's per-cell weight at every level (spec §3.3/§3.4),
so 'composition' is demoted to 'none' and exists ONLY at native resolution
Both demotion causes are in one line (the of class AND its §2.0 weights, per the sibling finding), so the operator reads the actual root cause rather than warn_excluded's "non-composable (D24 class 'none')" — which stays, since the field genuinely ends up excluded, but is no longer the only trace.
Pinned by the test in the same commit: test_declaration_demotes_packed_over_a_flux_weighted_divisor asserts the record names attrs.composition.of 'h_sig' and weights='flux' under caplog, so a future edit that drops the cause from the message fails rather than silently reverting to the misattributed warning.
| #: through arbitrary chains and counts stay within one lane quantization per | ||
| #: fold — deterministic, but NOT byte-equal to a direct aggregation, which is | ||
| #: why ``packed`` is its own class rather than an ``exact`` method. | ||
| COMPOSITION_LAW = "composition_kway" |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] _field_provenance still defaults an absent method to TDIGEST_LAW, so a packed entry without method gets stamped into the artifact as tdigest_kway.
_field_provenance (sweep_overview.py:1408-ish, unchanged by this PR) is:
entry = {"class": meta["class"], "method": meta.get("method", TDIGEST_LAW)}Verified on this head:
>>> _field_provenance({"class": "packed"})
{'class': 'packed', 'method': 'tdigest_kway'}
declared_fields always writes method today, so this is unreachable from zagg's own template path — but _field_provenance consumes a manifest entry (_write_overview's zagg_overview.fields, and _column_provenance → zagg_column.fields), and manifests outlive the writer that produced them and can come from external writers (moczarr is meant to decode from the spec alone, per #340). The failure is the exact class of dishonesty this PR exists to remove: an overview whose own provenance attrs claim a t-digest law over a dense uint64 word.
Suggested fix: key the default off the class now that there is more than one non-exact law, e.g.
default = COMPOSITION_LAW if meta["class"] == "packed" else TDIGEST_LAW
entry = {"class": meta["class"], "method": meta.get("method", default)}There was a problem hiding this comment.
🤖 from Claude
Folded in fa41e59 — the default is keyed off the class, exactly as suggested:
default = COMPOSITION_LAW if meta["class"] == "packed" else TDIGEST_LAW
entry = {"class": meta["class"], "method": meta.get("method", default)}Docstring says why in the terms the finding uses: declared_fields always writes method, but _field_provenance consumes a MANIFEST, and manifests outlive their writer and may come from an external one (§4.5) — so a packed entry must not have the artifact's own provenance claim a t-digest law over a dense composition word.
Test: test_provenance_defaults_the_method_by_class pins {"class": "packed"} → composition_kway, the approximate default unchanged, and an explicit method still winning over the class default.
| f"{sib_element.get('shape')!r}; the channel is one flat uint64 " | ||
| f"word per centroid row (spec §8.3/§9, §1.1)" | ||
| ) | ||
| elif meta["class"] == "packed": |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] The packed drift arm checks dtype and the of binding but not the stored spec/lanes — the two things that decide whether the words can be merged at all.
Compare the approximate arm directly above: it strict-checks the stored ragged element dtype and inner shape, the §2.0 weights declaration, every companion binding, and the sibling's element dtype/shape — precisely because declare_pyramid must not install a declaration the store contradicts. The packed arm stops at dtype + of.
merge_composition_kway unpacks byte lane i as LANES[i] unconditionally. A leaf whose attrs.composition carries a different spec (a future zagg-composition/2) or a permuted/truncated lanes list would be folded lane-wise under this writer's constants and produce a word that is well-formed and wrong — no error anywhere. Config validation guards this on the write side (config.py:869-883 rejects a disagreeing spec/lanes), which is exactly the argument for guarding it here on the read side too: the whole point of _field_drift is the store that a later declaration does not describe.
Secondary, same block: a leaf array with no composition attrs at all passes silently (stored_of is not None short-circuits), where the approximate arm returns drift when the ragged block is absent.
Suggested fix, on the array this probe already opened:
from zagg.stats.composition import COMPOSITION_ATTR, COMPOSITION_SPEC, LANES
stored = dict(arr.attrs.get(COMPOSITION_ATTR) or {})
if stored.get("spec") not in (None, COMPOSITION_SPEC):
return f"field {name!r}: stored composition spec {stored['spec']!r} != {COMPOSITION_SPEC!r}"
if stored.get("lanes") is not None and list(stored["lanes"]) != list(LANES):
return f"field {name!r}: stored composition lanes are not the {COMPOSITION_SPEC} order"(and use COMPOSITION_ATTR rather than the literal "composition" on line 1041, for the same single-source reason the module states in its own docstring).
There was a problem hiding this comment.
🤖 from Claude
Folded in 86c973d — the packed arm now checks the stored block, and both halves of the finding landed.
stored = dict(arr.attrs.get(COMPOSITION_ATTR) or {})
if not stored:
return f"field {name!r}: declared packed but the stored array carries no {COMPOSITION_ATTR!r} attrs block (spec §3.3)"
if stored.get("spec") != COMPOSITION_SPEC:
... # "this zagg folds 'zagg-composition/1' only (lanes are merged by position)"
if [str(lane) for lane in stored.get("lanes") or ()] != list(LANES):
... # "are not the zagg-composition/1 order (spec §3.1)"Notes on the two places I tightened past the suggestion:
specis compared strictly (!= COMPOSITION_SPEC), notnot in (None, COMPOSITION_SPEC)— the absent case is already refused one line above by the no-block check, so a present block with nospecis a malformed block rather than a pre-declaration store. This mirrors theapproximatearm, which refuses a ragged block whosespecis notRAGGED_SPECoutright;laneslikewise: the block is written bycomposition_attrs_block, which always stamps both keys, so a present-but-lane-less block is not a compatibility case.
COMPOSITION_ATTR replaces the literal "composition" on the of read, per the same single-source point. Test: test_packed_drift_checks_the_stored_composition_block builds the synthetic strata leaf, asserts the clean store is not drift, then mutates the block four ways (foreign spec, reversed lanes, rebound of, deleted block) and asserts each reports.
| # writer's constants (``grids.base.apply_field_attrs``), while the | ||
| # per-product ``of``/``threshold`` halves ride the manifest entry, | ||
| # which is the only description the overview writer has. | ||
| block = {"of": meta.get("of")} |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] A packed entry with no of stamps "of": null into every overview level's §3.3 attrs block, and the sweep then publishes an all-fill composition array — while fold_column raises on the same declaration.
Verified on this head:
>>> _overview_config({"composition": {"class": "packed", "dtype": "uint64", "fill_value": 0}})
... 'attrs': {'composition': {'of': None}}
declared_fields never writes that (the classifier requires a truthy of), so this is only reachable from a hand-edited or foreign manifest — but the postures then diverge three ways for one malformed entry:
sweep_overviews' filter (sweep_overview.py:1165) admits it onclassalone → template stampsof: null→_fold_node/_fold_childhitgroup[None]→TypeError→continue(debug only) → every level publishes a fill-0 composition array under an invalid §3.3 declaration. Per §3, fill0means "empty signal stratum", so the artifact positively asserts no flags anywhere rather than recording absence;fold_column(column.py:284-289) raises by name;_merge_slabs(sweep_stage.py:681-693) readsreader.read(res_src, None), whose behavior depends on the reader.
Suggested fix: make the entry filters carry the same requirement the classifier does — m.get("class") != "packed" or m.get("of") in sweep_overviews, composable_fields, and sweep_stage_pass — so a linkage-less packed entry is treated as non-composable (§4.5's stated unknown/unusable-declaration posture) instead of publishing a null-of array. At minimum, omit the attrs block here when of is None rather than writing null into the store.
There was a problem hiding this comment.
🤖 from Claude
Folded in 9651e97 — took the first option (make the filters carry the classifier's requirement) rather than the "omit the attrs block" fallback: a level that publishes a composition array with no §3.3 declaration is a second malformed artifact, not a fix.
The requirement is now one predicate, shared:
def _is_composable(meta: dict) -> bool:
cls = meta.get("class")
if cls == "packed":
return bool(meta.get("of"))
return cls in ("exact", "approximate")column.composable_fields and sweep_overview.sweep_overviews' own filter both go through it, which also closes the third site named in the finding: sweep_stage_pass filters through composable_fields (src/zagg/sweep_stage.py:955) before any of _gather_slabs/_merge_slabs/_write_stage_column sees fields, so the stage path inherits the same admission rather than needing its own copy. A linkage-less packed entry now degrades to native resolution exactly as an unknown class does, and fold_column's by-name raise stays as the last line for a caller that hands it an unfiltered map.
Test: test_a_packed_entry_without_its_of_linkage_is_not_composable strips of from the shipped strata declaration and asserts it drops out of composable_fields while the intact entry passes.
| if (words is None) != (of_values is None): | ||
| logger.warning( | ||
| f"stage sweep: column {reader.path} carries only one of " | ||
| f"{name!r}/{of_name!r} at resolution {res_src}; counting the " |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] Skipping a half-pair contributor here does not keep the pair consistent: the of digest folds that contributor anyway, so the published word's fractions are over a strict subset of the N_signal a reader will divide by.
This is not the located-pair situation the comment cites. There, both halves are the same field's payload+sibling, so refusing one refuses both and the output stays self-consistent. Here the divisor is a different declared field, folded independently in the approximate loop above (lines 597-664), which has its own _companion_group gate and knows nothing about this one. Reproduced on this head (4 children, factor=4, child 1 carrying h_sig but not composition):
half-pair comp: [372] counts folded=3 missing=0 unreadable=1
h_sig weight at the output cell: 18.0 # 3+4+5+6 — all four children
The word's lanes are Σ nᵢ·laneᵢ / 14; the level's N_signal is 18. A reader doing the §3.3 recovery counts_from_composition(word, N_signal) — which §3.4's new text explicitly licenses ("its N_signal at every level is the folded of digest's total weight at that level") — is off by 18/14, ~29% here. Presence stays exact, so nothing looks wrong. source_children.unreadable records that the artifact folded short, but it is per-child, not per-field, so it cannot tell a reader which denominator is the stale one.
The same skew arises in _fold_node (a leaf missing the composition array but carrying the digest: the packed arm never records it, the digest arm folds it) and in the /2 gather the PR body flags under "Questions for review" — but the gather already has this posture available: _gather_slabs pair-checks companions and does broken.add((i, 0)) + skip |= {...} for both halves (sweep_stage.py:509-516), so "tightening the gather would break its gen-1-untouched contract" understates what the gather already does.
Suggested fix (either is fine, but one of them should land): (a) make the skew normative in spec §3.4/§4.5 — a level's composition fractions are over the contributors that folded, which may be a subset of the of digest's contributors whenever source_children.unreadable/missing is nonzero, so N_signal recovery is exact only on a fully-covered level; or (b) record the folded weight beside the word (per output cell, Σ n of the parts actually merged) so the pair is self-describing. (a) is the cheap one and matches the PR's honesty framing.
There was a problem hiding this comment.
🤖 from Claude
Folded in 69a8e75. Neither (a) nor (b) — a third option, which I think is stronger than making the skew normative: poison the affected output cells. Absence over wrongness. A cell whose word would be over a strict subset of the level's N_signal keeps the fill word 0, which makes no §3.2 presence/fraction claim, instead of publishing a plausible word that a §3.3 recovery divides wrongly. The of digest is untouched and stays correct on its own, and source_children.unreadable still records that the level folded short.
_merge_slabs (src/zagg/sweep_stage.py):
poisoned: set[int] = set()
...
broken.add((i, w))
poisoned.update(range(base // factor, (base + src_per_child + factor - 1) // factor))
...
for j, parts in parts_by_cell.items():
if j not in poisoned:
out[j] = merge_composition_kway(parts)Poisoning drops cells AFTER accumulation, so every surviving cell still folds its parts in one k-way call — single quantization is preserved. The rule is keyed on the PAIR rather than on the skew direction, because a contributor's windows all land on the same output cells: one window's half-pair would otherwise mix into cells another window filled.
_fold_node gets the same treatment (packed_poison, per field, applied only once the leaf has read cleanly — an unreadable leaf is skipped whole, so the digest excludes it too and nothing needs poisoning). Its two half-pair paths are the word-absent one (poisoned only when the of array IS present — a leaf carrying neither half is ordinary schema evolution, nothing folds from it either way) and the of-absent one.
On _fold_child: verified, no change needed, and the invariant is now stated in the code. _cascade_node ASSIGNS each child's partials into a disjoint span (slabs[name][start : start + span] = partial, sweep_overview.py:1884-1886), so a child skipped for the field leaves its whole span at the fill word — no output cell can mix that child's absence with a sibling's contribution. That is the poisoned posture already.
Tests (both fail without the poisoning, checked by reverting the guard):
TestStageMergeHalfPair::test_a_half_paired_contributor_poisons_its_output_cells— two real written columns merged into ONE output cell (src_per_child=4,factor=8,n_out=1), control asserts the word is the k-way merge of all eight source cells; then one column loses itscompositionmember and the assertions are(folded, missing, unreadable) == (1, 0, 1),composition[0] == 0, andh_sig's weight at that cell UNCHANGED — the divisor still folds both, which is precisely why the word may not be published;TestLeafFoldHalfPair::test_a_half_paired_leaf_poisons_its_output_cells— the same shape at_fold_node, at the geometry where two leaves land in one output cell (shard order 3, cell order 4, k=1 ⇒target_order < shard_order).
| for w, reader in enumerate(row or ()): | ||
| if not _is_reader(reader): | ||
| continue | ||
| words = reader.read(res_src, name) |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] words here shadows the digest loop's words dict, which is a free variable of the _close closure defined at line 618.
_close (line 618) reads pending, words and sibling_slabs from the enclosing function scope at call time, not from its default args. Today that is harmless — every _close call happens inside the approximate loop, which finishes before this loop starts — so this is latent, not a live bug. But the two bindings are 65 lines apart in one function and have incompatible types (dict[str, dict[int, list]] vs a numpy array of uint64), so any future change that defers a close, hoists the packed loop, or merges the two passes turns it into a TypeError at best and a wrong-cell write at worst. Nothing in the file's comments warns the next editor.
Suggested fix: rename this local (and its uses on lines 686/699) to something the digest loop does not own, e.g. word_values, matching of_values right beside it.
There was a problem hiding this comment.
🤖 from Claude
Folded in ac42a24 — the packed loop's local is word_slab now (the read, the is None test, and both uses in the accumulation), leaving words to the digest loop's _close closure. Went with word_slab over word_values because the value is a whole cell slab, not a per-cell vector like of_values.
Comment at the binding says why, so the next editor does not reintroduce it:
# Not ``words``: that name is a free variable of the digest loop's ``_close``
# closure 65 lines up, with an incompatible type — harmless today (every close
# happens before this loop starts) but a latent wrong-cell write.| - **`fields`** — every aggregation field, keyed by name, with its | ||
| **composability class**: `exact` (folds byte-equal — count/sum/min/max), | ||
| `approximate` (t-digest merge — `np.isclose` equality class), or `none` | ||
| `approximate` (t-digest merge — `np.isclose` equality class), `packed` |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] The class list gained packed but the normative sentence that says what an entry carries was left two-class, so §4.5 now enumerates a class it never gives an entry grammar for in that paragraph.
Nine lines below (unchanged, docs/specification.md:947):
exact/approximateentries carry the foldmethod, any further fold provenance (anexactfold'snan_policy), and enough dtype/shape metadata to know the overview array's form up front.
A packed entry carries all of that (method, dtype, fill_value — verified against declared_fields on the shipped strata template), and the dedicated bullet added further down at line 1003 says so. But an external reader working the paragraph top-down — the moczarr posture #340 exists for — reads a MUST-shaped enumeration that excludes packed, then a bullet that contradicts it. The none sentence immediately above ("A none entry carries class only") is what makes this an exhaustive-looking partition rather than a list of examples.
Suggested fix: exact/approximate/packed entries carry the fold method, … — one token, and the paragraph stops disagreeing with line 1003.
There was a problem hiding this comment.
🤖 from Claude
Folded in e022105 — one token, as suggested:
exact/approximate/packedentries carry the foldmethod, any further fold provenance (anexactfold'snan_policy), and enough dtype/shape metadata to know the overview array's form up front.
The paragraph now agrees with the dedicated packed bullet further down and with what declared_fields writes (method, dtype, fill_value, plus of and the optional threshold).
| with each contributor's `n` taken from its `of` digest's weight at the same | ||
| cell. An overview's composition array carries the same §3.3 attrs block a | ||
| leaf does, and its `N_signal` at every level is the folded `of` digest's | ||
| total weight at that level (digest weights fold exactly, §2). One |
There was a problem hiding this comment.
🤖 from Claude (review)
[question] "digest weights fold exactly, §2" is stated without the caveat §4.6 already carries, and packed is the one class where that caveat is load-bearing.
docs/specification.md:754-756 (unchanged, §4.6) says the opposite side of the same fact:
…
2^24observations, and theresum(weights)is the nearest float32 to the true count rather than the count itself
payload_weight (sweep_overview.py:226) is int(round(float(decode_digest(...)[:, 1].sum()))) — a float32 accumulation, rounded to an int, used as the divisor at every fold site. So the new sentence promises exactness for the very quantity §4.6 documents as inexact past 2^24, and for the CA store (o9 leaves, ATL03 photon counts) coarse levels are squarely past it. The practical error is tiny (relative ~1e-7, far below one lane step of 1/255), which is why I am filing this as a question rather than a defect — but this PR's premise is that a declaration must not claim more than the code delivers, and this is the same shape of claim.
Suggested fix, if you agree: scope it the way §4.6 does — "(digest weights fold exactly up to the §4.6 float32 bound)" — or drop the parenthetical and let §4.6 be the single statement. If instead the intent is that N_signal recovery is only ever accurate to one lane quantization anyway (so float32 drift is provably immaterial), saying that here would be stronger than the current absolute.
There was a problem hiding this comment.
🤖 from Claude
Agreed, and folded in 05609a0 as wording only. Scoped it the way §4.6 does, and said which way the residual cuts rather than leaving the reader to weigh it:
… its
N_signalat every level is the foldedofdigest's total weight at that level — exact up to §2.1's float32 bound: past2^24pooled observations that sum is the nearest float32 to the true count (§4.6), and the fold's divisor rounds that float to an integer. The residual is relative ~1e-7, orders below one lane step of 1/255, so it never moves a published lane; it is stated because a reader recovering counts fromN_signalis entitled to the same bound §2.1 gives the digest itself.
Both halves of the suggestion, in effect: the claim is bounded rather than absolute, AND the "provably immaterial against one lane quantization" argument is on the page instead of left implicit. No code change — payload_weight is unchanged.
| from the folded strata matches leaf truth exactly (digest weights fold | ||
| exactly, spec §2).""" | ||
|
|
||
| def _fold(self, per_cell, fields=_STRATA_FIELDS, cell_order=5, resolution=4): |
There was a problem hiding this comment.
🤖 from Claude (review)
[question] Every packed assertion in this class goes through fold_column; the other three fold sites the commit wires are unexercised. Which phase covers them?
self._fold (line 431) calls zagg.column.fold_column only. So phase 3 ships four packed fold implementations and tests one:
| site | tested here | covered by phase 4 as scoped in the PR body? |
|---|---|---|
column.fold_column |
yes | — |
sweep_overview._fold_node (leaf → overview, start + i // fold_factor) |
no | yes ("template → build → overview fold") |
sweep_overview._fold_child (cascade, j*factor window) |
no | implied, not stated |
sweep_stage._merge_slabs (stage merge, (base + pos) // factor, windows folding across) |
no | not mentioned at all |
The three untested sites are where the indexing is easiest to get subtly wrong, and a wrong-but-plausible word is the failure mode this class exists to catch — fold_column's loop is the one whose index arithmetic is trivially [j*factor, (j+1)*factor) over a single flat slab. I did drive _fold_child and _merge_slabs by hand against this head and both reproduce merge_composition_kway over the right contributors (including the windows-fold-across path), so I have no defect to report — but nothing in the suite pins that, and _merge_slabs in particular has behavior no other site has (the half-pair broken classification, lines 686-694).
Concretely: is _merge_slabs in phase 4's scope, or should the phase-4 checklist item be widened to name it? If it is out of scope for this PR, a one-line note on the phase-4 entry saying so would keep the gap deliberate rather than accidental.
There was a problem hiding this comment.
🤖 from Claude
Answering the question directly: _fold_node and _fold_child were phase 4's scope, _merge_slabs was not — and it is covered now, so all four sites are pinned.
_fold_nodeand_fold_child: phase 4 (a73b64b), pushed after the snapshot this review saw.tests/test_strata_composability.py::TestEndToEndStrataPyramid::test_strata_and_composition_fold_at_every_levelbuilds a synthetic strata store and sweeps it under the/1cascade regime, so level 1 is exact-from-leaves (_fold_node,start + i // fold_factor) and level 0 is a cascade (_cascade_node→_fold_child, thej*factorwindow). At each level the word is recomputed independently from the arrays one level finer, and presence is checked against a direct pooled pack of the raw rows.test_overview_provenance_records_the_packed_lawpins the per-levelzagg_overview.fieldsentry for both regimes;_merge_slabs: nowTestStageMergeHalfPair::test_a_half_paired_contributor_poisons_its_output_cells(69a8e75), which drives it over two real written columns with an output cell spanning both children — including the half-pairbrokenclassification this site alone has. That commit also changes the behavior there (see the sibling thread on line 690): the half-pair no longer just skips, it keeps the covered cells at the fill word;_fold_node's multi-contributor geometry (target_order < shard_order, several leaves into one output cell) gets its own test in the same commit,TestLeafFoldHalfPair— the end-to-end fixture's geometry cannot reach it.
| declaration's ``fields`` map straight through can neither refuse a leaf | ||
| over a non-cell-extent vector slab nor materialize an all-empty ragged | ||
| group for a companion the column has no business carrying. | ||
| group for a companion the column has no business carrying. The ``packed`` |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] composable_fields' own docstring was de-dualized here ("the two composable classes" → "the composable classes"), but its caller's docstring, which reasons from this filter, was not.
leaf_slabs at column.py:183:
fieldsis filtered to the composable classes first (:func:composable_fields), which is what makes the(n_cells,)extent check sound: those two classes admit nothing but cell-resolution scalars and ragged payloads …
Three classes now, and the sentence is the load-bearing justification for the raise at line 191 — a reader checking whether the extent check still holds is sent to count a set that has changed size. The argument itself survives (a packed word is a cell-resolution dense scalar, one row per cell), which is exactly why it should be restated rather than left to be re-derived.
Suggested fix: "those classes admit nothing but cell-resolution scalars (including the §3 composition word) and ragged payloads".
There was a problem hiding this comment.
🤖 from Claude
Folded in 705591d, restated rather than left to be re-derived, as suggested:
fieldsis filtered to the composable classes first (composable_fields), which is what makes the(n_cells,)extent check sound: those classes admit nothing but cell-resolution scalars (including the §3 composition word) and ragged payloads, both of which are one row per cell …
Kept the argument's structure intact — it is still the justification for the raise below — with the packed word named as the case that would otherwise have to be checked against a class list.
| The distinction is only material for the `approximate` class: the exact | ||
| merge laws are associative, so a cascaded `sum`/`min`/`max` is the same | ||
| value either way, while a cascaded digest is a **merge of merges** — it | ||
| The distinction is material for the `approximate` and `packed` classes: the |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] §4.4 gained the packed cascade caveat but docs/hive_layout.md still states the two-class version of the same fact.
docs/hive_layout.md:266-268 (not touched by this PR):
The cost is precision in the
approximateclass — a cascaded t-digest is a merge of merges — which is why each level records the regime that made it …
That is now incomplete in exactly the way this line fixes: a cascaded composition word re-quantizes once per level too, so fold_source/exact_levels is a packed reader's concern as much as an approximate one. hive_layout.md is the narrative page a reader hits before the spec, and it also introduces pyramid as carrying "each field's D24 composability class (issue #201)" at line 253 — a sentence written when there were three tokens and now describing four.
Since this is the same wire-format change §4.4 is documenting, both should move together (§4 of CLAUDE.md pairs a spec change with its narrative docs). Suggested fix: "…precision in the approximate and packed classes — a cascaded t-digest is a merge of merges, and a cascaded composition word re-quantizes once per level (spec §3.4) —".
There was a problem hiding this comment.
🤖 from Claude
Folded in 9c08b9a — both hive_layout.md sentences, moving with the §4.4 change as §4 of CLAUDE.md asks:
- the cascade cost (line ~266): "The cost is precision in the
approximateandpackedclasses — a cascaded t-digest is a merge of merges, and a cascaded composition word re-quantizes once per level (specification §3.4) — which is why each level records the regime that made it …"; - the
pyramidintroduction (line ~253), which was the other three-token sentence: it now enumerates the class rather than gesturing at it — "each field's D24 composability class —exact,approximate,packed(the §3 composition word, issue D24: admit build_tdigest_where to the digest family; give composition an honest composability declaration #515) ornone".
Rewrapped the touched paragraphs to the page's width; no other content moved.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
…515) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
| n: dict(m) | ||
| for n, m in (decl.get("fields") or {}).items() | ||
| if isinstance(m, dict) and m.get("class") in ("exact", "approximate") | ||
| if isinstance(m, dict) and m.get("class") in ("exact", "approximate", "packed") |
There was a problem hiding this comment.
🤖 from Claude (review)
[blocking] sweep_stage_pass is a FOURTH admission site and it still carries the literal filter — _is_composable (9651e97) never reached it.
The fold reply on the _is_composable thread says the stage path "inherits the same admission" via composable_fields at src/zagg/sweep_stage.py:955. That call is inside _write_stage_column (the column WRITER). sweep_stage_pass lives in this file and builds its own fields map right here, and that map goes to stage_node unfiltered (line 216) -> _stage_fold -> _merge_slabs/_gather_slabs. So a packed entry with no of is admitted to the fold at the stage path.
The consequence is worse than the "degrade to native resolution" _is_composable was added to guarantee. In _merge_slabs' packed arm of_name is then None, so reader.read(res_src, None) reaches group[None], which zarr 3.3.0 raises TypeError on (verified locally), and _ColumnReader.read catches only KeyError/FileNotFoundError/GroupNotFoundError (src/zagg/sweep_stage.py:334). Nothing between there and here wraps it — stage_node calls _stage_fold bare (sweep_stage.py:1186) and sweep_stage_pass calls stage_node bare (line 210) — so the whole staged pass aborts. _fold_node and _fold_child guard exactly this case on purpose (except (KeyError, TypeError)), which is what makes the gap here visible.
Fix, one line plus the import, mirroring sweep_overviews:
from zagg.column import _is_composable
fields = {
n: dict(m)
for n, m in (decl.get("fields") or {}).items()
if isinstance(m, dict) and _is_composable(m)
}and extend test_a_packed_entry_without_its_of_linkage_is_not_composable to cover this entry point too, so "all sites agree" is pinned rather than asserted.
There was a problem hiding this comment.
🤖 from Claude
Folded in 484fd36. sweep_stage_pass's literal filter now routes through zagg.column._is_composable, exactly as sweep_overviews does — all four admission sites are one predicate. The comment at the site names why the literal was worse here than elsewhere: this map goes to stage_node unfiltered, so an of-less packed entry reaches _merge_slabs and reads group[None], a TypeError _ColumnReader.read does not catch.
Behavioral test rather than an import assertion: TestD24Classification::test_the_stage_pass_drops_a_packed_entry_without_its_of_linkage runs sweep_stage_pass twice on the same manifest shape, once with the of-less entry and once with the linkage restored. The fields gate sits before _candidate_decimals, so root_moc_stale in the summary is the "got past the gate" witness: absent for the broken entry, True for the whole one. Mutation-checked — the test fails with the literal filter restored.
| f"sweep[overview]: leaf {leaf} lacks {of_name!r} for " | ||
| f"packed field {name!r}" | ||
| ) | ||
| leaf_poison.add(name) |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] This half of the poison trigger fires in the direction where no skew is possible, and it deletes other leaves' correct words.
The sibling trigger at line 1681 is gated exactly right — if meta["class"] == "packed" and (meta.get("of") or "") in group — because that is the skew case: the word is gone, the divisor digest is present and the digest arm folds it, so the level's N_signal counts rows the surviving word never covered.
This one is the reverse: group[of_name] is absent. The digest arm hits the same KeyError for the of field at line 1673-1676 and skips it too, so the leaf contributes to NEITHER the word nor N_signal — the pair is already consistent. Poisoning here only discards what other leaves contributed to the same output cell. Reproduced on this branch, with TestLeafFoldHalfPair's own fixture but dropping leaf -3112's h_sig instead of its composition:
counts["failed"] = 0
weight(h_sig, 0) = 80 # == leaf -3111 alone; the divisor already excludes -3112
composition[0] = 0 # a consistent -3111-only word exists: 5999754895727340585
Both poison tests in 69a8e75 exercise only the word-missing direction, so nothing catches this. Suggested fix: drop leaf_poison.add(name) on this branch (keep the continue) and record in the comment why the two directions differ. If the pair-keying is deliberate, add the reverse direction to TestLeafFoldHalfPair so the loss is a pinned decision instead of a side effect.
There was a problem hiding this comment.
🤖 from Claude
Right, and folded in e5bcf12 — the trigger direction was backwards on this half. leaf_poison.add(name) is gone from the packed arm: when the of array is absent the digest arm reading that same array for the of field itself hits the same KeyError in the same leaf, so both halves drop together and the folded divisor already excludes the leaf. Nothing to skew, and poisoning destroyed sibling leaves' correct words. The comment now says so at the site, and the surviving trigger at the except KeyError branch is labelled the ONE skew direction (word absent, divisor PRESENT).
Both directions are now pinned by their own test, and each mutation-checked against its own half of the fix:
TestLeafFoldHalfPair::test_a_half_paired_leaf_poisons_its_output_cells(unchanged) fails when the skew-direction poison is disabled.TestLeafFoldHalfPair::test_a_leaf_missing_the_divisor_drops_without_poisoning(new) drops leaf-3112'sh_sigand asserts the cell keeps-3111's word and-3111-alone weights; it fails whenleaf_poison.add(name)is put back on the divisor-absent branch.
| # UNREADABLE leaf is skipped whole, so the digest excludes it too | ||
| # and the pair stays consistent without poisoning anything. | ||
| for name in leaf_poison: | ||
| packed_poison[name].update(range(start, start + span)) |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] Poisoning at this site leaves no trace a reader can see — _fold_node returns no source_children.
The reply on the stage thread says "source_children.unreadable still records that the level folded short". That holds at _merge_slabs (broken.add((i, w)) + logger.warning). It does not hold here: _fold_node's return (lines 1807-1817) carries generation/content_hash/granule_count/time_range/fold_source and no source_children; a poisoned leaf still does n_leaves += 1; and both triggers log at logger.debug. TestLeafFoldHalfPair asserts counts["failed"] == 0 — so a poisoned cell is byte-indistinguishable from genuine emptiness, which is precisely the situation _cascade_node's own comment at line 1953 refuses to accept ("a log line in an exited process cannot tell a reader that a fill cell is under-coverage rather than emptiness").
Not a correctness bug — the fill word makes no §3.2 claim either way — but "absence over wrongness" only stands up if the absence is recoverable. Suggested fix: raise the two poison-path logger.debug calls to logger.warning (matching _merge_slabs), and/or carry a poisoned-cell count into the fold dict so _overview_provenance can record it; at minimum correct the thread's claim, which currently overstates what the leaf fold traces.
There was a problem hiding this comment.
🤖 from Claude
Correcting the earlier claim: you are right that source_children.unreadable is _merge_slabs' trace, not _fold_node's — _fold_node returns no source_children, a poisoned leaf still counts in n_leaves, and the log was debug. The stage-thread reply overstated what the leaf fold records.
Folded minimally in 62b3bb7. There is now only ONE poison site in _fold_node (e5bcf12 removed the other), and it logs at logger.warning naming the node, the field, the leaf, and the poisoned span:
sweep[overview]: node {node} field {name!r}: leaf {leaf} carries {of!r} but not the
word; output cells [{start}, {start + span}) keep the fill word (spec §3.3)
with a comment saying plainly that this line is the only trace, since the fill word 0 is byte-indistinguishable from genuine emptiness.
Left standing deliberately: the artifact-visible record. A source_children analog for the /1 leaf fold is a NEW attrs key on the overview group — spec §4.3 grammar plus a conformance-fixture regeneration under the issue #340 rule — which is a scope change, not a review fold. Leaving it for espg to rule on rather than inventing the key here.
| f"contributor unreadable (spec §3.3, §1.1)" | ||
| ) | ||
| broken.add((i, w)) | ||
| poisoned.update( |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] The range arithmetic is exact; the trigger above it is not.
Span check first: the contributor covers source rows base .. base + src_per_child - 1, i.e. output cells base // factor .. (base + src_per_child - 1) // factor. (base + src_per_child + factor - 1) // factor == floor((base + src_per_child - 1) / factor) + 1, the exclusive end — no over- or under-poisoning of neighbours, at factor == 1 too.
The trigger is (word_slab is None) != (of_values is None), i.e. both directions, but only one skews. When the DIVISOR is the missing half, the digest loop above drops the same contributor as well (_companion_group / slab is None), so word and N_signal are already consistent and poisoning just deletes the other children's correct words. Reproduced with TestStageMergeHalfPair's fixture, removing child 1112's h_sig instead of its composition:
(folded, missing, unreadable) = (1, 0, 1)
composition[0] = 0 # a consistent 1111-only word exists: 6007324956125508149
The blast radius also runs wider than the comment's windows argument admits: factor = 4 ** (res_src - r) and src_per_child = 4 ** (res_src - child_order) (sweep_stage.py:787-788), so at every level with r < child_order — the ordinary coarse level, and the shape this test itself uses (factor=8, src_per_child=4) — one output cell is shared by factor / src_per_child different children, and one half-paired child blanks the cell for all of them.
Suggested fix: poison only when of_values is not None and word_slab is None, keeping broken.add((i, w)) for both directions. If pair-keying is deliberate, say in the comment that a poisoned cell drops SIBLING contributors and not merely other windows of the same one.
There was a problem hiding this comment.
🤖 from Claude
Folded the direction half in f9d799d; the shared-cell blanking stays by design.
Direction: the poison is now gated if of_values is not None, so it fires only in the skew direction (word missing, divisor present). broken.add((i, w)) still fires for BOTH directions — the level did fold short either way. The comment now says why the reverse direction is safe: the digest loop above reads that same array for the of field itself and drops the contributor too (slab is None), so word and N_signal already exclude the same rows.
Blast radius: left by design, and now stated in the loop comment rather than left to the windows argument. At every level with r < child_order one output cell is shared by factor / src_per_child children, so poisoning it does drop sibling contributions — but a shared cell whose folded N_signal counts rows no surviving word describes cannot carry an honest word. Blanking beats skewing; the fill 0 makes no §3.2 claim, the skewed word would.
Tests, both mutation-checked against their own half:
TestStageMergeHalfPair::test_a_half_paired_contributor_poisons_its_output_cells(unchanged, still the skew direction) fails when the poison is disabled outright.TestStageMergeHalfPair::test_a_contributor_missing_the_divisor_drops_without_poisoning(new) removes child1112'sh_sig, asserts(folded, missing, unreadable) == (1, 0, 1)and that the shared cell keeps1111's word and1111-alone weights; it fails when the poison is restored to both directions.
| for i in range(4) | ||
| if weights(g1, "h_sig", i) > 0 | ||
| ] | ||
| assert int(g0["composition"][whole]) == merge_composition_kway(parts) |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] This assertion cannot catch a misaligned cascade window.
_fold_child's packed branch slices range(j * factor, min((j + 1) * factor, len(words_arr))) (src/zagg/sweep_overview.py:2036). The fixture has ONE leaf, so of level 1's 16 cells only 0..3 carry data and all four land in output cell 0; every other source cell has n == 0 and is dropped by the n > 0 guard. A window that over-reaches into the next child's cells is therefore invisible. Verified by mutation on this branch:
_fold_child: min((j + 1) * factor, ...) -> min((j + 1) * factor + 1, ...)
pytest tests/test_strata_composability.py tests/test_sweep_overview.py tests/test_sweep_stage.py
307 passed
The rest of the geometry IS well pinned — range(j * factor + 1, ...) (a shift) fails, a 1-for-n divisor fails at both _fold_node and _fold_child, and start + (i + 1) // fold_factor on the leaf fold fails — so this is the one hole, and it is the boundary the level-0 claim rests on.
Suggested fix: populate a second, non-adjacent output cell at level 1 (a second committed leaf under a different rank of -31 in _build_store), then assert level 0's word at BOTH cells and fill in between. That makes the j * factor boundary load-bearing rather than incidental.
There was a problem hiding this comment.
🤖 from Claude
Folded in 0c105ad — the hole was real and the mutation now dies.
_build_store takes an optional second leaf and the fold test commits two: -311 (rank 0) and -312 (rank 1) under node -31, each with its own _strata_cells batch. Level 1 is populated at cells 0..7 (two four-cell spans), level 0 at cells 0 AND 1 — so the j * factor boundary now separates two POPULATED spans instead of a span and empty fill that the n > 0 guard swallows.
The level-1 and level-0 assertions loop over both leaves in the existing arithmetic style: level 1 at 4 * rank + j, level 0 at rank, each level-0 cell's word recomputed from its own four level-1 cells and its presence compared against a per-leaf pooled pack_composition_n of that leaf's raw rows. The composition[15] == 0 fill assertion and the §3.3 attrs-block check are unchanged.
Mutation verified: min((j + 1) * factor, ...) -> min((j + 1) * factor + 1, ...) in _fold_child now fails test_strata_and_composition_fold_at_every_level (it passed 309 tests before). Reverted after checking.
| """The synthetic strata leaf config: the CA shape at test scale.""" | ||
| from zagg.config import PipelineConfig | ||
|
|
||
| where = "(conf_land >= 2)" |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] The fixture config declares a strata cut that does not match its own composition threshold, so what it pins is a config that would break the §3.3 divisor invariant if it were ever actually built.
where = "(conf_land >= 2)" makes h_sig a LAND-only digest, while composition packs threshold: 2 across all five §3.1 surfaces and _strata_cells (the bytes actually written) uses signal = (conf >= 2).any(axis=1). The identity the packed class rests on — weight(of) == N_signal, spec §3.3 — therefore holds here only because _build_store writes hand-built arrays and the declared reducers never run. A real build from this config would give weight(h_sig) < N_signal at every cell and silently wrong recovered counts at every level.
That is exactly the coupling zagg.config deliberately does not validate ("The third coupling — the digest fields' where predicates committing the same cut as threshold — is not validated here", src/zagg/config.py:856-862), documented there as a silent-wrong-answer class. A fixture whose docstring calls it "the CA shape at test scale" is what gets copied into the next product template.
Suggested fix: make where the cut the word packs, e.g. "(conf_land >= 2) | (conf_ocean >= 2) | (conf_sea_ice >= 2) | (conf_land_ice >= 2) | (conf_inland_water >= 2)". Separately worth noting for the PR body: phase 4 is template -> hand-written slabs -> sweep, not template -> build -> sweep — nothing here drives the reducers, so the config/data agreement is unpinned either way.
There was a problem hiding this comment.
🤖 from Claude
Folded in a61847c. _strata_leaf_cfg's where is now the five-column union the word actually packs:
((conf_land >= 2) | (conf_ocean >= 2) | (conf_sea_ice >= 2)
| (conf_land_ice >= 2) | (conf_inland_water >= 2))
matching _strata_cells' signal = (conf >= 2).any(axis=1) and pack_composition_n's threshold: 2. Wrapped in outer parens so h_noise's f"~{where}" negates the whole union rather than binding ~ to the first term — _resolve_param evals it as a Python/numpy expression, where ~ binds tighter than |.
The comment above it says the predicate is never executed here (_write_strata_leaf writes the slabs by hand, so no reducer runs) but must still commit the SAME cut the words were packed at, points at zagg.config's documented non-validation of the third coupling, and names the reason: a land-only where gives weight(h_sig) < N_signal at every cell and breaks §3.3's recovery in anything copied from this fixture.
Your second point — phase 4 is template -> hand-written slabs -> sweep, not template -> build -> sweep, so config/data agreement is unpinned either way — is fair and left standing for espg rather than folded: driving the reducers end to end is a scope change, not a fixture fix.
|
|
||
| SHARD_ORDER, CELL_ORDER, K = 3, 4, 1 | ||
|
|
||
| def _leaf(self, root, dec, per_cell): |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] TestLeafFoldHalfPair._leaf and TestEndToEndStrataPyramid._build_store (line 913) are the same ~25 lines twice, added four commits apart in one file: HealpixGrid(..., config=_strata_leaf_cfg()), emit_shard_template, morton from generate_morton_children, count as n_signal + n_noise, the two ragged strata slabs, the composition word column, stamp_commit. They differ only in (SHARD_ORDER, CELL_ORDER) and the manifest write the e2e one additionally does.
Suggest one module-level helper — _write_strata_leaf(root, dec, per_cell, *, shard_order, cell_order) returning the leaf group path — with _build_store keeping only the manifest half. Beyond tidiness: both copies must track _strata_leaf_cfg, so a fix to one geometry (the where point on line 646, say) silently misses the other.
There was a problem hiding this comment.
🤖 from Claude
Folded in fbc2a5a — it fell out cleanly, and your "a fix to one geometry silently misses the other" argument decided it: the very next commit in this fold pass changed _strata_leaf_cfg's where, and the leaf after that added a second leaf to the e2e store.
Module-level _write_strata_leaf(root, dec, per_cell, *, shard_order, cell_order) now owns template / morton / count / the two ragged strata slabs / the packed word / stamp_commit, and returns the cell-order group path. TestLeafFoldHalfPair._leaf is a three-line call; _build_store keeps only its manifest half (and, since 0c105ad, the second-leaf loop). Both call sites and the whole file are green.
| load-bearing exactly as `location` is above: the overview writer | ||
| reconstructs a level's arrays from this entry alone, so without it the fold | ||
| has no divisor and the overview array's §3.3 attrs block could not be | ||
| written. A writer MUST declare a composition field `packed` only when the |
There was a problem hiding this comment.
🤖 from Claude (review)
[non-blocking] This MUST is now weaker than what the writer enforces — 3367b7f added a second condition to the demotion and the spec did not move with it.
declared_fields demotes to {"class": "none"} when the of digest is not approximate or when it declares non-default §2.0 weights (src/zagg/pyramid.py:255: classes.get(of) != "approximate" or of_weights not in (None, "counts")), on the reasoning that N_signal is a photon COUNT so a flux-weighted divisor is the wrong n. §4.5 states only the first half, so an external writer following this page emits packed over a flux digest where zagg would not — and §3.4's "its N_signal at every level is the folded of digest's total weight at that level" quietly assumes counts as well.
Per the #340 rule (external readers decode from spec + fixtures alone), suggest extending the sentence, e.g. "…only when the digest of names is itself declared approximate in the same map and carries the default §2.0 weights: counts — the law divides by that digest's weight at every level and N_signal is a count, so a flux-weighted divisor is a different quantity, and such a composition is declared {"class": "none"} instead." No fixture change needed: kitchen_sink's divisor is a counts digest, so the regenerated fixture already conforms.
There was a problem hiding this comment.
🤖 from Claude
Folded in f3973aa. The §4.5 packed bullet now carries both halves of what declared_fields enforces:
A writer MUST declare a composition field
packedonly when the digestofnames is itself declaredapproximatein the same map and carries the default §2.0weights: counts— the law divides by that digest's weight at every level andN_signalis a photon COUNT, so a composition whose divisor is excluded from the pyramid, or whose divisor is flux-weighted (a different quantity), is declared{"class": "none"}instead.
Wording tracks the demotion warning's own reasoning at src/zagg/pyramid.py. No fixture change needed as you note — kitchen_sink's divisor is a counts digest, and tests/test_spec_conformance.py is green (178 passed).
…515) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
…#515) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
…515) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
…515) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
…515) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
…#515) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
|
🤖 from Claude All 4 phases are complete at Standing items for review are in the PR body's "Questions for review": the artifact-visible poison record (needs a new spec §4.3 attrs key — scope decision), the config-side flux-divisor cross-check, the stage-gather packed-pair posture, and the §4 module-size note. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b
|
🤖 from Claude PR #511 merged, so the branch's single red is resolved: merged |
|
🤖 from Claude Recording espg's rulings (in-session 2026-08-24) on the four standing questions:
No code changes follow from these; the PR merges as-is. |
Closes #515.
Admits
build_tdigest_where(the strata builder) to the D24 digest family, and gives the packedcompositionword an honest composability declaration — per espg's ruling (admit, 2026-08-24) recorded on the issue, and the plan comment (#515 plan).The drift: both reducers are admitted by the spill fold gate (
_TDIGEST_SPILL_FUNCTIONS,merge_composition_kway— issues #370/#321) and the published CA store's 2,726 shards were built by k-way merging per-block strata partials under exactly those laws — yet D24 classified every one of their fields{class: none}. Verified live:atl03_tdigest_o9.zarr/morton_hive.jsondeclaresh_tdigest_signal/h_tdigest_noise/compositionallnone(the manifest vendored in full astests/data/ca_atl03_tdigest_o9_morton_hive.json).Phases
dd22f0ca). Pinned today'snoneclassifications (strata + composition), the spill-gate admissions, and the LIVE CA-manifest declaration as an in-tree known-answer fixture (no network in tests).tests/test_strata_composability.py. Review findings folded infd233463..d758a534.build_tdigest_whereadmission (2218b821). Added to_DIGEST_FAMILY_FUNCTIONS(streaming/spill gate tuples untouched — those re-run builders and keep their exact members); k-way parity tests for the strata payload and both companions (located → common-ancestor words, temporal → toc envelope) throughfold_digests, plus order-independence; the shipped strata template +output.pyramid = {}now declares{class: approximate, method: tdigest_kway, delta: 8192, overview_delta: 512, location: leaf_id}for both strata fields.28ddd8f1). New D24 classpacked(decision below): classifier arm insemantics.field_composability, manifest entry inpyramid.declared_fields(method: composition_kway, dense dtype/fill, the §3.3oflinkage +threshold), and the fold law wired at every stored-payload fold site —sweep_overview._fold_node(leaves),_fold_child(cascade),column.fold_column(leaf column),sweep_stage._merge_slabs(stage merge; gathers assign gen-1 bytes unchanged) — each pairing a contributor's word with itsofdigest's weight at the same cell and collapsing per output cell in ONEmerge_composition_kwaycall. Spec §3.4/§4.3/§4.4/§4.5 updated and thekitchen_sinkconformance fixture regenerated in the same commit (Promote zagg-ragged/1 + digest + composition byte layouts to a normative spec (reader-migration gate) #340 rule).a73b64b6). Synthetic strata store (shard order 2 / cell order 4, one committed leaf): template → build →sweep_overviews/1 cascade regime — the finest level exact-from-leaves (_fold_node), the coarser a cascade (_cascade_node). At every level: both strata'sweight_totalfolds exactly; the composition word equals the k-way merge of its contributors'(word, n)pairs, recomputed independently from the arrays one level finer; presence exact vs one direct pooled pack of every raw row; the overview composition array carries the writer-stamped §3.3 attrs block;zagg_overview.fieldsrecords{class: packed, method: composition_kway}at both levels. The CA-manifest before/after known-answer is the phase-1 fixture + the rebuild test (test_declaration_writer_rebuilds_the_published_block).Composition-declaration decision
A third class token
packed(methodcomposition_kway) — not an extension of theexactarm.exactnormatively means "folds byte-equal to a direct aggregation at the coarser order" (spec §4.4).merge_composition_kwayis deterministic and order-independent within one fold call (single quantization), but it is not byte-equal to a direct pack over the raw rows, and a cascade re-quantizes once per level — presence exact through the chain, counts within one lane quantization per fold (spec §3.4). Declaring thatexactwould recreate the exact dishonesty the issue exists to end; declaring itapproximatebinds readers to a digest-shaped entry (ragged decode,inner_shape,delta) that a dense uint64 word does not have.class in ("exact", "approximate"), so pre-D24: admit build_tdigest_where to the digest family; give composition an honest composability declaration #515 zagg (and any external reader) treats apackedentry as non-composable — native-resolution reads, exactly today's behavior. Extending theexactarm instead would send old sweeps intofold_densewith an unknown law: a fold-time crash rather than a graceful skip. Spec §4.5 now states the unknown-token posture explicitly.ofdigest's per-cell weights (N_signal, spec §3.3). Neither existing arm's machinery carries a cross-field law; a distinct class makes the pairing explicit at every fold site (word and divisor are read together, one missing half contributes nothing / counts the contributor unreadable) instead of special-casing "exact-but-not-really" everywhere.declared_fieldswritespackedonly when theofdigest is itself declaredapproximatein the same map, and demotes to{class: none}otherwise — the law divides by that digest's weight at every level, so composition folds only where its divisor does.Wire-format consequence, handled per the #340 rule in the same commit: spec §3.4 (k-way form + composability), §4.5 (the
packedentry grammar), §4.3/§4.4 (cascade materiality, inclusion gate), and thekitchen_sinkconformance fixture regenerated — its manifest now declares strataapproximate+ compositionpacked, and itsall.pyramid.zarrleaf column carries the strata, locations and composition groups through the real fold path.Fixture-regen note:
kitchen_sink's two*_locations/zarr.jsonsiblings were deliberately restored after regeneration (git checkout --) — they are the committed pre-§9 absent-locatedpin (TestLocatedDeclaration::test_absent_declaration_on_the_pre_section_9_fixture); a pristine-main regen also rewrites them, so that churn is staleness, not this PR. The generator docstring's staleness notes were updated to say exactly this.Deployment note
Classification is template-time worker-side — the admission takes effect on the 0.50 fleet. The CA retrofit uses the leaf-reading path (
declare_pyramid+ cascade-from-leaves): the strata leaf columns were never written (classnoneat build time), so the retrofit re-reads leaves; no re-aggregation.semantic_hashis a frozen manifest key, so the retrofit's manifest write needs the declare-pyramid path (which updates thepyramidblock only), never a manifest rebuild.Adversarial review record
Three fresh-context review rounds (phase 1; phases 2–3; phase 4 + folds), 26 inline findings total (1 blocking — a fourth class-filter site
sweep_stages.pymissed by an earlier fold, fixed in484fd36d), all folded or answered on-thread: 22 folded asfold review:commits, 4 answered leave-by-design/superseded with rationale. Notable folds beyond the findings' letter: half-pair poisoning now fires only in the true skew direction (word absent while the divisor folds) at both_fold_nodeand_merge_slabs, with mutation checks confirming each direction's test bites; the e2e store gained a second leaf so a cascade fold-window off-by-one is caught.Testing
tests/test_strata_composability.py(phases 1–4: characterization → parity → end-to-end, 27 tests) + updated pins intests/test_sweep_overview.py; conformance suite green against the regenerated fixture. Final full run: 4762 passed, 38 skipped (docker-only build test deselected locally).test_lambda_build::TestFunctionBuild::test_function_build_succeedsis environmental on this machine (docker daemon not running; slow-marked build test), unrelated — deselected locally, runs on CI.Testsis red on one known gate, not on this PR's logic:test_lambda_build::TestFunctionBuild::test_function_build_size— the function zip sits at exactly 30.0MB against the 30MB budget (phase 3's ~200 src lines nudged it over the line). The budget raise to 32MB is espg-ruled and lands on PR closest-observation Sentinel-2 ingest builder (issue #509) #511; per §1 this PR does not touch the gate. Resolves when closest-observation Sentinel-2 ingest builder (issue #509) #511 merges — will re-verify then. Every other job (Lint, Lambda Build, Docs) is green, and the pytest failure count on the Tests job is exactly this one test (4751 passed).src/zagg/registry.py::UnknownCapability;ruff format --checkflag ontests/data/benchmark/README.md. The PR lint bot selects E,F,W,I only.Questions for review
Module size (§4 note, not a raise-request for this PR):
sweep_overview.py(2,403 lines) andsweep_stage.py(1,408) were already past the 1,200-line trigger on main before this PR; the packed fold adds ~130 lines across the two. Flagging per §4 rather than splitting mid-PR — if a split is wanted, it should be its own issue.Artifact-visible poison record — when
_fold_nodepoisons a half-paired leaf's composition cells (absence over a skewed word), the trace is alogger.warningonly; an artifact-visible record would need a new §4.3 attrs key (asource_childrenanalog for the /1 leaf fold) plus a fixture regen. Left standing as a scope decision rather than invented here. (_merge_slabs' poisoning IS recorded viasource_children.unreadable.)Config-side cross-check of a flux-weighted divisor —
declared_fieldsdemotes a packed field whoseofdigest declares flux weights, but_validate_composition_attrsdoes not refuse the pairing at config load (doing so would break configs legal today with no pyramid declared). Left standing.Stage gathers and the packed pair — a
/2gather level assigns gen-1 bytes per field independently (_gather_slabs), so a child column carrying composition without itsofdigest (only possible via schema evolution or a hand-edited manifest) would gather the word against a fill divisor. The merge sites treat that half-pair as unreadable; the gather keeps its assign-only contract. Flagged rather than changed — tightening the gather to pair-check would break its "gen-1 content untouched" acceptance contract.