temporal + located companion channels through the kernel and the pyramid (issue #410) - #463
Conversation
| (``mortie.tocs_reduce``, the segmented ``toc_merge`` reduce, issue #177): | ||
| the conservative envelope containing every observation the centroid | ||
| summarizes, and — because the join is associative, commutative and | ||
| idempotent — **bit-identical under any fold tree**, which is what makes the |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix — the per-centroid channel is not "bit-identical under any fold tree", and §8.3 says so explicitly.
idempotent — **bit-identical under any fold tree**, which is what makes the
channel merge-order-independent where the digest payload itself is only
approximately so (spec §2.3/§8.3).The join is a semilattice, but a per-centroid vector is indexed by the centroid partition, and that partition moves with the fold tree. docs/specification.md §8.3 draws exactly this line:
The companion's exactness does not lift the field's composability class: a merged payload's centroids may differ, and the words are exact given the centroid partition they describe, not independently of it.
and the design ledger says the same (#410 comment, point (b)): "the exactness of the toc merge cannot lift the field's composability class on its own."
Measured on this commit (4 digests of 200 obs, δ=32, ./.venv/bin/python):
kway k = 37 pairwise k = 41
per-centroid words bit-identical across fold laws? False
cell-level envelope identical? True
balanced-tree words == kway words? False
balanced cell envelope == kway cell envelope? True
So the fold-tree invariance is a cell-level property (which is what licenses §8.4's per-centroid → per-cell coarsening, and what test_cell_envelope_is_fold_tree_independent actually pins). The PR body gets this right — "a cell-level reduction is bit-identical under any fold tree" — the docstring drops the qualifier. Same sentence needs the same fix at merge_tdigests_kway, line 575: "so its words are bit-identical under any fold tree, not merely under a permutation of one flat k-way call."
Consequence: a phase-3 reader of this docstring could conclude an overview's per-centroid words are reproducible from any fold tree and skip pinning the fold order in a conformance fixture, which is precisely the failure §8.3's clause exists to prevent.
nit, same docstring (line 223): issue #177 is an espg/mortie issue, not a zagg one — pyproject.toml writes it correctly as espg/mortie#177. As written it reads as zagg#177.
There was a problem hiding this comment.
🤖 from Claude
Folded in e1703a5. The docstring no longer claims per-centroid fold-tree invariance; it now says the join buys cell-level invariance and that a per-centroid vector is indexed by the centroid partition, whose words are exact given that partition and not independently of it — quoting docs/specification.md §8.3 directly, and pointing at zagg.stats.toc.cell_envelope / test_cell_envelope_is_fold_tree_independent as the thing that actually holds.
The same sentence at merge_tdigests_kway is rewritten too: it now separates permutation-independence of one flat k-way call (which the tie keys do buy) from fold-tree independence (which only the cell-level envelope has). I also fixed a third instance you did not flag — the temporal1, temporal2 parameter doc on merge_tdigests, which said "exact and fold-tree independent, unlike the payload"; same overclaim, same commit.
The bare issue #177 is now espg/mortie#177. Checked the rest of the tree: the remaining #177 mentions (tests/test_lambda_handler.py, docs/deployment/lambda.md) are the genuine zagg issue and were left alone.
|
|
||
| The temporal channel is the exception the difference does not reach: its | ||
| join is associative and commutative (spec §8.3), so a pairwise left-fold | ||
| and a k-way fold produce bit-identical toc words. |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix — this claim is false, and falsifiable without subtlety: the two fold laws don't even return toc vectors of the same length.
The temporal channel is the exception the difference does not reach: its
join is associative and commutative (spec §8.3), so a pairwise left-fold
and a k-way fold produce bit-identical toc words.build_tdigest_pairwise differs from build_tdigest only in the cross-block fold law (merge_tdigests left-fold vs merge_tdigests_kway). The per-centroid vector is one word per output centroid, and the two laws produce different centroid counts — that is the whole point of the pairwise/k-way distinction (merge_tdigests "drifts above the δ budget", two lines above). Measured on this commit, 4 blocks × 200 obs, δ=32:
kway k = 37 pairwise k = 41
per-centroid words bit-identical across fold laws? False
cell-level envelope identical? True
What is true is the cell-level statement: cell_envelope over either vector gives the same word. Suggest the docstring say that instead — e.g. "the channel's cell-level envelope is the exception … zagg.stats.toc.cell_envelope over either fold's words returns the same token, though the per-centroid vectors differ with the partition (§8.3)".
Note this is the same overclaim as the one at line 226 / line 575, but here it is stated as a concrete equality between two named functions, so it is the one most likely to be relied on.
There was a problem hiding this comment.
🤖 from Claude
Folded in 03dc7a2, taking your suggested phrasing. build_tdigest_pairwise now reads:
The temporal channel does not escape that difference either: the two laws
reach different centroid partitions (different counts, even), so their
per-centroid toc vectors differ. What the join being a semilattice (spec
§8.3) buys is the *cell-level* statement —
:func:`zagg.stats.toc.cell_envelope` over either fold's words returns the
same token — since the words are exact given the partition they describe,
not independently of it.
The differing-length point is now stated first, since that is the falsifiable half.
| delta: int = ..., | ||
| *, | ||
| temporal1: np.ndarray, | ||
| temporal2: np.ndarray, |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix — the both-channels overload is missing on merge_tdigests and on merge_tdigests_kway, so declaring both channels at a merge site is a mypy error. build_tdigest (line 283) got the 4th overload; the two merge functions did not.
Verified with the pinned pre-commit mypy (mirrors-mypy v1.19.1, MYPYPATH=src):
error: No overload variant of "merge_tdigests" matches argument types ... [call-overload]
note: Possible overload variants:
note: def merge_tdigests(d1, d2, delta=..., locations1: None = ..., locations2: None = ...) -> ndarray
note: def merge_tdigests(d1, d2, delta=..., *, locations1, locations2) -> tuple[ndarray, ...]
note: def merge_tdigests(d1, d2, delta=..., *, temporal1, temporal2) -> tuple[ndarray, ...]
error: No overload variant of "merge_tdigests_kway" matches argument types ... [call-overload]
(probe: merge_tdigests(d, d, 512, locations1=w, locations2=w, temporal1=w, temporal2=w) and merge_tdigests_kway([d], 512, locations=[w], temporal=[w]); the equivalent build_tdigest(..., locations=w, temporal=w) resolves cleanly to a 3-tuple.)
Consequence: phase 3 plumbs both channels through sweep_overview.fold_digests / _fold_child / _fold_node / sweep_stage._merge_slabs, i.e. exactly the both-channels merge call that has no matching overload — so pre-commit run --all-files will fail there and the fix will look like an unrelated typing change in a later phase. The runtime implementation already handles both together correctly (I ran it: merge_tdigests(empty, d3, 512, locations1=…, locations2=l3, temporal1=…, temporal2=t3) returns the right 3-tuple), so this is purely the missing declaration.
Also worth noting while you're in here: the located overloads' return narrowed from tuple[np.ndarray, np.ndarray] to tuple[np.ndarray, ...], which drops arity information for the existing 2-tuple call sites (processing/spill.py:863, processing/spill.py:964). mypy still accepts merged, merged_locs = merge_tdigests(...) against a variadic tuple (I checked), so nothing breaks — but a per-arity tuple[np.ndarray, np.ndarray] / tuple[np.ndarray, np.ndarray, np.ndarray] on each overload would keep what build_tdigest still has.
There was a problem hiding this comment.
🤖 from Claude
Folded in 0c99c67. Both merge functions now carry the both-channels overload, and I took your second suggestion too — the single-channel overloads are back to per-arity returns instead of the variadic tuple[np.ndarray, ...], matching what build_tdigest still has:
merge_tdigests: located →tuple[ndarray, ndarray], temporal →tuple[ndarray, ndarray], new*, locations1, locations2, temporal1, temporal2→tuple[ndarray, ndarray, ndarray].merge_tdigests_kway: same shape, with the new*, locations, temporaloverload.
Verified against the pinned pre-commit mypy (mirrors-mypy v1.19.1 with the hook's numpy==2.1, MYPYPATH=src) on your exact probes. Both [call-overload] errors are gone and the returns now reveal as expected:
merge_tdigests(d, d, 512, locations1=w, locations2=w, temporal1=w, temporal2=w)
-> tuple[ndarray, ndarray, ndarray]
merge_tdigests_kway([d], 512, locations=[w], temporal=[w])
-> tuple[ndarray, ndarray, ndarray]
merge_tdigests(d, d, 512, locations1=w, locations2=w)
-> tuple[ndarray, ndarray]
merge_tdigests_kway([d], 512, temporal=[w])
-> tuple[ndarray, ndarray]
The same run also flagged one more error inside merge_tdigests that phase 1 introduced — error: Item "function" of "Callable[...] | ndarray" has no attribute "copy" [union-attr] on c[keep].copy(), because keep doubles as the digest selector and the tuple index into (reducer, c1, c2). Since it is the same function and the same check, I fixed it in the same commit by dropping the index magic:
left = d2.size == 0
kept = np.asarray(d1 if left else d2, dtype=np.float32)
if not channels:
return kept
return (kept, *((c1 if left else c2).copy() for _, c1, c2 in channels))Left standing, flagged rather than folded: build_tdigest_pairwise still reports Argument "locations"/"temporal" to "build_tdigest" has incompatible type "ndarray | None"; expected "None" [arg-type]. That is pre-existing — the pre-phase-1 body had the identical build_tdigest(values, delta=delta, locations=locations) shape against the identical overload set, so phase 1 only doubled an error that was already there. Fixing it means the kwargs dict trick build_tdigest_where uses, which is a wider change than this finding. Worth noting mypy is not in CI here (lint.yml runs ruff only; mypy is pre-commit-local, and the hook env reports ~198 errors tree-wide including missing mortie/boto3/h5coro stubs), so none of this was gating — but the declaration was still wrong.
| A silent ``uint64`` cast would turn a mis-declared float column into | ||
| plausible-looking morton or toc words, so both channels require packed | ||
| ``uint64`` outright — what ``HealpixGrid.assign`` supplies as ``leaf_id`` | ||
| and what :func:`zagg.time_axis.observation_words` supplies as toc words. |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix — zagg.time_axis.observation_words does not exist.
plausible-looking morton or toc words, so both channels require packed
``uint64`` outright — what ``HealpixGrid.assign`` supplies as ``leaf_id``
and what :func:`zagg.time_axis.observation_words` supplies as toc words.Repo-wide, this docstring is the only occurrence of the name:
$ grep -rn "observation_words" src tests tools docs
src/zagg/stats/tdigest.py:261: and what :func:`zagg.time_axis.observation_words` supplies as toc words.
src/zagg/time_axis.py defines time_encoding, time_axis_dtype, time_axis_attrs, temporal_declaration_block, temporal_attrs, temporal_declaration, _internal_ns, encode_time_axis, decode_time_axis, time_axis_overlaps, read_time_axis — no observation_words. The HealpixGrid.assign half of the same sentence is real, which makes the toc half read as a live reference rather than a placeholder.
Consequence: a dead :func: role (broken cross-ref if these docstrings are ever rendered), and if phase 2's per-observation encode lands under a different name the docstring silently keeps pointing at nothing. Either drop the reference or phrase it forward-looking ("the per-observation toc encode phase 2 adds").
There was a problem hiding this comment.
🤖 from Claude
Already resolved by later work on the branch — no edit needed, and the reference is live now.
zagg.time_axis.observation_words was added by phase 2 in a4e123a, after this review was taken:
$ git log --oneline -S "def observation_words" -- src/zagg/time_axis.py
a4e123a8 phase 2 of issue #410
$ grep -n "observation_words" src/zagg/time_axis.py
135: "observation_words",
296:def observation_words(values, *, epoch, scale: str, units: str) -> np.ndarray:
It is in __all__, and its own docstring is "Per-observation offsets -> exact toc timestamp words (§8.3)" — i.e. exactly what the _check_words docstring said it supplies. So the :func: role resolves and the forward-looking rephrase you offered as the alternative is not needed.
The finding was correct at the sha it was written against; leaving the docstring as-is rather than churning it.
| from mortie import tocs_reduce | ||
|
|
||
| starts = np.asarray(starts, dtype=np.int64) | ||
| if temporal.size and not temporal.all(): |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — the zero test itself is right, but its placement leaves the reserved word un-refused on exactly the paths that don't reduce.
The predicate is correct for uint64 (0 is the only falsy word, .all() short-circuits nothing but is a single vectorized pass, and the temporal.size guard covers the empty case) — and refusing rejects nothing legitimate. I confirmed the grammar side against mortie 0.9.9:
time2toc(0) -> [2147483648] # epoch instant, flag bit at 31
span2toc(0, 0) -> [1] # shortest range word
span2toc(0, 1) -> [1]
so 0 is unproducible, matching §8.2's "The reservation is cost-free".
The gap: the check lives in _centroid_envelopes, which is only called when a reduction actually happens. The pass-through arms skip it:
merge_tdigests(d, empty, 512, temporal1=<vector with a 0>, temporal2=empty)
-> passthrough with reserved 0: NOT refused -> 0
merge_tdigests_kway([d, empty], delta=512, temporal=[<vector with a 0>, empty])
-> kway single-contributor reserved 0: NOT refused -> 0
merge_tdigests(d, d, 512, temporal1=<vector with a 0>, temporal2=t)
-> refused: "temporal words contain the reserved 0 word (spec §8.2's unob…"
A single-block cell (the common spill shape) and a single-contributor overview fold therefore write the reserved word straight through, which is the one thing §8.2 says a writer MUST NOT store for an observed cell. The natural home is _check_words (line 255) — it already runs on every channel argument on every arm including the pass-throughs, and it's where toc.py::_checked puts the same check. That would also collapse the current duplication: two independent implementations of "refuse the reserved 0" with two different messages ("temporal words contain the reserved 0 word (spec §8.2's unobserved marker); …" here vs "toc words contain the reserved 0 word (spec §8.2's unobserved-cell marker); …" in toc.py), only one of which the tests/test_stats_toc.py / tests/test_tdigest.py match="reserved 0 word" assertions distinguish.
(If the split is deliberate — _check_words being channel-agnostic and 0 being a toc-only reservation — a one-line comment saying so would stop the next reader from "tidying" it.)
There was a problem hiding this comment.
🤖 from Claude
Folded the placement half in 36812c3; declined the cross-module de-duplication half, with a comment left behind saying why.
Folded — the check moved to _check_words. Your read of the gap is right, so the refusal now sits in the one validator every arm runs, and _centroid_envelopes no longer carries it. That closes both pass-throughs you measured, and the message stayed on the reserved 0 word substring the existing assertions pin. Pinned in the same commit:
test_pass_through_arms_still_refuse_the_reserved_zero— all three arms:merge_tdigests(d, empty, temporal1=leaked, ...), the mirroredmerge_tdigests(empty, d, ..., temporal2=leaked), andmerge_tdigests_kway([d, empty], temporal=[leaked, empty]).test_reserved_zero_refusal_is_channel_agnostic— the located channel now refuses a leaked fill on its pass-through as well. That is a deliberate tightening beyond the finding:0is no more a valid morton word than a toc word (validate_mortonrejects it), and the located channel had the identical hole becausecommon_ancestoralso only runs when a reduction happens. Ranpytest -k "digest or spill or streaming or toc or overview or sweep"— 825 passed, so nothing was relying on a zero surviving a pass-through.
Declined — collapsing toc.py::_checked into _check_words. Judgment call, and it stays standing for review. _check_words's second job is aligning a channel against a digest's centroid count, a shape the per-cell reducers have no analogue for, so sharing it means making that argument optional and importing a private symbol from the digest kernel into the module whose docstring exists to say it owns the coarser shape. It would also cost toc.py its TOC_UNOBSERVED self-documentation (the shared message hardcodes 0) and churn two public error messages for no behavioral gain — the duplication is four lines.
Taking your own suggestion for that case instead: _checked now says so out loud, so the next reader does not tidy it —
Deliberately separate from the digest kernel's
:func:`zagg.stats.tdigest._check_words`, which makes the same refusal: that
one's other job is aligning a channel against a digest's centroid count, a
shape these per-cell reducers have no analogue for, and this module names the
marker through :data:`zagg.time_axis.TOC_UNOBSERVED` rather than a literal.
Thanks for the mortie 0.9.9 probes confirming 0 is unproducible — that is what makes the refusal safe to hoist to the validator, where it now runs on every declared channel rather than only on the reducing arms.
| ) | ||
| np.testing.assert_array_equal(merged, d) | ||
| np.testing.assert_array_equal(out, t) | ||
| assert out is not t, "the channel must not alias the caller's array" |
There was a problem hiding this comment.
🤖 from Claude (review)
nit (test strength) — TestTemporalMergeLaws never exercises merge_tdigests with two non-empty sides. The three tests that touch it are the arity raise (line 971), this pass-through (line 975), and nothing else — so the pairwise merge's temporal fold, the
joined = [np.concatenate([c1, c2])[order] for _, c1, c2 in channels]→ _centroid_envelopes(words, starts, n) path at lines 526/536, has no correctness assertion on the channel at all. The lines are covered (98% line + branch on stats/tdigest.py from tests/test_tdigest.py tests/test_stats_toc.py), but only by the pre-existing located tests going through the same channel-agnostic loop; nothing asserts a merged toc word contains its members, or that the two channels stay row-aligned with each other, through the pairwise law.
That matters because merge_tdigests is the law two shipped call sites use — processing/streaming.py:342 and processing/spill.py:873 — and it is a distinct code path from the k-way one that test_kway_* covers (different concatenate, different tie-break: merge_tdigests uses argsort on means with no channel tie key, merge_tdigests_kway uses the 4-key lexsort).
The cheapest fix mirrors the build-path test one class up: two overlapping-value digests with known member instants, merge_tdigests(d1, d2, delta=8, temporal1=t1, temporal2=t2), then assert every output word's decoded [start, end) contains its contributors' words via mortie.toc_contains / toc_overlaps — no need to reconstruct the partition. Adding both channels at once to the same call would also cover the zip(channels, joined, strict=True) pairing, which is currently only pinned k-way (test_kway_both_channels_independent_of_each_other).
Two smaller notes in the same vein, both genuinely fine at runtime — I checked each by hand — but unpinned:
merge_tdigests'skeep = 1 if d2.size == 0 else 2/c[keep]index magic (line 517). Thekeep == 2arm is covered for the located channel (line 360,merge_tdigests(empty, d1, locations1=…, locations2=l1)) but not for temporal, andkeepdoubles as both the digest selector and the tuple index into(reducer, c1, c2). I verified all four emptiness combinations return the right channel, un-aliased, with one and two channels declared. Amerge_tdigests(empty, d, temporal1=…, temporal2=t)case would pin the arm that a channel-swap regression would break silently.build_tdigest_where's channel shape check (line 411) is the one new statement instats/tdigest.pywith no coverage from either new test file;test_mismatched_lengths_raiseonly exercises thebuild_tdigestcopy of the check.
There was a problem hiding this comment.
🤖 from Claude
Folded in fe05788 — all three gaps, plus the both-channels pairing you noted in passing.
The two-non-empty-sides temporal fold (test_pairwise_merge_of_two_sides_envelopes_contain_their_members): two overlapping-value digests from _parts(k=2, n=150) merged at delta=8, asserting len(out) == len(merged) < len(t1) + len(t2), that mortie.toc_is_range(out).any() (so the compressing path really ran), and that every input word is covered by some output envelope via decoded [start, end) bounds. I used toc2time bounds rather than toc_contains, because toc_contains takes an integer window and a timestamp envelope decodes to [t, t] — an empty half-open window, which contains nothing — so the containment predicate has to handle the timestamp case explicitly. Same convention the sibling build-path test at line 864 already uses.
The both-channels pairwise pairing (test_pairwise_merge_both_channels_independent_of_each_other): mirrors test_kway_both_channels_independent_of_each_other onto the pairwise law — each channel byte-identical to declaring it alone, digest byte-identical to declaring neither. A swap in zip(channels, joined, strict=True) would leave each vector reduced by the other channel's law, so neither would match.
The keep == 2 arm for temporal (test_pairwise_merge_passes_the_right_side_through): merge_tdigests(empty, d, temporal1=empty_w, temporal2=t), asserting the digest and channel come back as d / t and that the channel is not aliased. Note the arm itself no longer uses the keep index magic — 0c99c67 replaced it with an explicit left = d2.size == 0 when fixing the [union-attr] mypy error on c[keep].copy(), so the selector and the tuple index are no longer the same variable.
build_tdigest_where's channel shape check (test_where_reducer_refuses_a_mismatched_channel): pins the build_tdigest_where copy with match="temporal shape .* does not match values shape", which is tight enough to distinguish it from the build_tdigest copy test_mismatched_lengths_raise already covers.
pytest tests/test_tdigest.py tests/test_stats_toc.py is 139 passed.
|
|
||
| def test_empty_group_raises(self): | ||
| words = _toc_words(6) | ||
| with pytest.raises(ValueError, match="empty segment|empty"): |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — match="empty segment|empty" collapses to match="empty": the second alternative subsumes the first, so the assertion passes on any ValueError whose message merely contains the substring "empty" — including one raised for an unrelated reason (a future numpy "empty array" message, say), and including one raised before the reduce is reached.
The message mortie 0.9.9 actually raises here is stable and specific:
ValueError: group 1: tocs_reduce of an empty segment (the merge has no identity element)
so match="empty segment" alone (or match="no identity element", which matches cell_envelope's own wording too) pins what the test means to pin. Worth noting the sibling test one class up uses the tight form — match="no identity element" at line 63 — so this is just the loose one.
While here: mortie validates the rest of the offsets contract that cell_envelopes delegates, and does it well — I probed the three shapes zagg passes through unchecked and all three raise ValueError with the offending index named (offsets must be monotonically non-decreasing (2 < 4), offsets must end at the word count: offsets[1] is 3 but words has 6 entries, group 1: offset 99 exceeds word array length 6). So the decision not to re-validate offsets in cell_envelopes looks right; no finding there.
There was a problem hiding this comment.
🤖 from Claude
Folded in be1b640 — match="empty segment|empty" is now match="empty segment", matching the sibling match="no identity element" at line 63 for tightness. Kept empty segment rather than no identity element because that is the wording mortie raises on this path (group 1: tocs_reduce of an empty segment ...), so the assertion pins the message the delegated reduce actually produces.
Noted on the offsets contract — no change made there, and none was asked for.
|
🤖 from Claude P0 evidence: the per-centroid toc byte cost, measured on real ATL03The consolidated plan's step (1) — "Sierra tail subset: per-centroid toc byte cost post-zstd … and the What I could not use. There are no Sierra-tail artifacts in the tree ( What I used instead. The local repeat-track-1336 stack — 29 real ATL03 v007 granules, cycles 01–30, 2018-12-25 → 2026-03-11, 56 GB, beam Geometry is the shipped located config's, unmodified — Result — two independent along-track bands
Stored bytes per centroid (post-zstd;
What this settles
The read-side half, and its caveat
Wall time is NOT measured here and the local number should not be quoted. These are local-disk reads (+16.4–16.9% wall), and the memory that matters is that reads are the wall — over h5coro/S3, per-column byte-range behavior, request count and concurrency dominate, none of which a local Reproducing
|
| # ``utc_to_offset`` of the epoch against itself is 0 for a post-2017 epoch | ||
| # and the scale-UTC offset for a pre-2017 one — windows.py's branch, reused | ||
| # rather than restated. | ||
| shift_s = utc_to_offset(epoch_dt, epoch=epoch_dt, scale=scale, units="seconds") |
There was a problem hiding this comment.
🤖 from Claude (review)
blocking — shift_s double-counts the leap offset. For a pre-2017 epoch the stored word is 18 s (gps) / 37 s (tai) off the instant and 18 s away from what windows.offset_to_utc places the same observation at — the exact hazard this design is the answer to.
shift_s = utc_to_offset(epoch_dt, epoch=epoch_dt, scale=scale, units="seconds")
...
offsets = np.asarray(values, dtype=np.float64) * UNIT_SECONDS[units] - shift_sbase is not a naive count. _internal_ns → mortie.from_datetime64 is leap-aware, and mortie's internal scale is the continuous GPS scale (GPS − UTC = 0 at its 1980-01-06 alignment):
| UTC instant | _internal_ns |
naive ns since 1850 | diff |
|---|---|---|---|
| 1850-01-01 | 0 | 0 | 0 |
| 1980-01-06 | 4102790400000000000 | 4102790400000000000 | 0 |
| 2017-01-01 | 5270054418000000000 | 5270054400000000000 | +18 s |
| 2018-01-01 | 5301590418000000000 | 5301590400000000000 | +18 s |
So base already carries the scale correction, and the correct conversion for a continuous-scale column is internal = base + v·1e9, with no shift at all. Measured with scale: gps, epoch 1980-01-06 (a GPS-native column — the case the comment above says the branch exists for) and v = 1.4e9 (a 2024 observation):
word (current) = 2024-05-17T16:52:44
word (no shift) = 2024-05-17T16:53:02 <- the true instant
router says = 2024-05-17T16:53:02 (windows.offset_to_utc)
current vs router = -18 s
Repro:
import numpy as np, mortie
from zagg.time_axis import observation_words
from zagg.windows import offset_to_utc
w = observation_words(np.array([1.4e9]), epoch="1980-01-06T00:00:00", scale="gps", units="seconds")
print(mortie.to_datetime64(mortie.toc2time(w)[0])[0]) # 2024-05-17T16:52:44
print(offset_to_utc(1.4e9, epoch="1980-01-06T00:00:00", scale="gps")) # 16:53:02+00:0018 s is ~8 quanta of the range variant's ~2.15 s start grid, so this is precisely "an observation routed into window W whose stored word reads as outside W" from the Q4 discussion on #410 — and §8.3's "MUST encode an instant as a timestamp word, exact to the nanosecond" is missed by 18 s, not by rounding.
Two notes on the surrounding rationale:
- The docstring's "the scale-vs-UTC correction for a pre-2017 epoch is taken from
zagg.windows.utc_to_offsetrather than recomputed here, so the toc words and the window routing cannot drift apart" is falsified by the number above: the shift does not achieve router agreement, it creates an 18 s disagreement. Dropping it makes the word exact and makes word and router agree for every post-2017 observation, leaving onlywindows.py's own documented ≤ 1-leap-second tolerance, which nothing at this seam can fix. scale's load-bearing role here is theTOC_SOURCE_SCALESrefusal ofutc.utc_to_offset's quantity (scale − UTC, era-gated on the epoch) is simply not the quantity a leap-awarebaseneeds; for a UTC-labelled epoch,vcounts SI seconds from an instantbasealready locates, on either continuous scale.
No shipped template is affected: ICESat-2's 2018 ATLAS SDP epoch is post-2017, so shift_s == 0 there — which is also why the suite is silent about it (separate comment on the test).
There was a problem hiding this comment.
🤖 from Claude
Folded in c602d1f8 — shift_s is gone; the conversion is internal = base + v·1e9 with no correction, exactly as derived. Your repro now prints 2024-05-17T16:53:02 for the word, matching windows.offset_to_utc. The shipped 2018-epoch path is byte-identical (shift_s was 0 there), and tests/test_spec_conformance.py::TestTemporalCompanions::test_the_production_kernel_reproduces_the_committed_words still passes against the committed §7 fixture words.
The docstring paragraph you quoted is rewritten to say why no correction belongs here — mortie.from_datetime64 is leap-aware, so base already places the epoch on the continuous scale and values counts SI seconds forward from it — instead of claiming the correction buys router agreement.
One residue recorded rather than hidden: word/router agreement is now exact for a post-2017 epoch and for the GPS-native case you measured, but a pre-2017 tai-labelled epoch still splits by 19 s, because windows.offset_to_utc reads a pre-2017 non-UTC epoch as native to its own scale (seconds -= 37) while the word takes the epoch string as the UTC instant values counts from. The old code was 18 s off the router there too, in the other direction. That is a windows.py epoch-convention question rather than something this seam can bridge, so the docstring names it explicitly and it stands for review.
| out = self._words([]) | ||
| assert out.shape == (0,) and out.dtype == np.uint64 | ||
|
|
||
| def test_agrees_with_the_window_router_at_a_boundary(self): |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix — this test cannot catch the bug it exists to prevent, because every case in TestObservationWords uses EPOCH = "2018-01-01T00:00:00", and utc_to_offset(epoch, epoch=epoch, ...) returns 0.0 for any epoch on/after 2017-01-01. So shift_s == 0 in all ten tests: the pre-2017 branch of observation_words — the only branch where the correction does anything, and the one that is wrong (see the comment on src/zagg/time_axis.py line 328) — is never executed. Flipping the sign of - shift_s, or deleting it, leaves the whole class green.
test_round_trips_to_the_declared_instants has the same blind spot from the other side: expected is re-derived as np.datetime64(EPOCH) + rint(offsets * 1e9), i.e. the code's own naive-nominal formula, so it pins the round trip only where the correction is inert.
Cheapest fix that would have caught it — one case on a native GPS epoch, asserted against windows.offset_to_utc rather than against a re-derivation:
def test_agrees_with_the_window_router_for_a_pre_2017_epoch(self):
import mortie
from zagg.windows import offset_to_utc
epoch, v = "1980-01-06T00:00:00", 1.4e9 # GPS-native epoch, a 2024 observation
word = self._words([v], epoch=epoch)
decoded = mortie.to_datetime64(mortie.toc2time(word)[0])[0]
assert decoded == np.datetime64(
offset_to_utc(v, epoch=epoch, scale="gps").replace(tzinfo=None), "ns"
)Today that asserts 2024-05-17T16:52:44 == 2024-05-17T16:53:02 and fails by 18 s.
There was a problem hiding this comment.
🤖 from Claude
Folded in e6e6221f. Added test_pre_2017_epoch_is_not_leap_shifted on the GPS-native epoch you suggested, asserting both against the literal instant (2024-05-17T16:53:02) and against windows.offset_to_utc, so a correction reintroduced with either sign fails by 18 s. The other half you named is fixed too: test_round_trips_to_the_declared_instants now compares against three literal datetime64[ns] instants instead of re-deriving epoch + rint(offsets * 1e9), with a comment recording that the re-derivation was the blind spot.
| Returns ``{"field", "epoch", "scale", "units"}`` with defaults resolved, or | ||
| ``None`` when nothing is declared. **An absent block falls back to | ||
| ``output.windowing``** when that block already carries a continuous-scale | ||
| clock: window routing and toc ingest then derive from ONE declaration, which |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix — the fallback single-sources the clock only when output.time_source is absent. When both blocks are declared, nothing compares them, so a store can carry two conventions after all — the failure this paragraph says the fallback prevents.
Measured against validate_config (accepted, no warning):
output = {
"windowing": {"schedule": "yearly", "time_field": "delta_time",
"epoch": "2018-01-01T00:00:00", "scale": "gps", "units": "seconds"},
"time_source": {"field": "other_time", "epoch": "2020-06-01T00:00:00",
"scale": "tai", "units": "days"},
}
validate_config(cfg) # passes
toc_source(cfg) # -> {'field': 'other_time', 'epoch': '2020-06-01T00:00:00', ...}Window routing then filters on delta_time against a 2018 epoch while every stored word is encoded from other_time against a 2020 epoch on a different scale in different units — unbounded disagreement, not a one-quantum edge effect. _validate_time_source checks shape, vocabulary and column membership, but never cross-checks the windowing block; toc_source reads whichever is present and prefers time_source.
Two options, either fine by me:
- When both blocks exist, require the four normalized keys to agree (or at least
field+epoch), naming both declarations in the message. This makes the docstring's claim true rather than aspirational. - Keep them independent but say so here — drop "which is what keeps them from disagreeing at a window boundary" and state that a store declaring both is responsible for their agreement.
Related, worth a sentence either way: the sentence below ("A windowed store on scale: utc gets no fallback ... and must declare a continuous column explicitly") is the one path that deliberately ends with two declarations of the same column on different scales. That is harmless for a post-2017 epoch (utc_to_offset differs between utc and gps only for a pre-2017 epoch) and an 18 s split for a pre-2017 one — the same 18 s as the shift_s finding on line 328, so whatever lands there should decide this sentence too.
There was a problem hiding this comment.
🤖 from Claude
Folded option (1) in f666ec7c. config._validate_time_source now cross-checks the two blocks whenever both are declared and output.windowing carries a continuous clock: field/scale/units compared directly, epoch compared as parsed instants (the windowing block's is canonicalized to ...+00:00), and the message names every disagreeing key with both values. The config you measured now raises instead of validating.
scale: utc windowing is exempt by construction — the check only runs when the windowing scale is in TOC_SOURCE_SCALES — because that is the path that must declare a continuous column separately. Pinned by test_both_clock_declarations_must_agree and test_a_utc_windowing_block_is_exempt_from_the_cross_check; the toc_source docstring now states the cross-check and the exemption rather than implying the fallback covers the both-declared case.
On your closing paragraph: with the shift gone (c602d1f8) the utc-windowed pairing is no longer an 18 s split at a pre-2017 epoch — routing on scale: utc places v at epoch + v and so does the word, so they agree. The tai pre-2017 case still differs by 19 s, for the windows.py-convention reason noted on the shift_s thread.
| f"reserved name of the derived toc word column output.time_source " | ||
| f"materializes (spec §8.2/§8.3) — rename the column" | ||
| ) | ||
| ds_vars = ds_vars | {TOC_WORD_COLUMN} |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix — validation and materialization disagree about when toc_word exists, so a config can name it, validate clean, and then die at runtime.
Here the column is added to ds_vars whenever toc_source(config) is not None — which, via the output.windowing fallback, is every windowed store on gps/tai, temporal companion or not. But aggregate.calculate_cell_statistics materializes it only when a field declares one:
if n_obs and _temporal_fields(agg_fields):
cell_data = {**cell_data, TOC_WORD_COLUMN: _toc_word_column(cell_data, config)}So with a plain windowed ICESat-2 config and no temporal: anywhere:
# aggregation.variables
"t_max": {"function": "numpy.max", "source": "toc_word", "dtype": "uint64", "fill_value": 0}validate_config(cfg) passes, and the first cell raises KeyError: 'toc_word' inside calculate_cell_statistics. Same shape via the precompute hook, which reserves the name but now also admits it as a readable column:
aggregation["chunk_precompute"] = {"tmax": {"expression": "toc_word.max()"}}
# validate_config: accepted; _eval_chunk_precompute: NameError: name 'toc_word' is not definedThe comment above ("it is a real base-rate column everywhere one is valid") and aggregate.py's ("Materialized only when a field declares a companion") are the two halves of the contradiction. Either condition works, they just have to be the same one:
- materialize on
toc_source(config) is not None(matches this validation, costs one encode pass on windowed runs that never read it), or - gate this
ds_varsaddition onany(m.get("temporal") for m in config.aggregation.get("variables", {}).values())(keeps the encode off configs that don't ask for it, and keeps_validate_chunk_precompute's name reservation unconditional, which is the right side for a reserved name).
I'd take the second: the reservation should be unconditional, the readability shouldn't.
There was a problem hiding this comment.
🤖 from Claude
Folded your option (2) in ea9d3297. The ds_vars widening is now gated on any(m.get("temporal") ...) over aggregation.variables — exactly the condition aggregate.calculate_cell_statistics materializes on — and the name reservation moved out of the toc_source(config) is not None branch, so it is unconditional like leaf_id.
Both of your repros now fail at validation rather than in the worker: source: toc_word with no temporal: anywhere gives Variable 't_max': source 'toc_word' not in data_source.variables, and the precompute form gives expression references 'toc_word' which is not in data_source.variables or numpy namespace. Declaring a companion makes both resolve. Pinned by test_derived_column_is_only_readable_where_it_is_materialized and test_chunk_precompute_cannot_read_the_unmaterialized_derived_column; test_declared_column_may_not_shadow_the_derived_name gained an unclocked config so the reservation's unconditionality is pinned too.
| # overflows the cast (numpy warns and yields garbage) where the grammar's | ||
| # own limits are what the caller needs named. The window is the timestamp | ||
| # variant's own domain — ns since the epoch, below the 63rd bit. | ||
| ns = np.rint(offsets * 1_000_000_000.0) |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix — np.rint(offsets * 1e9) is not the nearest nanosecond at ICESat-2 magnitudes, which is the one number §8.3 states as a MUST ("a writer MUST encode an instant as a timestamp word, exact to the nanosecond") and this function's docstring restates ("exact timestamp words").
By 2026 delta_time is ~2.5e8 s, so offsets * 1e9 lands near 2.5e17 where a float64 ulp is 32 ns. The multiply therefore quantizes before rint ever runs, and the result can be up to 16 ns off the nearest ns of the float64 offset it was handed. Measured over 20 000 draws in [2.4e8, 2.5e8), compared against Fraction(v) * 10**9 (exact):
rint(v * 1e9) max |err| = 16 ns
floor/frac split max |err| = 0 ns
The fix is local and keeps everything else:
whole = np.floor(offsets)
ns = whole * 1_000_000_000.0 + np.rint((offsets - whole) * 1_000_000_000.0)(whole is exactly representable and offsets - whole is in [0, 1), so both products are exact/nearest.)
Two framing notes so this isn't over-read: the source column is itself float64, whose own ulp at 2.5e8 s is ~30 ns, so no writer can be closer to the true instant than that — this is about the avoidable half, i.e. being the nearest ns of the value the reader actually delivered, which is the strongest claim a writer can make and the one the docstring makes. And units: days is worse in the same way (a ~2900-day offset multiplies to the same 1e17 range through one extra rounding).
If instead the intent is that ~30 ns is inherent and the MUST is read as "timestamp variant, not widened into a range", then the docstring should say that rather than "exact", because an external reader decoding from §8 + the fixtures will take "exact to the nanosecond" literally.
There was a problem hiding this comment.
🤖 from Claude
Folded in 480ee86b — the floor/frac split as you wrote it, with one addition: the final combine is done in int64 (base + whole * 10**9 + frac_ns) rather than float, because the float sum near 2.5e17 cannot hold an arbitrary nanosecond integer either — same 32 ns ulp — so keeping it in float would have re-lost up to 16 ns at the last step. Re-measured against Fraction(v) * 10**9 over 20 000 draws in [2.4e8, 2.5e8): max |err| 0 ns, against 16 ns for the old form. Pinned by test_is_exact_to_the_nanosecond_at_icesat2_magnitudes, which checks 2 000 words against exact rational arithmetic.
The float sum survives only as the pre-cast domain-check figure, and that check itself moved to exact Python ints in c3f0ab0e. The committed §7 fixture words are unchanged — test_the_production_kernel_reproduces_the_committed_words still passes.
| # own limits are what the caller needs named. The window is the timestamp | ||
| # variant's own domain — ns since the epoch, below the 63rd bit. | ||
| ns = np.rint(offsets * 1_000_000_000.0) | ||
| ceiling = float(2**63 - 1 - base) |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — the ceiling is not the grammar's domain, so the pointed message doesn't fire in the last 4.29 s of the range. mortie refuses at 2**63 - 2**32, not 2**63 - 1:
>>> mortie.time2toc(np.array([2**63 - 1], dtype=np.uint64))
ValueError: timestamp 9223372036854775807 ns is at or beyond the toc span ceiling (2^63 - 2^32 ns ...)
so for an offset landing in [2**63 - 2**32 - base, 2**63 - 1 - base) this check passes and the caller gets mortie's message instead of the one the comment above promises ("the grammar's own limits are what the caller needs named"). Verified: observation_words with the ICESat-2 epoch and v = (2**63 - 1 - base)/1e9 raises from mortie, not from here. Using 2**63 - 2**32 (or importing the constant if mortie exposes one) restores the intent. test_post_ceiling_time_refused passes either way because 1e18 s is far outside both.
Second, smaller point on the same two lines: base exceeds 2**53, so float(base) / float(ceiling) are only exact when base happens to be a multiple of the local ulp (1024 ns here). An epoch at an odd second — "2018-01-01T00:00:01" — gives base divisible by 512 but not 1024, and the bound then floats by up to 512 ns in whichever direction the cast rounds. Harmless in effect (mortie re-checks the real domain, and a negative internal wraps into the refused band), but the comment claims this check is the domain check, and it is approximate. Comparing in Python ints (int(ns.min()) < -base) costs nothing here since ns is already materialized.
There was a problem hiding this comment.
🤖 from Claude
Folded in c3f0ab0e. The ceiling now comes from mortie rather than being restated: from mortie.toc import TOC_MAX_NS (2^63 − 2^32), with the maximum allowed offset TOC_MAX_NS - 1 - base, since mortie refuses "at or beyond" it. The refusal message reads 2^63 - 2^32 ns instead of 2^63 ns.
Your second point is folded too: both comparisons are Python ints now, built as int(whole[i]) * 10**9 + int(frac_ns[i]) at the argmin/argmax of the offsets, so float(base)'s up-to-512 ns wobble is gone and the check really is the domain check — the "by N ns" figures in both messages are exact as well. Doing it in ints also keeps the int64 combine unreachable for a grossly out-of-domain offset, which was the reason the check ran in float in the first place. Pinned by test_the_ceiling_is_the_grammars_own_span_not_the_naive_63rd_bit, which pushes an offset into the last 4.29 s band (previously mortie's message, now this one) and checks that the band just below still encodes.
| cells_with_data``) — one element more than StreamingAggregator, since | ||
| spill serves located fields. | ||
| spill serves located fields. The §8.3 temporal channel is NOT among them: | ||
| ``validate_spill_fold`` refuses a ``temporal:`` field, so the channel |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — this describes the spill path as not carrying the temporal channel, but the single-block regime does carry it, correctly. validate_spill_fold is a probe here, not a refusal:
try:
validate_spill_fold(config)
self._mergeable = True
except ValueError as e:
self._mergeable = False
self._fold_problems = str(e)A temporal: config therefore constructs a SpillAggregator with _mergeable = False, and chunk_outputs routes to _chunk_outputs_exact → _aggregate_chunk_cells, which emits the full {field: {"locations": [...], "times": [...]}} mapping. The refusal only lands when a block actually closes (SpillOverflowError, carrying _fold_problems) — loudly, so nothing is silently lost; I checked this specifically because a non-mergeable config skipping the fold gate is the failure I most expected here, and it isn't one.
So the accurate statement is stronger than the one written: single-block spill serves both channels exactly; a block close refuses, because the cross-block channel state is located-only. Worth fixing here and in the _chunk_outputs_merged comment at line 1093 ("temporal: never reaches here (refused by validate_spill_fold)") — true of that method, but for the reason that it only runs when _mergeable, not because the config was rejected.
There was a problem hiding this comment.
🤖 from Claude
Folded in fb9217e3. The SpillAggregator docstring now describes the mechanism: validate_spill_fold is a probe here, a temporal: config lands _mergeable = False and routes through _aggregate_chunk_cells, so single-block spill carries times beside locations exactly; what refuses is a block close (SpillOverflowError, carrying _fold_problems) — loudly — because the cross-block channel state is located-only. The _chunk_outputs_merged comment is corrected the same way: temporal: never reaches there because that method only runs when _mergeable, not because the config was rejected. streaming.validate_spill_fold's own docstring already frames itself as a mergeability probe, so it stands as written.
| from zagg.time_axis import TOC_WORD_COLUMN, toc_source | ||
|
|
||
| if toc_source(config) is not None: | ||
| if TOC_WORD_COLUMN in ds_vars: |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — the reservation covers data_source.variables and the segment-level names (both are in ds_vars) and chunk_precompute (line 1723), but not data_source.coordinates. Verified accepted:
cfg.data_source["coordinates"] = {"toc_word": "{group}/toc_word"}
validate_config(cfg) # passes with output.time_source declaredCoordinate columns are read into col_arrays alongside the variables, so they reach cell_data, where {**cell_data, TOC_WORD_COLUMN: ...} overwrites the read column with the derived words — silently, and only for the cells that have observations. Same corruption shape the ds_vars branch above exists to prevent, one dict away. Folding set((config.data_source or {}).get("coordinates") or {}) into the membership test (with the existing message) closes it.
There was a problem hiding this comment.
🤖 from Claude
Folded in 41cc48c2: data_source.coordinates is now in the membership test, and the message names the surface the name was found on (data_source.coordinates declares 'toc_word' ...). Your repro raises. Pinned by test_declared_coordinate_may_not_shadow_the_derived_name, with the overwrite-in-cell_data reason in the comment.
|
🤖 from Claude Blocked on one ruling: how the per-cell toc companion is bound at overview levelsPhases 1–3 have landed the kernel channel, the ingest path, and ruling 4 (located fields fold through the pyramid). Phase 3 stopped exactly at ruling 3 — "per-cell toc range at overview levels, even where leaves are per-centroid" — because implementing it needs a decision I should not make unilaterally. Two coupled questions, both with concrete options. Q1 — What binds an overview's per-cell toc array to its field?At a leaf, §8.3 is unambiguous: the payload array carries a spec-owned So there are three implementable readings, and the store bytes differ:
My read: (a). It is the only one that keeps a reader binding by declaration at every level, the relaxation is confined to the writer-internal template path, and it is what §8.3's per-level clause seems written for. But it does add config surface, so it is a call rather than a detail — and if you prefer (b) I would want a sentence in §8.2 saying a per-cell companion may be reached by convention, so the asymmetry with §8.3 is recorded rather than inferred. Q2 — The fixture regeneration, and the moczarr window it opensWhichever way Q1 goes, reclassifying temporal out of class
§7 records that "moczarr vendors the same fixtures for its parity gates (espg/moczarr#19/#20)" with espg/moczarr#23 as the divergence detector, and that regenerated fixtures need a coordinated moczarr bump in the same window. That is a cross-repo, side-effecting decision, so I am not regenerating anything on my own initiative. What I would do on a green light
Phases 1–3 stand on their own and are complete, folded and green regardless of how this goes. |
|
{ |
| return encode_digest(merge_tdigests_kway(digests, delta=int(delta)), dtype) | ||
| return ( | ||
| encode_digest(digests[0], dtype), | ||
| *(encode_digest(v[0], "uint64") for v in kept.values()), |
There was a problem hiding this comment.
🤖 from Claude (review)
blocking-adjacent / should-fix — fold_digests returns its channel slots in two different orders depending on which arm runs, and a mismatch is a silent morton↔toc swap.
The merge arm delegates the ordering to the kernel, which fixes it by signature:
payload, *words = merge_tdigests_kway(digests, delta=int(delta), **kept)merge_tdigests_kway builds declared from its own literal (("locations", …), ("temporal", …)) tuple, so its return is always (digest, locations, temporal) regardless of kwargs order. But the other two arms iterate the caller's dict:
if not keep:
return (b"", *(b"" for _ in channels))
...
if len(digests) == 1:
return (
encode_digest(digests[0], dtype),
*(encode_digest(v[0], "uint64") for v in kept.values()),
)Probed on this HEAD:
merge arm slot0 identical across dict orders: True
merge arm slot1 identical across dict orders: True
single arm slot0 identical across dict orders: False
-> single-arm reversed slot0 == the LOCATION words? False
-> single-arm reversed slot0 == the TOC words? True
So a caller that builds channels={"temporal": …, "locations": …} gets the toc vector in the locations slot on the single-contributor arm — and every caller zips the result onto field_companions(...) order, so those words land in {field}_locations. Nothing raises: I confirmed the two grammars are mutually accepting (mortie.validate_morton accepts toc words, mortie.toc_is_range accepts morton words), so the swap survives every downstream check and the store's containment claims are simply false.
No live caller violates it today — all five sites build the dict from field_companions, which walks COMPANION_CHANNELS. But the function's own docstring says it length-checks "HERE rather than at the call sites, because this function is the documented seam they must come from together", and it names the single-contributor arm as "the majority case at the finest overview level, where an output cell usually has exactly one populated source". The one precondition whose violation is silent corruption is the one left unenforced. Normalizing through the table makes all three arms agree unconditionally, e.g.
kept = {kw: [channels[kw][i] for i in keep] for _k, kw, _n in COMPANION_CHANNELS if kw in channels}(with the empty arm iterating the same normalized keys), which also removes the zip(declared, words, strict=True) assumption from all four call sites.
There was a problem hiding this comment.
🤖 from Claude
Fixed in db4ebb1. All three arms now normalize through COMPANION_CHANNELS, so the return order is unconditional:
declared = [kwarg for _key, kwarg, _namer in COMPANION_CHANNELS if kwarg in channels]
unknown = sorted(set(channels) - set(declared))
if unknown:
raise ValueError(...)
...
if not keep:
return (b"", *(b"" for _ in declared))
kept = {kwarg: [channels[kwarg][i] for i in keep] for kwarg in declared}
if len(digests) == 1:
return (
encode_digest(digests[0], dtype),
*(encode_digest(kept[kwarg][0], "uint64") for kwarg in declared),
)Three notes on the shape of the fix:
- The length-check loop iterates
declaredtoo, so validation and emission walk the same list. - I added the
unknownrefusal deliberately. Normalizing through the table silently DROPS a key that is not in it, which would return fewer slots than the caller zips — astrict=Trueblowup at best, an off-by-one channel assignment at worst. Rejecting is the honest seam. - The docstring now states the ordering guarantee and why (the k-way merge fixes its own order by the same table; the grammars are mutually accepting so a swap raises nowhere).
I kept zip(declared, words, strict=True) at the four call sites. It is no longer an assumption — field_companions walks the same table, so the pairing is guaranteed by construction — and it is still the thing that maps a returned word to its sibling ARRAY NAME, which the call sites need regardless. Dropping the strict=True would only remove a free assertion.
Tests in 7e0a6a0 (see the reply on the coverage finding). Two go straight at this: TestOverviewFoldDigests::test_slot_order_is_the_table_not_the_callers_dict parametrized over 0/1/3 contributors — all three arms, forward and reversed dict order, asserted byte-identical — and test_an_unknown_channel_is_refused_not_dropped. Plus four end-to-end tests that fail under the swap: I verified by mutating declared to [::-1] on this HEAD, which reproduced exactly the bug you describe (the merge arm is unaffected, the single arm swaps) and took down TestBothChannelsOverviewFold::test_singleton_centroids_round_trip_both_words_unswapped[single-arm], ::test_merged_centroids_contain_their_members_in_both_grammars, ::test_cascade_folds_both_siblings and TestBothChannelsColumn::test_the_fold_returns_both_siblings_against_their_own_oracles.
| for _kwarg, sibling in declared: | ||
| arrays[sibling] = reader.read(res, sibling) | ||
| present = [k for k, v in arrays.items() if v is not None] | ||
| if not present or len(present) == len(arrays): |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix (test strength) — the whole point of this phase is N channels on one field, and no test in the suite ever puts two channels on one field through any sweep_overview or sweep_stage fold path. Every fold-path test is location:-only:
tests/test_sweep_overview.py—LOCATED_FIELDS_DECLsets only"location": "leaf_id";TestLocatedOverviewFold/TestLocatedDeclarationGatenever declaretemporal.tests/test_sweep_stage.py—LOCATED_FIELDS/TWO_LOCATED_FIELDSlikewise. NoteTWO_LOCATED_FIELDSis two located fields (h_tdigest,h2_tdigest), which pins_close's per-field rebinding, not two channels on one field.tests/test_column.py—LOCATED_FIELDSislocation:only.grep -rn 'channels=' tests/returns exactly three hits, all{"locations": …}single-key.
So _companion_group's new partial rule is never exercised at its interesting arity. With one channel arrays has 2 keys and the rule degenerates to the old (payload is None) == (words is None); the branch this phase actually added is len(present) == 2 out of 3 — payload + one sibling present, the other absent — and nothing reaches it. test_a_gather_source_without_its_channel_is_not_written_half and test_a_merge_source_without_its_channel_is_skipped_not_raised drop the only sibling there is, i.e. they still test the 1-of-2 shape.
I wrote the probes rather than assume: all of it works, so this is coverage, not a bug. For the record, on this HEAD with {"location": "leaf_id", "temporal": "per-centroid"} on h_tdigest:
# _fold_node / pyramid sweep (harness = test_sweep_overview._make_located_leaf + a times sibling)
failed: 0
payload rows (4, 2) loc (4,) times (4,)
loc anc match: True # common_ancestor(col) == common_ancestor(leaf words)
time envelope match: True # cell_envelope(col) == cell_envelope(leaf words)
# _gather_slabs / _merge_slabs ladder (harness = test_sweep_stage._stage_store)
ladder: 7 written, 0 failed
gather level rows: 2 aligned merge level rows: 1 aligned
# the partial-group branch: rmtree(3/h_tdigest_times), locations left in place
partial ladder: 7 written, 0 failed
gather counts: {'folded': 1, 'missing': 0, 'unreadable': 1}
payload[0] empty: True
Parametrizing TestLocatedStageSweep and TestLocatedOverviewFold over a BOTH_FIELDS declaration is ~40 lines against harnesses that already exist, and it is what would catch a channel dropped, mis-paired, or folded separately at these five sites — which is the exact failure mode the module docstrings cite as "silent corruption". Related: test_temporal_per_centroid_is_approximate also carries "location": "leaf_id", so no test pins the temporal-only per-centroid shape (which is what a build_tdigest field with a clock and no location: is).
There was a problem hiding this comment.
🤖 from Claude
Covered in 7e0a6a0 — a both-channel field now folds end to end at all five sites, and both siblings are asserted against their own grammar.
tests/test_sweep_overview.py — BOTH_CHANNEL_FIELDS_DECL and TIMED_FIELDS_DECL; _located_leaf_cfg / _make_located_leaf take location= / temporal= and build both siblings in ONE build_tdigest call. New TestBothChannelsOverviewFold:
test_both_siblings_are_emitted_row_aligned_and_declared— both arrays present, both row-aligned with the payload, each declaring its own convention (zagg-located/1vszagg-toc/1), and the payload binding both by name (ragged.locations+ the top-leveltimesattr).test_singleton_centroids_round_trip_both_words_unswapped[single-arm|merge-arm]— parametrized so one populated leaf row takesfold_digests' single-contributor arm and two take the merge arm. Every centroid is a singleton at δ=64, so BOTH channels must reproduce their contributor's exact word:assert_array_equalagainst the leaf truth, per channel. This is the decisive anti-swap pin.test_merged_centroids_contain_their_members_in_both_grammars— 400 obs, real merges; per centroid,common_ancestorcontainment (§9.1) AND toc coverage (§8.3) against the same weight-derived partition, plus the negative (_toc_contains(locs[0], ...)is false).test_cascade_folds_both_siblings— the fold-of-folds path, checked at cell granularity (cell_envelopeidentity and thecommon_ancestoranalogue).test_temporal_alone_folds_through_the_pyramid— your last point: abuild_tdigestfield with a clock and nolocation:, which nothing pinned.
tests/test_sweep_stage.py — BOTH_CHANNEL_FIELDS (one field, two channels — distinct from TWO_LOCATED_FIELDS) and TIMED_FIELDS; _located_leaf_slabs generalized to write whichever siblings a field declares. New TestBothChannelsStageSweep: the ladder row-aligned at a gather and a merge level; the gather relaying each channel into its own slot byte-for-byte; the merge satisfying both containment claims with the cross-check that the located bytes do NOT satisfy the temporal one; test_one_channel_missing_refuses_the_whole_group — _companion_group's new branch at len(present) == 2 of 3, which is what you flagged as unreachable: it asserts the payload AND the intact sibling both stay fill and source_children == {folded: 1, missing: 0, unreadable: 1}; and the timed-only ladder.
tests/test_column.py — BOTH_CHANNEL_FIELDS, _located_cell_slabs(cells, fields), TestBothChannelsColumn: both siblings byte-equal to the corresponding output of the SAME merge_tdigests_kway call on the pooled row, plus the single-contributor row passing each channel's own bytes through, leaf_slabs picking up both, and the by-name refusal for a missing _times slab.
Mutation-checked rather than assumed: with declared reversed in fold_digests (the exact bug in the other finding), four of these fail — three in TestBothChannelsOverviewFold and TestBothChannelsColumn::test_the_fold_returns_both_siblings_against_their_own_oracles. Full suite green apart from the pre-existing test_lambda_build::TestFunctionBuild::test_function_build_succeeds; no committed fixture byte moved (git status tests/data/ empty).
| words = np.frombuffer(bytes(arr[:][i]), "<u8") | ||
| assert words.shape == (rows,), f"{group}/{name} row {i}" | ||
| # §8.2's reserved 0 never appears in a real word. | ||
| assert words.all() |
There was a problem hiding this comment.
🤖 from Claude (review)
should-fix (test strength) — this is the only golden for a merge-produced companion (the §7 paragraph added in this phase says exactly that: "the fixture set's only golden for a companion produced by a merge rather than by ingest"), but its assertions cannot tell a correct fold from a swapped or wrongly-folded one.
The two content assertions are words.shape == (rows,) and words.all(). Both hold under a straight swap of the two committed sibling chunks — same length, same nonzero-ness — and the declaration checks above are on attrs, which don't move when the bytes do. I checked whether the grammars discriminate; they don't:
4 row 0: rows=19
validate_morton(loc)=accepts validate_morton(times)=accepts
toc_is_range(loc)=accepts toc_is_range(times)=accepts
times decode -> 2019-05-14T02:11:05.877 .. 2019-05-14T02:40:18.223
LOC-as-times decode -> 1869-07-21T02:59:08.949 .. 1869-08-15T18:42:44.391
A morton word decodes as a perfectly well-formed (1869-era) toc word and vice versa, so no validator in the stack raises. Compare the leaf, which is pinned two independent ways — test_words_conservatively_contain_their_members (containment against obs_span_ns) and test_the_production_kernel_reproduces_the_committed_words (word-for-word against the reducer). The folded column gets neither.
The committed bytes are right — I verified independently, and this is the check that belongs in the test:
res 5: 4 cells, factor 4
cell 0: envelope leaf==col: True loc common_ancestor leaf==col: True
cell 1: envelope leaf==col: True loc common_ancestor leaf==col: True
cell 2: fill
cell 3: envelope leaf==col: True loc common_ancestor leaf==col: True
res 4: 1 cells, factor 16
cell 0: envelope leaf==col: True loc common_ancestor leaf==col: True
i.e. int(cell_envelope(column_times[j])) == int(cell_envelope(concat(leaf_times[j*f:(j+1)*f]))) — the §8.3 cell-level identity _centroid_envelopes' docstring names as the invariant that survives an arbitrary fold tree — and the mortie.common_ancestor analogue for the located slot. Two lines per resolution, and they make the channel identity load-bearing.
(The rest of the fixture regeneration checks out: git diff 351741fb...HEAD -- tests/data/spec/temporal/ touches no leaf array byte — only 11213.zarr/zarr.json's written_at and morton_hive.json — FROZEN_COMBINED/FROZEN_ARRAYS/FROZEN_COMBINED_COLUMN were last edited in f1fbb5e6 (phase 3) so nothing was re-pinned, the committed semantic_hash is unchanged, and re-running tools/generate_spec_fixtures.py --only temporal reproduces all six new column chunks byte-identically.)
There was a problem hiding this comment.
🤖 from Claude
Added in 0f153a9 — TestTemporalCompanions::test_the_columns_words_reproduce_the_leafs_cell_by_cell. It is exactly the check you verified: per column cell, per channel, reduce the column's words and the leaf words under them and compare.
reducers = {
"h_tdigest_locations": lambda w: int(common_ancestor(w)),
"h_tdigest_times": lambda w: int(cell_envelope(w)),
}
for group in sorted(...):
factor = 4 ** (leaf_order - int(group))
...
assert reduce(got) == reduce(members), f"{group}/{name} cell {j}"with members = concat(leaf[name][j * factor : (j + 1) * factor]), an unpopulated column cell asserted to have had no members (so an empty row cannot hide a dropped fold), and a checked counter so the loop cannot pass vacuously. It runs over both committed groups (5 at factor 4, 4 at factor 16) and both channels — 10 populated (cell, channel) pairs.
You are right that this closes the ordering gap from the fixture side: the two reducers are channel-specific, so a swapped pair fails. I checked that rather than assuming it — pointing got at the other channel's chunk while keeping members and the reducer makes the test fail. It is the only golden for a merge-produced companion, so it is worth it being load-bearing for moczarr.
No fixture byte moved (git status tests/data/ empty) — this is a test-side addition against the bytes already committed.
| the value co-sort carry along so each output centroid gets the envelope of | ||
| the samples that survived into it | ||
| (:func:`zagg.stats.tdigest._centroid_envelopes`). Given, the return is a | ||
| ``(digest, words)`` pair. Per-centroid at every level, symmetric with the |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — "Per-centroid at every level" reads as a statement about this reducer's stores, and it can't be: build_waveform_digest is not in zagg.processing.streaming._TDIGEST_FUNCTIONS, which is the allowlist semantics.field_composability's ragged branch tests, so a waveform field is composability class none and never appears above native resolution at all.
>>> field_composability({"kind": "ragged", "function": "zagg.stats.waveform.build_waveform_digest",
... "inner_shape": [2], "temporal": "per-centroid", "dtype": "float32"})
'none'
>>> field_composability({**_, "function": "zagg.stats.tdigest.build_tdigest"})
'approximate'That is consistent with the GEDI template's pyramid: false, and it is presumably deliberate — but as written the sentence invites a reader to expect a rx_flux_times sibling on a GEDI overview, and there will never be one. Suggest scoping it: the ruling is per-centroid at every level; this reducer emits the leaf's words, and a waveform store has no overview levels to carry them. (The honesty property below — one shot ⇒ exact timestamps — is well grounded and pinned by TestGediTemplate::test_template_reads_and_aggregates_the_fixture, so only the "at every level" clause is at issue.)
There was a problem hiding this comment.
🤖 from Claude
Corrected in cf699e0. The "at every level" clause is scoped to what is true for this reducer:
The per-centroid shape is the one the espg ruling of 2026-08-17 makes universal, identical to the located channel's; the ruling's at every level half does not describe this reducer's stores, because
build_waveform_digestis absent fromzagg.processing.streaming._TDIGEST_FUNCTIONS— so a waveform field is D24 classnone(issue #422, and the GEDI template'spyramid: false) and exists at native resolution only. There is no waveform overview to carry a companion, and a reader must not expect one.
The honesty property is unchanged, moved to its own paragraph. _TDIGEST_FUNCTIONS left alone per your note.
Also pinned the classification so the corrected sentence cannot drift back: TestFieldComposability::test_a_temporal_waveform_field_is_still_none asserts field_composability is none for a build_waveform_digest field carrying temporal: per-centroid, and approximate for the same declaration under build_tdigest — i.e. the class turns on the function, not on the channel.
There was a problem hiding this comment.
🤖 from Claude
Correction to my reply just above: the new test is TestComposabilityClasses::test_a_temporal_waveform_field_is_still_none, not TestFieldComposability — I named the class from memory. Same test, same assertions, in cf699e0 alongside test_temporal_per_cell_stays_none.
| # the nanosecond while a centroid is one photon. Per-centroid at EVERY | ||
| # pyramid level, symmetric with the located channel (espg-ruled | ||
| # 2026-08-17). Stored as the {field}_times sibling, bound from the | ||
| # payload's `times` attr; read it back with zagg.readers. |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — "read it back with zagg.readers" overstates what the reader package offers for this channel. zagg.readers exports read_locations, which binds the §9 sibling from attrs["ragged"]["locations"]; there is no counterpart that binds the §8.3 times key, and read_locations raises by name for a field that "declares no locations channel". The only route today is read_raw_values(store, "h_tdigest_times", cell) — which does work, since the sibling is an ordinary ragged uint64 array — but a reader following this comment will look for a times helper and not find one.
Relatedly, src/zagg/readers/tdigest_tensor.py's on-disk layout header still documents only the located sibling:
{group}/{field}_locations <- located fields only (issue #87): the uint64
...
"locations": "<sibling name>"} # located fields only
so the reader module's own description of the layout is now incomplete for the stores the two shipped templates write. Either point the comment at read_raw_values explicitly, or note the sibling in the reader header — whichever, so the yaml and the reader agree.
There was a problem hiding this comment.
🤖 from Claude
Corrected in 8cc8bb5 — and one correction to the finding: read_raw_values does not work on the sibling either. I tried it against the committed §7 fixture before writing the comment:
>>> read_raw_values(LocalStore(leaf), "6/h_tdigest_times")
File "src/zagg/readers/tdigest_tensor.py", line 982, in read_raw_values
weights = np.asarray(digest[:, 1], dtype=np.float64)
IndexError: too many indices for array: array is 1-dimensional, but 2 were indexed
It decodes a (n, 2) digest and gates on centroid weights, so a flat (n,) word vector has no route through it. So there is no reader path for this channel at all today, and the comment now says that instead of naming a substitute:
NOTE zagg.readers has no helper for this channel yet: read_locations binds the section 9
ragged.locationskey only, and read_raw_values decodes a (n, 2) digest, not a flat word vector. Until one exists, decode the sibling directly -- it is an ordinary ragged uint64 vlen array whose element dtype/shape its ownraggedattrs declare -- and interpret the words with mortie (toc_is_range, toc2time).
And the reader module's layout header now documents the sibling and the gap: {group}/{field}_times added to the layout block, attrs["times"] = "<sibling name>" added to the attrs block, plus a paragraph recording that no helper here binds the channel yet and why neither existing function substitutes.
Left standing for @espg (per the fold rules — a new public reader is scope, not a fold): I do think a read_times is wanted, and the natural shape is a channel= parameter on read_locations rather than a third near-duplicate sweep, since the two differ only in which attrs key binds the sibling and how the words are interpreted. Both shipped templates now write the sibling, so a client following the §8.3 docs has nothing to call.
|
🤖 from Claude Ready for review — all phases complete, CI greenHead Phase 4 under the (c) rulingRuled option (c) — companions per-centroid at every level, symmetric with located, §8.4's reduction licensed-but-unused. What it bought, beyond matching the ruling: it made the code smaller than the alternative. No second shape to produce, no per-cell overview array, and the binding question I was blocked on simply vanished — there is nothing to bind, so §8.2's missing binding grammar never had to be closed. The plumbing was generalized rather than duplicated. Phase 3 had threaded one channel through five fold sites with a bespoke pair at each; adding a second by copy would have doubled that. Instead A Fixture regeneration
That also closed a standing question for free: this is the fixture set's first golden for a companion produced by a merge rather than by ingest, now pinned by value — per column cell, Cross-repo, not executed here: moczarr re-vendors this fixture after merge (espg/moczarr#19/#20, detector #23) — the #43 re-check is the natural vehicle. Different repo, so flagged rather than actioned. The GEDI companion is GPS-scale, verified
Ruling 2's honesty property survives, expressed per centroid: all of a shot's samples share its instant, so a single-shot cell's centroids are exact timestamps and only genuinely pooled cells produce ranges. All six questions resolved
The one standing itemNo reader binds the temporal channel. The shape of the fix is a StateFour phases plus the P0 measurement, 41 commits. 28 review findings across four adversarial passes, 27 folded, 1 declined (a new §7 fixture for a located pyramid — it needed the same cross-repo decision, so doing it inside a fold would have landed that by side effect); every thread has a reply. Three syncs onto Local: 4,433 passed, 38 skipped, 1 failed — |
1 participant
Refs #410 (the issue stays open for the fleet rebuild it gates).
The kernel + pipeline half of the temporal/located companion work. PR #456 landed the spec half —
docs/specification.md§8 (zagg-toc/1) and §9 (zagg-located/1) plus the §7 conformance fixture attests/data/spec/temporal/— and deliberately landed the declaration surface ahead of any producer, withtime_axis.TOC_PRODUCING_FUNCTIONSempty andconfig._validate_temporal_producerrefusing everytemporal:outright. This PR builds the producer and lifts that gate.Implemented against the merged spec text, which post-dates parts of the consolidated plan — where they differ the spec wins.
Approach
One companion mechanism, two channels. Post-#279 the vectorized t-digest kernel already threads a per-centroid uint64 channel through the merge:
_compressreturnsstarts(the partition map from inputs to output centroids) and_centroid_ancestorsreduces the located morton words over it.temporal=becomes the second channel through the same seam —_centroid_envelopes, reducing per-observation toc words over the samestartsvia mortie's segmentedtocs_reduce.startsis already the arrow-offsets layout that call takes, so a whole cell's partition crosses into Rust once, with no per-centroid Python loop (the located channel still needs one for its multi-member centroids).The temporal channel's law is stronger than the located one's, and the code says so. The digest payload is order-dependent (§2.3) and the located ancestor reduction is exact only given a centroid partition; the toc join is a semilattice, so a cell-level reduction is bit-identical under any fold tree (§8.2). That is what licenses §8.4's
per-centroid→per-cellcoarsening at overview levels, and it is pinned bytest_cell_envelope_is_fold_tree_independent.Phases
temporal=besidelocations=inbuild_tdigest/build_tdigest_where/build_tdigest_pairwise/merge_tdigests/merge_tdigests_kway;_centroid_envelopesviamortie.tocs_reduce; the quaternary tie key;zagg.stats.toc(the per-cell reducer + its segmented sibling);mortie>=0.9.9.output.time_source(the per-observation clock),time_axis.observation_words(the encode), the derivedtoc_wordcolumn,aggregate.pywiring for both companion shapes, theTOC_PRODUCING_FUNCTIONSgate lift, and named refusals on the two streaming paths. Pinned againsttests/data/spec/temporal/by a production-parity test.semantics.pyreclassification, the channel threaded through every fold site (sweep_overview.fold_digests/_fold_node/_cascade_node/_fold_child,sweep_stage._gather_slabs/_merge_slabs), the overview template, and the retrofit gate. Fixture-neutral — verified, see below.temporal/fixture regenerated.Phase 3 detail — ruling 4, and why it was a bug
semantics.field_composabilityclassified a located ragged fieldnone, andpyramid.pyputsnonefields in the excluded list, whichdocs/specification.mdmakes normative: "exactandapproximatefields appear,nonefields are absent." So declaringlocation:on a digest field removed that digest from every overview level — the hole diagnosed on the issue thread ("the location companion is a leaf-and-spill artifact only"), and what ruling 4 calls "the bug, not the design".The fold law already existed —
merge_tdigests_kway(..., locations=)has carried the channel since #279. What was missing was the wiring, at five sites:fold_digests(..., locations=)(payload, words)pair_fold_node{field}_locationsinside the payload's guarded block, accumulates index-parallel with the digests, folds the pair per cell_cascade_node/_fold_childsweep_stage._gather_slabs/_merge_slabs_overview_config/_field_driftThe pair is never folded in two passes. Spec §9.1/§2.3 make the words exact only given the centroid partition the merge produced, so a payload folded without its channel — or with it — would describe different partitions. Every site therefore reads both or neither: a leaf carrying the payload but no sibling is skipped loudly (
failed += 1), never folded with the channel silently dropped, whichtest_a_leaf_without_the_sibling_is_skipped_loudlypins.Heterogeneous orders are the point, not an accident. §9.1: "A fold coarsens only as far as its contributors force, so one overview array's words routinely carry different orders." An unmerged centroid keeps its order-29 point word beside a merged centroid's coarse area word.
Nothing changed for an unlocated field. The
locationkey is recorded keyed-only-when-set, andtest_unlocated_fields_are_byte_identicalasserts an unlocated overview has no sibling and nolocationsbinding.Fixture impact: none, and that is checked rather than claimed
git status tests/data/is empty on the phase-3 commit. The reason is specific: the only committed fixture field that is both located and folds under_TDIGEST_FUNCTIONSistemporal/h_tdigest, whosetemporal:declaration still forcesnone;kitchen_sink's located strata fields usebuild_tdigest_where, which has no_TDIGEST_FUNCTIONSlaw. Reclassifying temporal is what moves committed bytes, and that is exactly the blocked half.Module size
sweep_overview.pyis 2,023 lines after this phase (1,908 before, onmain). CLAUDE.md §4's 1,200-line raise trigger was crossed on that file by prior merged work; this diff is +115 lines of threading inside existing functions rather than new surface, so it is not the change that should trigger the split — flagging it rather than acting on it.Phase 3 coverage
tests/test_sweep_overview.py::TestLocatedOverviewFold— six cases: the leaf fold writes the sibling row-aligned andvalidate_morton-clean; every overview word contains its contributors (§9.1, checked against the real member words per centroid via the digest's own weights, with a kind-aware containment helper — an order-29 point word folded with itself yields its cell's area word, so the naive comparison would be wrong); the sibling carries the §9 declaration and the payload binds it by name; the cascade folds the sibling too; a leaf missing the sibling is skipped loudly; and an unlocated field is byte-identical. Plus the flippedTestComposabilityClassescases (located →approximate, temporal stillnone) and the manifest-entry test.Phase 2 detail — where the nanoseconds come from
This is the phase that answers open question Q4 on the issue ("Where the per-observation nanoseconds come from"), and it answers it as that comment's option (b): a declaration of its own, requiring a continuous scale and rejecting
scale: utc.output.time_source— one clock per store.Four decisions in it, each with its reason:
tocstart quantum reads as outside W. Cheap to prevent by construction (derive both from one conversion), expensive to discover later" — is prevented by construction: an absent block falls back tooutput.windowingwhenever that block already carries a continuous-scale clock, so a windowed store has exactly one declaration feeding both window routing and toc ingest.scale: utcis refused by name. §8.3 requires an instant "exact to the nanosecond"; a nominal-UTC offset column is only good to the leap seconds elapsed since its epoch (windows.py's documented ≤ 1 s tolerance) — a full quantum wider than the range variant's ~2.15 s start grid is meant to imply. Window routing tolerates that; a word claiming nanosecond exactness cannot. Autcwindowing block therefore does not serve as the fallback either.observation_wordstakes its pre-2017 scale-vs-UTC correction fromwindows.utc_to_offsetrather than recomputing it, so the two sides cannot drift:test_agrees_with_the_window_router_at_a_boundarydecodes a routed boundary instant's word back to the very instant the router converted.One conversion point, both shapes.
output.time_sourcematerializes a derived per-observation column,toc_word— the toc analogue of the HEALPixleaf_idmorton column, and reserved the same way (a config may read it, never declare it). Both companion shapes consume it:temporal: per-centroid—aggregate.pypasses it as the reducer'stemporal=channel, exactly as it passesleaf_idaslocations=;temporal: per-cell— the field declaressource: toc_wordandfunction: zagg.stats.toc.cell_envelope, so the config reads honestly (the field is derived from that column) with no magic name.It is derived in
calculate_cell_statisticsrather than in the read path deliberately: that is the single funnel every route into the aggregation passes through (pooled, spill read-back, chunk-precompute), and per-cell derivation makes the total encode work exactly one pass over the shard's rows.Gate lift.
TOC_PRODUCING_FUNCTIONSis populated, and split into halves that partition by shape —TOC_PER_CELL_FUNCTIONS(whole-cell reducers) and the rest (digest kernels carrying the channel). A crossed declaration is therefore named as such rather than validating and writing a wrong-shaped array:Two named refusals rather than two silent gaps.
temporal:is refused under bothaggregation.streamingmodes, mirroring howvalidate_streamingalready refuses a located field undermode: merge:mode: merge— the running merged state carries no companion channel at all, so the §8.3 sibling would be stamped and left empty;mode: spill— the words themselves fold exactly (the §8.2 join is associative, commutative and idempotent), but the block close's per-field channel state is located-only, so a temporal field would emit a companion missing every block past the first. That is a §8.3 row-alignment break, not an approximation, so it refuses and names the pooled path. Threading the channel through the block close is follow-on work, not a silent hole.The
ragged_locations→ragged_channelscontract. The aggregation stage's 5-tuple keeps its arity; its fourth element becomes{field: {channel: [per-cell words]}}instead of a locations-only mapping — the "generalize the channel first" shape from alternative (B) on the thread, taken as far as this PR needs it.write._channel_entrytrims trailing absent channels, so a pre-#410 config still produces the historical 2-tuple sink entry and a located-only config the historical 3-tuple — byte-identical, not merely equivalent.Fixture parity (CLAUDE.md §4)
tests/data/spec/temporal/was committed in #456 with its words computed in the generator and handed to the production write path, because no reducer produced them yet. Rather than regenerate anything, this PR pins the production path to those committed bytes:tests/test_spec_conformance.py::TestTemporalCompanions::test_the_production_kernel_reproduces_the_committed_wordsimportstools/generate_spec_fixtures.py(so the clock, the RNG and the cell plan cannot drift from the fixture they check), drives the generator's own inputs through the real reducers —build_tdigest(..., locations=, temporal=)andzagg.stats.toc.cell_envelope, with the words coming fromobservation_words— and asserts the digest, the located words, the per-centroid words and the per-cell word for all four populated cells. All four match, including the 1-observation cell that commits the exact-timestamp variant.No committed fixture byte has moved in phases 1–2.
Testing
pytest -vgreen locally except two pre-existing failures unrelated to this diff, both flagged rather than fixed (CLAUDE.md §4):test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds(environmental — needs the container build toolchain) andtest_client_transport.py::TestStatusPoller::test_invoke_fault_burns_an_attempt_and_retries(wall-clock poller assertion). Note also thatruff format --check src testsreportstests/data/benchmark/README.mdas unformatted onmain— pre-existing, untouched here.Phase-2 coverage:
TestTimeSource(15 cases: the resolved shape, the windowing fallback and itsutcnon-fallback, every refusal, and thetoc_wordreservation from all three directions),TestObservationWords(12: the timestamp-never-range MUST, round-trip to the declared instants,units: days, the epoch floor and the ~2142 ceiling, non-finite refusal, and the window-router boundary agreement),TestTemporalCompanionProduction(16: both shapes out of one conversion, per-centroid containment, empty-cell channel arity, a temporal-only field, the words tracking the declared clock rather than the field source,_aggregate_chunk_cells' channel collection,_channel_entryarities), the rewrittenTestTemporalShapeDeclaration(the lifted gate, the shape↔reducer partition, the clock requirement, thetemporalkwarg reservation), and the three streaming/spill refusals.Phase 1 detail
src/zagg/stats/tdigest.py(digest, locations, temporal). Either channel may be declared alone; the digest bytes are identical whichever are.merge_tdigests_kway):lexsortgains one key per declared channel — location tertiary, toc quaternary — so a permutation of the inputs returns the same companion vectors, not just the same digest.lexsorttakes keys least-significant-first, hence the reversal in the call.0word is refused on input. §8.2 reserves0as the unobserved-cell marker and no encoder produces it, so a zero reaching a fold is a leaked fill value — and the join would happily absorb it and return an envelope reaching back to the grammar's 1850 epoch. This is the toc analogue of the validation the located channel gets for free fromcommon_ancestor(which raises on a zero word).zagg.stats.toc(new, 100 lines):cell_envelopeis thetemporal: per-cellreducer (§8.2 — one cell's observations in, one word out, raising rather than inventing an identity for an empty cell), andcell_envelopesits segmented sibling, which is §8.4's licensed shape-coarsening reduction and what phase 3's overview fold will call.Dependency floor:
mortie>=0.9.8→>=0.9.9.tocs_reduce(espg/mortie#177) is released in 0.9.9.deployment/aws/build_layer.shderivesMORTIE_SPECfrom this block per issue #322, sopyproject.tomlis the only edit — nothing underdeployment/is touched.Phase 1 coverage
tests/test_tdigest.py::TestTemporalChannel— the digest is byte-identical with and without the channel; singleton centroids round-trip exact nanosecond timestamps (toc_is_rangeis false across the whole vector, the §8.3 never-widen-an-instant MUST); merged centroids' envelopes contain their real members, asserted against the member instants rather than assumed — with a range's end treated as exclusive and a timestamp's two bounds as its instant (§8.1); NaN values drop their words; the reserved0is refused; both channels together return in the documented order and each is byte-identical to declaring it alone.tests/test_tdigest.py::TestTemporalMergeLaws— permutation independence of the k-way fold over the channel; the two channels are independent of each other; fold-tree independence at cell level; andtest_per_cell_envelope_is_the_envelope_of_the_per_centroid_words, which pins §8.3's closing clause directly (an overview's per-cell word IS the envelope of the per-centroid words beneath it, and equally of the raw observations).tests/test_stats_toc.py— the per-cell reducer: one observation and many observations sharing one instant both stay timestamps (the GEDI shot-pooling case, ruling 2), spread observations become a conservative range, order independence, empty raises, reserved0refused, and it resolves throughconfig.resolve_function.tests/conftest.pygainstoc_words, the temporal sibling ofpoint_words, on the same clock as the §7 fixture generator.Phase 4 detail — the ruled shape, and what it simplified
Ruled option (c): companions are per-centroid at every level, symmetric with located. The consequence worth stating is that this made the code smaller than the alternative, not larger — there is no second shape to produce, no per-cell overview array, and no binding grammar to invent.
The plumbing was generalized rather than duplicated. Phase 3 had threaded
locationsthrough five fold sites with a bespoke pair at each. Adding a second channel by copy would have doubled that. Insteadfold_digests(..., locations=)becamefold_digests(..., channels={kernel kwarg: [per-digest vectors]}), driven by one table:field_companions(name, meta)returns(kwarg, sibling)per declared channel in that fixed order — the kernel's own tuple order — so every site zips the merge's extra returns onto siblings positionally, and the order lives in exactly one place.check_located_matchbecamecheck_companion_match(attrs, field, kwarg), whose temporal arm additionally pins the shape toper-centroid: aper-cellblock reaching a per-centroid fold would decode one word per cell as a vector and produce envelopes whose containment claim is false, and that is a store this fold has no law for rather than an arithmetic error to discover later.A
temporal: per-celldense field stays classnone— the one sub-decision I made rather than read off the ruling, flagged as such in the ruling reply. Its fold law is the grammar's join over a cell group, not its own reducer, so classifying it byfunctionwould fold e.g.nanmaxover toc words. The §8.2 shape still works at a leaf (that is GEDI'sobserved-style companion, and the fixture still commits it); only its pyramid behavior is left unwired, which no shipped config needs since GEDI declarespyramid: false.GEDI's companion keeps ruling 2's honesty property, expressed per centroid.
build_waveform_digestgained the channel: the clip mask and the value co-sort carry the words along (temporal[keep][order]) before reducing over the partition_compressreturned — never reindexed afterwards, which would misalign it. All of a shot's samples share its instant, so a single-shot cell's centroids are exact timestamps and only genuinely pooled cells produce ranges.tests/test_read_vlen.pypins exactly that:toc_is_rangefalse across the vector, one distinct word, decoding to2018-01-01T00:00:01fordelta_time[0] == 1.0.One validator widened.
_validate_time_sourcehad checkeddata_source.variablesalone, which refused GEDI's shot-rate clock. Base-rate means one value per observation after the read, and a broadcast level variable qualifies — so the accepted set is now the same one every other column reference validates against (variables+_segment_variable_names). This is the narrow, real form of the ds_vars question the phase-2 review raised.Adversarial review + fold (CLAUDE.md §2)
Three review passes, one per phase, each a fresh-context subagent posting inline findings, each folded by a separate subagent — 23 findings, 22 folded, 1 declined, every thread replied to. The three that changed behavior rather than prose:
observation_wordsdouble-counted the leap offset —mortie.from_datetime64is already leap-aware, sobasecarries the scale correction and subtractingwindows.utc_to_offseton top made a pre-2017-epoch word 18 s (gps) / 37 s (tai) wrong, and 18 s away fromwindows.offset_to_utcc602d1f8); the pre-2017 branch now has a test that fails without it (e6e6221f). Shipped 2018-epoch configs were unaffected —shift_swas 0 there — so no committed byte moved_overview_config's newlocationkey made the §4.6 leaf column write a bound but empty location siblingcolumn.leaf_slabs/fold_columnnow carry the pair (3fb8a21c)sweep_stage._gather_slabssilently wrote a payload without its channel, where_merge_slabsraised on the same conditionunreadable, leaving both halves at fill (0e2ccd73) — the stage sweep is soft-barrier everywhere else, and theraisewas the one thing that could take a whole level down on the expected case of a column predating alocation:additionAlso folded:
np.rint(offsets * 1e9)was up to 16 ns off the nearest nanosecond at ICESat-2 magnitudes, against §8.3's exact-to-the-nanosecond MUST — replaced with a floor/frac split combined in int64, measured at 0 ns againstFractionarithmetic (480ee86b); the two clock declarations are now cross-checked so a store cannot carry two (f666ec7c); the derivedtoc_wordcolumn no longer validates as asourceon stores that never materialize it (ea9d3297);fold_digestsgained a row-alignment check on its single-contributor arm — the common path, and the one that made the two blocking bugs silent (667654c3);check_located_matchnow gates the fold at both sites (aa195cf9); and the docstring overclaim that per-centroid words are "bit-identical under any fold tree" was corrected to the cell level, which is the only place §8.3 supports it (e1703a50).Phase 4's round
Five findings, all five folded, every thread replied to. The one that mattered was latent rather than live, and is the class this process exists for:
fold_digestsreturned its channel slots in two different orders depending on which arm ran (db4ebb17). The merge arm delegates ordering tomerge_tdigests_kway, which fixes it by its own literal table; the empty and single-contributor arms iterated the caller's dict. Sochannels={"temporal": …, "locations": …}would have put toc words into{field}_locations— on the majority path at the finest overview level — and becausemortie.validate_mortonaccepts toc words whilemortie.toc_is_rangeaccepts morton words, the swap survives every downstream check and silently falsifies the store's containment claims. No live caller tripped it (all five build the dict fromfield_companions), but the function's own docstring claims to be the seam that enforces exactly this. All three arms now walkCOMPANION_CHANNELS, and an unrecognized kwarg is refused rather than silently dropped (normalizing through the table would otherwise return fewer slots than the caller zips).7e0a6a08, +504 lines across three test modules): the phase's whole point is N channels on one field, and nothing exercised two. NewTestBothChannelsOverviewFold/TestBothChannelsStageSweep/TestBothChannelsColumn, plus a slot-order test over 0/1/3 contributors × forward/reversed dict. Mutation-verified rather than assumed — reversing the order reproduces the reported bug and fails 4 of the new tests across two modules. The stage tests also reach_companion_group's 2-of-3 branch, which was unreachable before.0f153a94): it asserted shape and non-zero-ness, both of which survive exchanging the two sibling chunks. It now pins the column's words by value against the leaf cells beneath them —cell_envelopeidentity per channel and thecommon_ancestorhull — with acheckedcounter so it cannot pass vacuously.noneand has no overview levels (cf699e0a), and the ATL03 yaml promised a reader that does not exist (8cc8bb50— see the standing item above).Questions for review
One new item, surfaced by the phase-4 fold — no reader binds the temporal channel.
There is no read path for
{field}_timesat all.readers.read_locationsbinds theraggedblock'slocationskey only, andread_raw_valuesis not an alternative — it indexesdigest[:, 1]and raisesIndexErroron a flat word vector (verified against the committed §7 fixture). So the channel is written, declared, folded and conformance-pinned, but a consumer has to open the sibling by hand.That is fine for this PR's scope — the spec is what external readers decode from, and moczarr reads the bytes directly — but zagg's own reader surface is now asymmetric between the two companions. The shape of the fix is a
channel=parameter onread_locationsrather than a third near-duplicate sweep of the same ragged-open/decode path. Left standing rather than implemented inside a fold, since it is new public API. The yaml comment andreaders/tdigest_tensor.py's layout header were corrected to state the gap accurately instead of promising a reader that does not exist.Resolved or spun out
The per-cell binding fork— ruled option (c) (espg, 2026-08-17, amending ruling 3): companions stay per-centroid at every level, symmetric with located; §8.4's reduction stays licensed for producers that want it. The binding question vanishes — there is no per-cell overview array to bind, so §8.2's missing binding grammar never has to be closed. What landed.Pre-2017— spun out to issue windows.py: pre-2017 tai-labelled epoch conventions split word/router conversions by 19 s #469 (ataiepoch splits the two conversions by 19 swindows.pyepoch-convention question, not reachable at this seam). No shipped config is affected; all use the 2018 ATLAS SDP epoch.check_companion_matchis not called fromsweep_stage._merge_slabscheck_weights_matchisn't called there either), so it is one pre-existing hole for both conventions rather than half-closed for §8.3/§9.No §7 fixture covers a folded companion— closed by this phase, and for free: the regeneratedtemporal/fixture's §4.6 leaf column carriesh_tdigestwith both siblings at both resolutions, which is the fixture set's first golden for a companion produced by a merge rather than by ingest.test_the_columns_companions_are_row_aligned_and_declaredasserts row alignment and both declarations on the committed bytes.output.time_sourcein the D19 semantic corecb5d0648). Recorded rationale: the D19 hash epoch: granule_workers into DATA_SOURCE_PACKAGING_KEYS; semantic_core widened to the leaf-shaping output knobs (issue #415) #420 boundary applies — the declared epoch changes what is computed (the words are normative bytes), which makes it identity, not packaging; it is the temporal analogue of the §2.0 weights currency; conditional keying plus resolved recording means zero disturbance to pre-Per-centroid temporal companion for t-digests: a 64-bit hierarchical time cell, mirroring the spatial location companion #410 configs or the merged epoch; and California's temporal stores must be born with the clock in their identity, since adding it later would cost a second epoch.GEDI's— verified empirically.delta_timetimescale unverifiedBEAM0000/ancillary/master_time_epochreads exactly 1,198,800,018.000000 on two independent real granules (GEDI01_B_2019128…andGEDI01_B_2020024…): the 2018-01-01 epoch in leap-aware GPS seconds (a naive UTC count would be 1,198,800,000; the +18 is GPS−UTC at 2018). So GEDI shares ICESat-2's convention,scale: gpsis exact offset arithmetic, and the GEDI template carries a per-centroid companion. The constant is cited in the template comment and pinned bytest_the_gedi_clock_is_the_verified_gps_epoch.Coordination for @espg — cross-repo, NOT executed here
moczarr re-vendors
tests/data/spec/temporal/after this merges. §7 records that moczarr vendors these fixtures for its parity gates (espg/moczarr#19/#20) with espg/moczarr#23 as the divergence detector. This PR regenerated that fixture, so the vendored copy goes stale on merge. The moczarr #43 re-check is the natural vehicle. Nothing was done in that repo from here — it is a different repo, so it is flagged rather than actioned.What moved, exactly, so the bump has something precise to follow:
11213.zarr/6/*)written_atall.pyramid.zarr(§4.6 column)h_tdigest,h_tdigest_locations,h_tdigest_timesat resolutions 4 and 5all.pyramid.stats.jsoncombinedmoves tob6859b89…morton_hive.jsonh_tdigestclassnone→approximate(+location,temporal, δ keys);observedstaysnoneFROZEN_COMBINED/FROZEN_ARRAYS/ golden semantic-hash pinInteractions
4211f748, reconciliationdd536396), plus a second sync onto small fixes 2026-08-17 (2): s2_neon_o9 toc flip, CHANGELOG entry for PR #447 #455/small fixes 2026-08-17: read-plan 1-element runs, GEDI template right-sizing #461/inline backend: evict the chunk-map walk's h5coro cache lines (issue #460) #462 and a third onto Asset-sourced shot-level variables + GEDI DEM geolocation-validity gate (issue #464) #466 (mergee9436ea8). No textual conflict arose in any of them; the one semantic reconciliation was movingtime_sourceinto the epoch'score["output"]block rather than leaving it top-level — detail in the sync comment. The GEDI template is the only file where three PRs' additions meet (Asset-sourced shot-level variables + GEDI DEM geolocation-validity gate (issue #464) #466's DEM gate, small fixes 2026-08-17: read-plan 1-element runs, GEDI template right-sizing #461's worker sizing, this PR's toc keys); they are disjoint sections and the union validates.mainfrom prior merged work, so flagged rather than acted on:sweep_overview.py1,908 → ~2,150,sweep_stage.py1,195 → ~1,340. Phase 4's channel-map refactor made most of that net-neutral — it replaced per-channel duplication with oneCOMPANION_CHANNELStable and afield_companionshelper, so adding the second channel cost far less than the first.deployment/is touched. Themortie>=0.9.9floor lives only inpyproject.toml, whichbuild_layer.shderivesMORTIE_SPECfrom per issue Move the mortie decimal-parse boundary off the private _decimal_to_word #322.