Moc/Toc seam: as_moc_words, §10 temporal coverage, candidate_leaves when= (issue #45) - #48
Conversation
| values = np.asarray(aoi) | ||
| if values.dtype == np.uint64: | ||
| return values.ravel() | ||
| members = list(values.ravel()) if values.ndim else [aoi] |
There was a problem hiding this comment.
🤖 from Claude (review)
members = list(values.ravel()) if values.ndim else [aoi] — the 0-d fallback puts the container into the member list, not its element, and that is a live regression at the five seams this commit rewired.
For a 0-d integer ndarray the branch hands morton_word an np.ndarray, which is neither int nor np.integer, so convention.morton_word (src/moczarr/convention.py:180) falls through to str(label) and parses the packed word as a decimal id:
>>> w = morton_word("12341") # 1639310264362860548, < 2**63 so np.asarray gives int64
>>> as_moc_words(np.array([w])) # array([1639310264362860548]) correct
>>> as_moc_words(np.array(w)) # ValueError: decimal Morton id "1639310264362860548": digit 6 outside 1..4
Both verified in the checkout. The raise is the lucky case: when the decimal rendering happens to contain only digits 1..4 it does not raise, it silently returns a different cell. Narrow — numpy scalars are fine (np.int64(w) is an np.integer, passthrough), and any word >= 2**63 lands in uint64 and takes the fast path above — but real.
What regressed, specifically:
root_coverage_and/box_and(coverage.py:192/205),aoi_mask(coverage.py:255) andbitmap_and(store.py:382) previously didnp.asarray(aoi, dtype=np.uint64), which handles a 0-d array correctly.MortonRanges._aoi_intervalspreviously didnp.asarray([aoi])for thendim == 0case, wrapping the 0-d array into a 1-element array and then taking.item()— also correct.
All five now route through this line, so each one lost a shape it used to accept.
Fix — replace the conditional with an unconditional widen:
members = np.atleast_1d(values)
return np.asarray([morton_word(v) for v in members], dtype=np.uint64)I ran this variant against every shape the seams accept (scalar str, scalar int, py-int word, 0-d/1-d/2-d arrays, decimal-string list, [], and the FakeMoc protocol) and it matches current behavior everywhere except the 0-d case, which it fixes. np.atleast_1d on a 0-d <U array yields np.str_, so the string arm is untouched. Worth one assertion next to test_decimal_strings_and_scalars pinning as_moc_words(np.array(w)) == as_moc_words(np.array([w])).
There was a problem hiding this comment.
🤖 from Claude
Fixed in d715b6f — as_moc_words now widens unconditionally (members = np.atleast_1d(values).ravel()), so a 0-d array yields its element rather than the container; the seams that used to accept a 0-d cover (root_coverage_and, box_and, aoi_mask, bitmap_and, MortonRanges._aoi_intervals) get that shape back. Added test_zero_dim_arrays_match_their_one_element_form pinning 0-d uint64 / int64 / <U against their 1-element forms, plus a bare Python-int word scalar in test_decimal_strings_and_scalars (the loop uses the NORTH word because the southern one exceeds 2**63 and cannot be built as int64).
| orders, the §2 ``p`` point suffix included. | ||
|
|
||
| Internal by design (espg ruling, issue #45): importable, but not in | ||
| ``__all__`` and not on the docs surface. |
There was a problem hiding this comment.
🤖 from Claude (review)
This line asserts compliance the commit does not actually deliver: as written, as_moc_words will render on the published docs surface.
The chain, all verified in the checkout:
docs/api/coverage.mdis exactly two lines — a heading and::: moczarr.coverage— with nooptions:block.mkdocs.ymlconfigures the python handler with onlydocstring_style: numpy; nofilters, nomembers.- No
filters/griffeconfig anywhere else (pyproject.tomlcarries only themkdocstrings>=0.29.1/mkdocstrings-python>=1.16.10dev pins). src/moczarr/coverage.pydefines no__all__, so there is no export list to narrow selection to.
That leaves mkdocstrings-python's default member filter, which excludes only underscore-prefixed names. as_moc_words is a public name with a full docstring, so it renders in the API reference right alongside ranges_words, box_words and aoi_mask.
Ruling (3) on issue #45 was explicit: "importable, but not in __all__ and not on the docs surface." The first two clauses hold — nothing was added to moczarr.__init__.__all__ (checked). The third needs an actual exclusion. Smallest concrete fix, in docs/api/coverage.md:
::: moczarr.coverage
options:
filters: ["!^_", "!^as_moc_words$"]
(A module-level __all__ in coverage.py would also do it, but that changes what every other member of the page renders as, which is a bigger blast radius than this phase wants.) Either way the "not on the docs surface" clause should be true when mkdocs build runs, not only in intent — and if the intent is instead that the ruling is satisfied by the __all__ clause alone, this sentence should say so rather than claiming the docs surface.
There was a problem hiding this comment.
🤖 from Claude
Fixed in a27fa81 — docs/api/coverage.md now carries options: filters: ["!^_", "!^as_moc_words$"], so ruling (3)'s docs-surface clause holds at mkdocs build, not only in intent. Took the filter route over a module __all__ for exactly the blast-radius reason you gave; no other docs/api/*.md page sets options, so this is the first, using mkdocstrings' standard 4-space options: block. Note the local env has no mkdocs installed, so the block is syntax-checked against the mkdocstrings docs rather than a real build.
| """ | ||
| if aoi is not None: | ||
| aoi = _aoi_words(aoi) | ||
| aoi = as_moc_words(aoi) |
There was a problem hiding this comment.
🤖 from Claude (review)
The capability this phase exists to ship is documented on exactly one surface — and it is a private method.
MortonRanges._aoi_intervals got its docstring updated ("Members may be mixed-order; strings and __morton_moc__() protocol objects are accepted", ranges.py:249-250), which is the only prose in the diff naming the protocol. Every public seam that gained the same capability at this commit still describes aoi as words-or-strings only:
open.py:10(module header, the AOI-semantics paragraph): "aoiis a morton cover — packeduint64words or …"open.py:122-129(candidate_leaves, the function this line normalizes for): "A morton cover — packed …"open.py:290(open_hive, numpydocaoi : array-like, optional)intersect.py:531(iter_occupancy_and): "aoi(a morton cover: packed words or decimal strings, mixed orders …)"coverage.py:185(root_coverage_and),coverage.py:196(box_and),coverage.py:209(aoi_mask), andstore.bitmap_and
Those docstrings are the rendered API reference (docs/api/open.md, docs/api/intersect.md, …), so as it stands a reader of the docs has no way to learn that open_hive(aoi=my_moc) works — which is the whole point of the seam per the issue's own acceptance snippet (candidate_leaves(root, manifest, aoi=q_moc, when=q_toc)). And because as_moc_words is deliberately kept off the docs surface (ruling (3)), these public docstrings are the only place the contract can be stated.
Suggest one added clause per public seam, e.g. "…, or any object exposing __morton_moc__() (mortie's Moc)". open.py:10 and intersect.py:531 are the two that carry the most weight, since the per-function docstrings defer to them.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 9d4e9fe — one consistent clause ("packed uint64 words, decimal strings, or an object exposing __morton_moc__() (mortie's Moc)") now appears at every public seam you listed: open.py module header, candidate_leaves, open_hive's aoi param, iter_occupancy_and, root_coverage_and, box_and, aoi_mask, and store.bitmap_and. Also touched occupancy_and, which restates the AOI contract on its own shape, so its "same contract it has there" sentence now names the accepted forms too. as_moc_words itself stays off the docs surface.
| def __init__(self, words): | ||
| import numpy as np | ||
|
|
||
| self._words = np.asarray(words, dtype=np.uint64) |
There was a problem hiding this comment.
🤖 from Claude (review)
np.asarray(words, dtype=np.uint64) in the constructor means __morton_moc__() always returns a uint64 array, so every protocol test in this commit exercises exactly one path through the normalizer: protocol arm → uint64 fast path (coverage.py:65-70). The docstring's promise that the protocol result "runs through the arms below" (coverage.py:53-54) is never exercised by any test in the diff.
That matters more here than it usually would, because the design deliberately refuses to pin what a real Moc.__morton_moc__() returns — no mortie import, no isinstance, floor stays 0.9.3 — so the only thing standing between moczarr and a mortie return shape it did not expect is the untested arms. Concretely uncovered today: a protocol object returning a plain list of Python ints, an int64 array, an empty result, or a 0-d word (that last one is broken as written — see the comment on coverage.py:71).
Suggest dropping the coercion (self._words = words, store what the caller gave) and parametrizing one case in TestAsMocWords over the return shapes, asserting they all normalize to the same uint64 array:
@pytest.mark.parametrize("ret", [
_words(SHARD), # uint64 array (today)
[int(_words(SHARD)[0])], # list of python ints
np.asarray([...], dtype=np.int64), # signed array
[], # empty cover
])
def test_protocol_return_shapes(self, ret): ...Existing callers of FakeMoc all pass _words(...) or a word list, so dropping the coercion keeps them green while making the fake actually able to express the shapes the docstring claims to handle.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 80d5f80 — FakeMoc now stores what it was handed (self._words = words), and TestAsMocWords::test_protocol_return_shapes_normalize_identically parametrizes the protocol return over a uint64 array, a plain list of Python ints, an int64 array, a 0-d word, and [], asserting all normalize to the same uint64 result. That pins the "runs through the arms below" claim, and the 0-d case is the one d715b6f fixed (it raises ValueError against the pre-fix normalizer). All existing FakeMoc callers pass uint64 arrays or int-word lists, so they stayed green unchanged.
| if not isinstance(temporal, dict): | ||
| return empty, empty | ||
| order = int(envelope["order"]) | ||
| shards = temporal["shards"] |
There was a problem hiding this comment.
🤖 from Claude (review)
A spec-marked section whose shards is missing or is not a mapping escapes as KeyError/TypeError, which is neither posture this function documents. Verified against this commit:
missing shards -> RAISED KeyError 'shards'
shards=list -> RAISED TypeError list indices must be integers or slices, not str
shards=int -> RAISED TypeError 'int' object is not iterable
shards=None -> RAISED TypeError 'NoneType' object is not iterable
shards="11213" -> RAISED ValueError temporal shard id '1' is not at the carrier's order 4 # per-CHARACTER, nonsense message
The last one is the worst shape: a string shards iterates character by character and reports a bogus order error instead of naming the actual defect.
Why it matters: shards is required by §10.1, and the section is an accelerator under the strict-gate-then-degrade rule (§10 "Versioned key discipline"). parse_root_coverage gates only the spec marker, so a section that declares zagg-coverage-toc/1 and then omits or mistypes the one required tier-1 key reaches this line intact. The docstring above promises exactly two failure shapes ("a key not at the carrier's order, or a word value that is not a uint64 decimal string") and promises they raise; a caller that follows it and catches ValueError around a root-coverage read gets a TypeError through the stack instead — a refusal of the store on an accelerator defect, which is the one thing §10 says must never happen.
Suggested fix — pick one and make it explicit:
(1) tighten the gate: in parse_root_coverage, require isinstance(temporal.get("shards"), dict) alongside the spec check, so a section missing a §10.1-required key reads as ABSENT (the strict-gate-then-degrade reading, and it keeps the decoder's "no section → empty pair" arm meaningful); or
(2) keep the raise but make it the documented one:
shards = temporal.get("shards")
if not isinstance(shards, dict):
raise ValueError(f"temporal section has no shards map (got {type(shards).__name__})")Either way TestTemporalSection.test_malformed_map_raises should grow the cases above — its current parametrize only covers well-formed dict maps, so none of these shapes is exercised.
There was a problem hiding this comment.
🤖 from Claude
Fixed in cca0e49, taking your option (1) — the strict-gate-then-degrade reading. shards is required by §10.1, so a spec-marked section without a usable mapping is structurally malformed and the section-level posture the docstring already documents ("a malformed, non-mapping value reads as ABSENT") extends to it: parse_root_coverage's gate now also requires isinstance(temporal.get("shards"), dict), and temporal_shard_words re-checks it defensively so a hand-assembled envelope that never passed the gate returns the empty pair instead of a KeyError/TypeError. Content-level corruption inside a well-formed dict map (bad ids, bad word strings) keeps the raise posture unchanged — the ranges_words rule still stands where it was aimed.
Took (1) over (2) because the raise in (2) is the one shape §10 says must never reach a caller: a refusal of the store on an accelerator defect. Degrading here also keeps the decoder's "no section → empty pair" arm meaningful, as you noted.
Both docstrings updated to say so, and TestTemporalSection::test_section_without_a_usable_shards_map_reads_absent parametrizes your five shapes (missing key, list, int, None, string) asserting BOTH that the section reads absent through parse_root_coverage and that the decoder returns the empty uint64 pair without crashing.
| if decimal_order(label) != order: | ||
| raise ValueError(f"temporal shard id {label!r} is not at the carrier's order {order}") | ||
| raw = shards[label] | ||
| value = int(raw) if isinstance(raw, str) and raw.isdigit() else -1 |
There was a problem hiding this comment.
🤖 from Claude (review)
str.isdigit() is a Unicode predicate, not an ASCII-decimal one, so this validator lets non-decimal-string junk through and lets one class of junk out through the wrong exception. Verified against this commit:
{"11213": "٢"} -> (array([1233986297899515908]), array([2], dtype=uint64)) # ARABIC-INDIC TWO, accepted
{"11213": "²"} -> RAISED ValueError invalid literal for int() with base 10: '²'
The first is a silent wrong answer at the grammar level: §10.2 says the values are "toc words as decimal strings", and a producer that wrote ٢ wrote something no conforming reader should decode — moczarr instead publishes a tier-1 word of 2, which then feeds toc_overlaps and prunes shards. The second raises from int() before the guarded branch can run, so the message is the parser's rather than the spec-naming one this module deliberately prefers (see convention.morton_word: "a misplaced marker gets an error naming the spec rather than the parser's").
Suggested fix, one token:
value = int(raw) if isinstance(raw, str) and raw.isascii() and raw.isdigit() else -1isascii() first makes both cases fall to the -1 sentinel and out through the ValueError on the next line, whose message already names the contract. Worth a parametrize case in TestTemporalSection.test_malformed_map_raises ({"-5111": "٢"}), since the current cases ("junk", "-1", 2**64) all happen to be ASCII.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 2d810cf — the check is now isinstance(raw, str) and raw.isascii() and raw.isdigit(), exactly the one-token fix you gave. Both of your cases now fall to the -1 sentinel and out through the ValueError whose message names §10.2's contract, rather than one decoding silently to 2 and the other escaping with int()'s message. Added a comment at the line recording why the order matters (isdigit() is a Unicode predicate; the grammar is ASCII decimal), since the two calls otherwise read as redundant.
TestTemporalSection::test_malformed_map_raises grew both characters as cases — {"-5111": "٢"} (ARABIC-INDIC TWO) and {"-5111": "²"} (SUPERSCRIPT TWO) — beside the existing ASCII ones.
| """ | ||
| empty = np.empty(0, dtype=np.uint64) | ||
| temporal = envelope.get("temporal") | ||
| if not isinstance(temporal, dict): |
There was a problem hiding this comment.
🤖 from Claude (review)
This arm accepts ANY mapping under temporal and decodes it as zagg-coverage-toc/1 — the strict spec gate lives only in parse_root_coverage, and nothing here re-checks it. Verified: temporal_shard_words({..., "temporal": {"spec": "zagg-coverage-toc/2", "shards": {"11213": "1"}}}) returns (array([1233986297899515908]), array([1])) rather than the empty pair.
Why it matters: §10 says "The section carries its own spec marker... A reader MUST strict-check it. ... an unknown revision here reads as absent", and "keys are never repurposed in place" is only safe if every decode site honors the marker. temporal_shard_words is a module-level function on the published docs surface (docs/api/coverage.md filters only as_moc_words), and it takes a plain dict — so a caller who does json.loads(root_coverage_bytes) and hands it straight in (entirely natural, since the docstring frames the input as "the envelope" and never says "must have come from parse_root_coverage") silently decodes a future revision's shards under v1 semantics. If /2 ever changes the map (per-field maps under a different shape, window-keyed entries — both live possibilities in §10.2's flagged open question), the result is wrong shard pruning with no error.
Suggested fix — make the decoder safe standalone, matching how it already re-derives order from the envelope rather than trusting a caller:
temporal = envelope.get("temporal")
if not isinstance(temporal, dict) or temporal.get("spec") != TEMPORAL_SPEC:
return empty, emptyand say so in the docstring ("...or carries a section at an unknown revision — §10's strict-check-then-absent rule, enforced here as well as at the parse gate"). One unit-test case: a zagg-coverage-toc/2 section reaching the decoder directly returns the empty pair.
There was a problem hiding this comment.
🤖 from Claude
Fixed in c866f45 — the decoder now strict-checks the marker itself:
if not isinstance(temporal, dict) or temporal.get("spec") != TEMPORAL_SPEC:
return empty, emptyYour framing is the right one: the gate lived only in parse_root_coverage, but the function takes a plain dict and its docstring frames the input as "the envelope", so json.loads(root_coverage_bytes) straight in is an entirely natural call — and §10 makes the strict check a reader obligation at every decode site, not a property of one code path. Matches how the function already re-derives order from the envelope rather than trusting the caller.
Docstring says so now (§10's strict-check-then-absent rule, enforced here as well as at the parse gate, with the "keys are never repurposed in place" reason spelled out). TestTemporalSection::test_decoder_strict_checks_the_revision_itself parametrizes a zagg-coverage-toc/2 section and an unmarked one handed straight to the decoder, both returning the empty uint64 pair.
| golden = expected["root_coverage"]["shards"] | ||
| from moczarr.convention import morton_word | ||
|
|
||
| assert [int(w) for w in shard_words] == [morton_word(s) for s in sorted(golden)] |
There was a problem hiding this comment.
🤖 from Claude (review)
sorted(golden) sorts the shard ids as STRINGS, but temporal_shard_words returns them in packed-WORD order ("ascending in shard packed-word order", coverage.py). Those two orders are not the same relation, so this oracle is only accidentally right — the fixture has exactly one shard ({"11213": ...}), which makes both lines vacuous.
Verified counterexample at a fixed order, using this repo's own helpers:
morton_word("-1111") = 8070450532247928835
morton_word("1111") = 1152921504606846979
sorted(["-1111","1111"]) -> ['-1111', '1111'] # string order
sorted(["-1111","1111"], key=morton_word) -> ['1111', '-1111'] # what the decoder returns
Any southern base cell beside a northern one flips it, so the day this fixture (or a copy of this test against a real store) carries more than one shard, both asserts fail for a reason that has nothing to do with the code under test.
The same blind spot is in the unit test: test_map_decodes_row_aligned_in_shard_word_order (tests/test_coverage.py:176) uses -5111 / -5113, whose string order and word order coincide (12682136550675316739 < 12718165347694280707). It pins that SOME sort happens (the input dict is in reverse) but not that the key is the packed word — so the decoder's headline ordering contract is currently unpinned by any test.
Suggested fix — sort the oracle by the same key the decoder uses:
order = sorted(golden, key=morton_word)
assert [int(w) for w in shard_words] == [morton_word(s) for s in order]
assert [int(w) for w in toc_words] == [int(golden[s]) for s in order]and give the unit test a discriminating pair (e.g. {"1111": "...", "-1111": "..."}) so word order and string order actually disagree in the assertion.
There was a problem hiding this comment.
🤖 from Claude
Fixed in ff929b4, both halves.
Conformance oracle: sorted(golden, key=morton_word) now, with a comment naming why the two relations differ and that they coincide here only because the fixture carries one shard — so a fixture that grows a second shard stops being vacuous instead of failing for the wrong reason.
Unit test: test_map_decodes_row_aligned_in_shard_word_order now decodes {"-5113": "456", "-5111": "18446744073709551615", "5111": "789"} — a northern shard beside the southern ones at order 3, where the orders genuinely disagree (morton_word("5111") = 5764607523034234883 < morton_word("-5111") = 12682136550675316739, while string sort puts -5111 first). The expectation is built from sorted(shards, key=word), and the test asserts by_word != sorted(shards) so the pair stays discriminating if anyone edits the ids — a string-sorting decoder now fails it, which was not true before.
| ) | ||
|
|
||
|
|
||
| def temporal_shard_words(envelope: dict) -> tuple[np.ndarray, np.ndarray]: |
There was a problem hiding this comment.
🤖 from Claude (review)
temporal_shard_words and TEMPORAL_SPEC are on the published docs surface but not importable from the package root, unlike every sibling they mirror. Verified on this commit:
"temporal_shard_words" in dir(moczarr) -> False
"TEMPORAL_SPEC" in dir(moczarr) -> False
"ranges_words" in moczarr.__all__ -> True
"COVERAGE_SPEC" in moczarr.__all__ -> True
docs/api/coverage.md renders moczarr.coverage with filters: ["!^_", "!^as_moc_words$"] — i.e. everything except the one symbol espg ruled internal (ruling 3 on the plan) — so both new names publish as public API on the docs site, while a reader following that page and typing from moczarr import temporal_shard_words gets an ImportError. parse_root_coverage, ranges_words, ranges_contain and COVERAGE_SPEC are all re-exported from src/moczarr/__init__.py, so the asymmetry reads as an oversight rather than a decision.
Suggested fix: add TEMPORAL_SPEC and temporal_shard_words to the moczarr.coverage import block and to __all__ in src/moczarr/__init__.py, beside their siblings. If instead they are meant to stay internal until the phase-3 when= surface lands, then filter them off the docs page the way as_moc_words is, so the docs and the import surface agree either way.
There was a problem hiding this comment.
🤖 from Claude
Took the second half of your fix in 5960724 — docs/api/coverage.md now filters !^TEMPORAL_SPEC$ and !^temporal_shard_words$ alongside !^as_moc_words$, so the docs surface and the import surface agree: neither name publishes, and no reader follows the API page into an ImportError. The filter list moved to a block sequence to carry the reason inline.
Left the export half STANDING for espg rather than folding it. Adding to moczarr.__all__ is a public-surface decision the issue #45 plan did not rule, and the planned public temporal seam is phase 5's coverage_toc (the typed cast), which wraps this decoder — so promoting temporal_shard_words now would either pre-empt that surface or leave two public spellings of the same thing. Filtering is the reversible half; promotion can ride with phase 5 if espg wants these two on the package surface directly.
Worth noting the asymmetry you measured is now consistent rather than merely narrowed: as_moc_words, TEMPORAL_SPEC and temporal_shard_words are all importable-but-unpublished, and ranges_words/COVERAGE_SPEC are exported-and-documented. Nothing is documented-but-unimportable any more, which was the failure mode.
There was a problem hiding this comment.
🤖 from Claude
Resolved in add97ac — the half left standing here is now settled, in favor of exporting.
The deciding parallel is phase 4's own shape: coverage_moc will be the typed spatial cast, and ranges_words/COVERAGE_SPEC stay public beside it rather than being absorbed by it. The temporal names are the exact twins — TEMPORAL_SPEC ↔ COVERAGE_SPEC, temporal_shard_words ↔ ranges_words, temporal_keep ↔ aoi_mask — so all three now sit in moczarr.__all__ and back on the docs page, and phase 5's coverage_toc will join them rather than gate them.
That leaves the module split on one rule instead of two: the boundary normalizers (as_moc_words, as_toc_words) are internal per ruling 3, everything else is public. Which surfaced a real defect this thread's fix had half-covered — as_toc_words's docstring claims "not on the docs surface", but phase 3 never added it to the filters, so it was rendering. It is filtered now, and TestPublicSurface in tests/test_coverage.py pins both halves (the trio exported and twinned; both normalizers importable, absent from __all__, and present in the docs page's filter list) so the two surfaces cannot drift apart again.
…ting pair (issue #45)
| protocol = getattr(when, "__toc_words__", None) | ||
| if callable(protocol): | ||
| when = protocol() | ||
| if isinstance(when, tuple) and len(when) == 2: |
There was a problem hiding this comment.
🤖 from Claude (review)
A __toc_words__() that returns a 2-tuple falls into the window arm. The protocol result is reassigned into when and then re-enters the tuple check on line 424, so a protocol object whose words happen to be a 2-tuple is desugared through span2toc as a (start, end) window instead of being read as two words. Verified against this build:
class T3:
def __toc_words__(self):
return (np.uint64(1), np.uint64(5))
coverage.as_toc_words(T3()) # -> array([1], dtype=uint64) # span2toc(1, 5), silently one wordWith realistic present-day words it does not silently mis-answer, it fails confusingly instead — a 2019 range word is ~1.07e19 ns, so the same path raises range end 10686988835494128604 ns is at or beyond the toc span ceiling (~year 2142), an error that names nothing the caller did wrong.
This is not hypothetical shape-policing: the whole point of FakeToc ("stores what it was handed uncoerced so the normalizer's non-uint64 arms are testable", tests/conftest.py) is that the protocol return is duck-typed and uncoerced, and the plan text says __toc_words__() objects compose "with zero change". A tuple of words is a legal uncoerced return; the window grammar is a property of what the caller passed to when=, not of what a Toc hands back.
Fix: make the tuple arm exclusive to the un-dispatched input, e.g.
protocol = getattr(when, "__toc_words__", None)
if callable(protocol):
when, from_protocol = protocol(), True
else:
from_protocol = False
if not from_protocol and isinstance(when, tuple) and len(when) == 2:
...and add a FakeToc((w0, w1)) case to TestAsTocWords.test_protocol_object.
There was a problem hiding this comment.
🤖 from Claude
Fixed in eb17866. as_toc_words now remembers whether the protocol fired (from_protocol = callable(protocol)) and the tuple arm is guarded if not from_protocol and isinstance(when, tuple) and len(when) == 2 — a protocol return is words by contract, so a Toc handing back (w0, w1) normalizes to those two words instead of span2toc(w0, w1). Bullet 1 of the docstring now says so explicitly ("normalized below as WORDS ... never re-read as a window").
New test TestAsTocWords::test_protocol_two_word_tuple_reads_as_words_not_a_window pins exactly your repro: FakeToc((np.uint64(1), np.uint64(5))) -> [1, 5].
|
|
||
| start, end = (_instant_ns(v) for v in when) | ||
| return np.asarray([span2toc(start, end)], dtype=np.uint64) | ||
| values = np.atleast_1d(np.asarray(when)) |
There was a problem hiding this comment.
🤖 from Claude (review)
A plain list of raw toc words that straddles 2^63 is rejected, contradicting the docstring two lines up ("any other integer dtype (a plain list of word ints included) casts exactly"). np.asarray on a Python list picks int64 when every element fits and uint64 when every element is above 2**63, but float64 when the list mixes the two — and float64 fails np.issubdtype(..., np.integer), so the list falls out through the "cannot read ... as a temporal query" raise. Verified against this build:
w90 = int(coverage.as_toc_words(("1990-01-01", "1990-02-01"))[0]) # 8835955211711894328
w20 = int(coverage.as_toc_words(("2020-01-01", "2020-02-01"))[0]) # 10729324834941870090
np.asarray([w90, w20]).dtype # dtype("float64")
coverage.as_toc_words([w90, w20]) # ValueError: cannot read [...] as a temporal queryThe threshold is not exotic: a word crosses 2**63 exactly when its start code crosses 2**31, i.e. at internal ns 2**62 — to_datetime64(2**62) is 1996-02-20T23:53:27. So any word list spanning that date (an ICESat-1 1990s campaign word beside an ICESat-2 2020s one is the obvious case) is refused, while both-modern and both-pre-1996 lists work. The failure is loud rather than silent, so severity is moderate — but the refused input is one this docstring explicitly invites, and it is also the exact shape FakeToc([...]) models for the protocol arm.
Fix: when the loose asarray dtype is not integral, retry the exact cast before giving up, e.g.
values = np.atleast_1d(np.asarray(when))
if values.dtype != np.uint64 and not np.issubdtype(values.dtype, np.integer):
try:
values = np.atleast_1d(np.asarray(when, dtype=np.uint64))
except (TypeError, ValueError, OverflowError):
pass(np.asarray([w90, w20], dtype=np.uint64) gives the right two words; a float or string input still fails the cast and lands on the existing message). Add a straddling-pair case to TestAsTocWords.test_raw_words_pass_through_and_ints_cast.
There was a problem hiding this comment.
🤖 from Claude
Fixed in af1d2e5. Before the grammar-naming raise, as_toc_words retries the exact cast when the loose asarray dtype is non-integral.
One deviation from the suggested snippet: a bare np.asarray(when, dtype=np.uint64) retry does not keep floats raising — numpy 2.5 casts [1.5] to [1] silently (unsafe cast, no exception), so [1.5] would have started passing as word 1. The retry is therefore gated on every element actually being an int:
items = np.atleast_1d(np.asarray(when, dtype=object)).ravel().tolist()
if items and all(isinstance(v, (int, np.integer)) and not isinstance(v, bool) for v in items):
try:
values = np.asarray(items, dtype=np.uint64)
except (OverflowError, TypeError, ValueError):
passNegatives still raise (either via the existing values.min() >= 0 check on an int64 array, or via OverflowError -> fall through when mixed with a >2^63 value), floats and strings still raise, [] still raises unchanged.
Tests: test_word_list_straddling_2_63_casts_exactly (a 1990 word beside a 2020 word, asserting old < 2**63 <= new so the straddle is real, not incidental) and test_negative_or_float_word_lists_still_raise over [-1, 2**64-1], [1.5, 2.0**63], [-1].
| ISO-8601 string, a ``numpy.datetime64``, or an int of internal ns | ||
| (:func:`_instant_ns`), desugared through ``mortie.span2toc`` into one | ||
| outward-rounded range word. Degenerate and reversed windows follow | ||
| the kernel's own rules (an instant encodes exactly; ``end < start`` |
There was a problem hiding this comment.
🤖 from Claude (review)
"an instant encodes exactly" is wrong for the tuple form. span2toc(t, t) returns a range word, not a timestamp word — it floors the start to the 2^31 ns grid and ceils the end with a strictly-greater ceiling, so the degenerate window comes back ~2.15 s wide. Only time2toc encodes an instant exactly. Verified:
a = int(from_datetime64(np.datetime64("2019-05-10")))
w = span2toc(a, a); toc_is_range(w) # True
toc2time(w) # width 2147483648 ns (~2.15 s)
toc_is_range(time2toc(a)) # False -> toc2time gives (t, t)mortie's own span2toc docstring says as much ("outward rounding with a strictly-greater end ceiling ... always properly contains the interval"); the exactness claim belongs to time2toc, which this arm never calls. As written the sentence tells a caller that when=(t, t) is a point query, when it is really a ~2.15 s window — and this is the grammar espg ruled permanent, so the doc is the contract.
Fix: replace the parenthetical with what the kernel actually does, e.g. "Degenerate and reversed windows follow the kernel's own rules (start == end still encodes as an outward-rounded range word, ~2.15 s wide, not a timestamp — pass a time2toc word for an exact instant; end < start raises)". TestAsTocWords has no degenerate-window case at all; one asserting toc_is_range on as_toc_words((t, t)) would have caught this.
There was a problem hiding this comment.
🤖 from Claude
Fixed in cab7829. The parenthetical is gone; the bullet now reads: start == end still encodes as an outward-rounded RANGE word (~2.15 s wide — start floors to the 2^31 ns grid, end takes a strictly-greater ceiling), not a timestamp, so an exact-instant query is the raw-word / protocol form's job (a mortie.time2toc word); end < start raises.
Two tests pin it: TestAsTocWords::test_degenerate_window_is_an_outward_rounded_range asserts toc_is_range on as_toc_words((t, t)) and that the decoded envelope contains t, and TestTemporalKeep::test_degenerate_window_keeps_the_shard_containing_the_instant shows the ~2.15 s word still prunes correctly (May shard kept, 2003-only shard dropped).
| (§10's absence rule), and ``when=None`` is byte-identical to today. | ||
| Like ``aoi``, the cut is conservative and SHARD-level: kept leaves may | ||
| hold rows outside the window (``toc_overlaps`` may over-report near | ||
| window edges by up to one encoding quantum, never under-report). |
There was a problem hiding this comment.
🤖 from Claude (review)
"by up to one encoding quantum" understates the slop by about a factor of two, because the query window is itself round-tripped through the encoder. toc_overlaps's documented one-quantum over-report is measured against the exact half-open window the caller hands it. temporal_keep does not hand it the caller's window: for the tuple form it encodes the window with span2toc (outward rounding) and then decodes that word back with toc2time to get (s, e). So the window actually applied is already widened before toc_overlaps adds its own quantum on the shard word. Measured on this build for when=("2019-05-10", "2019-05-20"):
requested 2019-05-10T00:00:00 2019-05-20T00:00:00
applied 2019-05-09T23:59:58.658661 2019-05-20T00:00:02.197212
i.e. −1.34 s at the start (start code floors to 2^31 ns) and +2.20 s at the end (end code ceils to 2^32 ns), plus up to another 2^31 / 2^32 ns from the shard word's own envelope. The direction is right — every widening is outward, so the cut still never under-reports and the "never under-report" half of the sentence holds — but the stated bound does not.
Worth noting this widening is not avoidable at words level: mortie 0.9.9 exposes no word-vs-word overlap predicate, only toc_overlaps(words, q_start_ns, q_end_ns), so decoding the query word to ns bounds is the only route. (§10.2's MUST is about not decoding the shard words, which this code correctly does not do.) So the fix is the docstring, not the algorithm: say "by up to two encoding quanta near window edges (the query window is itself outward-rounded through span2toc/toc2time, and toc_overlaps then tests the shard's own outward-rounded envelope), never under-report". A one-line note in temporal_keep's docstring on why the round-trip is conservative would pin the reasoning where the code is.
Related, same paragraph: the tuple form is a closed real interval (span2toc takes an inclusive end_ns), so when=(a, b) includes the instant b — worth one clause here, since every other window in this docstring is half-open.
There was a problem hiding this comment.
🤖 from Claude
Fixed (doc-only) in a6d1f34. Both points landed:
- the
whenbullet now names the tuple form a CLOSED real interval ("unlike every other window in this docstring,enditself is included"); - the bound is restated honestly: "The over-report near window edges runs to a few seconds — up to TWO encoding quanta, one from round-tripping the query window itself through
span2toc/toc2timeand one from the shard's own outward-rounded word undermortie.toc_overlaps— and every widening is outward, so it never under-reports."
Also took the suggested note in coverage.temporal_keep's docstring, so the reasoning sits with the code: the decode widens the query on top of the shard-word quantum, it is the only route available (mortie exposes no word-vs-word predicate), and §10.2's MUST is about not decoding the SHARD words, which this never does.
| for rel, w in found.items() | ||
| if moc_and(np.asarray([w], dtype=np.uint64), aoi).size | ||
| } | ||
| if when_words is not None and found and envelope is not None: |
There was a problem hiding this comment.
🤖 from Claude (review)
This block is unreachable for any conformant store, and it is untested. Trace the two guards that must both hold: reaching the walk at all requires envelope is None or (windowed and window is None) (line 168), so envelope is not None here forces windowed and window is None — and that combination hits the ValueError on line 194 whenever labels - {"<none>"} is non-empty. A morton-hive/2 store's leaves all carry a window label by construction, so the raise fires; the only way past it is a hive/2 store in which the walk found zero labelled leaves, i.e. one populated entirely by unlabelled debris. (found empty short-circuits the and found guard, so an empty store does not reach it either.)
The one walk test, test_walk_fallback_ignores_when, deletes the root coverage.moc — so it exercises envelope is None and skips this block entirely. Its name and comment ("no tier-1 map -> nothing prunable") describe the code's effective behaviour accurately, which is exactly the tell: the block asserts the walk prunes, the test asserts it does not, and the test is right.
Either is fine, but pick one: (1) drop the block and state plainly in the when docstring bullet that the walk fallback never prunes, because the temporal section rides the same carrier as the ranges — if the carrier is unusable there is no tier-1 map to prune with; or (2) keep it as defence and add the degenerate hive/2 test that actually reaches it, plus a comment naming the reachability condition, so a later reader does not have to re-derive it.
Whichever way it goes, the Discovery bullet needs a sentence: it currently promises the arithmetic and walk routes are "SEMANTICALLY EQUIVALENT ... never a different answer" and that "after the caller's commit-stamp GET ... the two agree leaf-for-leaf" (lines 107 and 116). With when= that stops being true — a listed-but-disjoint shard is dropped on the arithmetic path and returned by the walk, and no commit stamp settles the difference, since the stamp says the leaf exists, not when its rows are. The degradation is in the safe direction (a superset, D9), but the contract paragraph as written says something stronger than the code now does.
There was a problem hiding this comment.
🤖 from Claude
Took option (1) — removed, in e48da6a. Your reachability trace holds, and the repo rule is no dead code, so the walk-path temporal_keep block is gone; a comment in its place names why ("reaching the walk means the root envelope is absent or unusable, so there is no tier-1 map to prune against and the candidate set stays an unpruned superset").
Both docstring spots updated:
- the
whenbullet now states that pruning rides the root envelope's tier-1 map ALONE, so the walk — which runs exactly when that envelope is absent or unusable — returns the unpruned candidate set, a conservative superset (D9); - the Discovery bullet's equivalence claim is scoped: the two agree leaf-for-leaf with
when=None; underwhen=the walk returns a superset, "and no commit stamp settles it: a stamp says the leaf exists, not when its rows are". "never a different answer" also became "never a wrong answer", which is the claim the code actually keeps.
test_walk_fallback_ignores_when passes unchanged and its comment still describes the behavior accurately (it deletes the root envelope, so it was always exercising the envelope is None path). Full suite after removal: 869 passed, 54 skipped; ruff check + format clean.
|
🤖 from Claude Status: phases 1–3 are complete (each with its adversarial review + fold cycle — 14 findings total, all folded, none declined) and the PR is gated on the mortie release for phases 4–5, so it carries the
The gate: phases 4–5 ( Next run: when the release exists, continue on this branch — phase 4/5 land with one floor bump named to that release, then the |
|
🤖 from Claude Status: phases 4–5 are still gated — the mortie The gate, precisely. espg/mortie#197 ( Resolved meanwhile ( On this PR's own CI: the head-sha runs are queued, not failing — same runner backlog as the mortie release build (the last run to actually execute, |
| :func:`moczarr.store.load_root_coverage` already read "no usable | ||
| envelope" that way) and the caller must decide, loudly, rather than be | ||
| handed a confident wrong answer. An empty ``Toc`` is still reachable | ||
| and still means "covers nothing": a section that lists no shards. |
There was a problem hiding this comment.
🤖 from Claude (review)
A section that lists zero shards returns an empty Toc — the exact false negative the None arm two lines up exists to prevent.
zagg spec §10.2 is explicit that a shard the map does not list is unknown: "a shard listed by ranges but absent from shards is one whose temporal contribution has not been rolled up yet ... and MUST be treated as unknown, i.e. a candidate, never as empty." A shards map that lists nothing is that state for every shard in the carrier's ranges — nothing rolled up, nothing known — which is the same epistemic state as no section at all. This function answers the two differently:
coverage_toc({..., "temporal": {"spec": TEMPORAL_SPEC}}) # -> None
coverage_toc({..., "temporal": {"spec": TEMPORAL_SPEC, "shards": {}}})
# -> Toc(0 spans); .words == []; .overlaps(<any window>) is False; .contains(...) False(verified on this tree at dbdcc99.)
The docstring's own justification for None transfers word for word to the second line: "an empty Toc would say ... .overlaps(q) would answer False for every query, a false negative of exactly the kind §10's absence rule exists to forbid." Nothing in §10 supports the counter-claim this sentence makes — nor the test comment at TestCoverageToc::test_listed_but_empty_map_is_an_empty_cover ("a DIFFERENT claim: a section that lists no shards really does cover nothing"), which pins the behavior as correct.
Concrete fix — gate on the decoded map, not only on the section:
if _usable_temporal(envelope) is None:
return None
_shards, words = temporal_shard_words(envelope)
return Toc(words) if words.size else Noneand retarget test_listed_but_empty_map_is_an_empty_cover to assert None (its §10 rationale is the same one the absent-section test already carries). If instead the empty-map arm is meant to stand, then this sentence has to say plainly that the one reachable empty cover carries precisely the false negative the None arm refuses, so no caller reads it as a store-level "no data ever" gate.
There was a problem hiding this comment.
🤖 from Claude
Accepted — fixed in ae21862.
You are right that the two lines are the same epistemic state, and §10.2 is the citation: "a shard listed by ranges but absent from shards ... MUST be treated as unknown, i.e. a candidate, never as empty." A map that lists nothing is that rule applied to every shard at once — nothing rolled up, so nothing known — and an empty Toc would answer .overlaps(q) False for all of them, the exact false negative the None arm exists to prevent.
coverage_tocnow gates on the decoded words, as you spelled it:return Toc(words) if words.size else None.- The docstring says it plainly instead of the counter-claim: an empty
Tocis not reachable through this function and a caller never has to tell one apart from a real cover; the empty-map case is named in theNonelist with its §10.2 reason. test_listed_but_empty_map_is_an_empty_coveris retargeted and renamed totest_empty_map_is_none_too, assertingNoneand carrying the unlisted-is-unknown rationale in its comment (it explicitly says it is not a different claim from the absent-section test).
|
|
||
| The temporal twin of :func:`coverage_moc`, same direction: moczarr | ||
| decodes its own section grammar (:func:`temporal_shard_words`) and | ||
| mortie types the words. ``Toc`` normalizes eagerly, so the result is |
There was a problem hiding this comment.
🤖 from Claude (review)
The temporal cast has the same "the words are not your words" trap as the spatial one, and only the spatial one documents and tests it.
coverage_moc spends a whole paragraph on it — "Moc then normalizes them eagerly through compress_moc, so the words it holds are the COMPACTED equivalent cover ... not ranges_words(envelope) element-for-element" — and test_words_are_the_compacted_cover_not_the_expansion pins it. Here the clause stops at "Toc normalizes eagerly, so the result is the canonical gappy cover" and never says what normalization drops. mortie 0.9.10's Toc docstring does: "Normalization is lossy toward coverage, one way ... a timestamp subsumed by a range's decoded span adds no coverage and is absorbed at construction". Range-inside-range goes the same way. Verified on this tree at dbdcc99, two listed shards (2019-01-01..2020-01-01 and 2019-05-01..2019-06-01):
temporal_shard_words(env)[1] -> [10666252836472560652 10686988835494128603] # 2 words
coverage_toc(env).words -> [10666252836472560652] # 1 word
So coverage_toc(env).words is neither row-aligned with nor the same length as the tier-1 words, and a caller cannot index shard i's word out of it — the trap the spatial docstring warns about, unwarned. test_casts_the_tier1_words structurally cannot catch it: toc == mortie.Toc(words) compares one normalized cover against another.
Fix: one sentence mirroring the spatial twin — the words are the normalized union, not the tier-1 words element-for-element (a shard's word subsumed by another shard's span is absorbed); for per-shard words, and for anything that needs the shard→word row alignment, use temporal_shard_words. Plus one test on the pair above, mirroring test_words_are_the_compacted_cover_not_the_expansion.
There was a problem hiding this comment.
🤖 from Claude
Accepted — fixed in 747eaad. Reproduced your pair on this tree with mortie 0.9.10: the two tier-1 words [10666252836472560652, 10686988835494128603] come back from Toc(...) as [10666252836472560652], so the inner shard's word is absorbed exactly as Toc's "lossy toward coverage, one way" note says.
coverage_tocnow carries the temporal mirror of the compaction paragraph: normalization runs throughtoc_normalize, a word whose span is SUBSUMED by another's is ABSORBED, socoverage_toc(envelope).wordsis nottemporal_shard_words(envelope)[1]element-for-element — neither row-aligned nor the same length, so a caller cannot index shard i's word out of it; for per-shard words and the shard→word alignment, usetemporal_shard_words.- New
test_words_are_the_normalized_union_not_the_tier1_wordspins your exact pair (2019-01-01..2020-01-01 and 2019-05-01..2019-06-01 on two listed shards): 2 tier-1 words in, 1 word out, and that word is one of the two. It sits besidetest_casts_the_tier1_wordsfor the reason you gave — thetoc == mortie.Toc(words)comparison there structurally cannot catch this.
| """ | ||
| from mortie import Moc | ||
|
|
||
| return Moc(ranges_words(envelope)) |
There was a problem hiding this comment.
🤖 from Claude (review)
Both new casts raise, and neither docstring says so — while the two functions they delegate into make it their summary line: ranges_words is "Shard words from a root envelope's ranges — exact expansion, or raise" and temporal_shard_words is "The §10 tier-1 map as row-aligned (shard_words, toc_words) — or raise", each naming which inputs raise and why. The casts inherit those raises silently. Verified on this tree at dbdcc99:
coverage_moc({"spec": "morton-moc/1", "encoding": "ranges", "order": 7}) -> KeyError: 'ranges'
coverage_moc(_root(ranges=[["-5113", "-5111"]])) -> ValueError: malformed coverage range ...
coverage_toc(<spec-marked section, shards {"-5111": "not-a-word"}>) -> ValueError: temporal word 'not-a-word' for shard -5111 is not a uint64 decimal string
coverage_toc(<usable temporal section, envelope with no "order">) -> KeyError: 'order'
It bites hardest on coverage_toc, whose -> Toc | None reads as a total function and whose docstring makes absence the headline: a caller who dutifully handles the None arm still gets an unannounced ValueError out of a well-formed map with one corrupt word. That behavior is right (it is temporal_shard_words' documented content-level posture — degradation stops at the section gate); it is just undocumented at the surface a caller actually reads.
Fix: put "or raise" in both summary lines and add one clause each naming the raising inputs — coverage_moc the ranges_words malformed-range / missing-key contract, coverage_toc content-level corruption inside a well-formed map per temporal_shard_words. Neither raise is pinned through the new surface either; the corrupt-word case is a one-line addition to TestCoverageToc.
There was a problem hiding this comment.
🤖 from Claude
Accepted — fixed in 0acaefe. Both summary lines now use the module's "or raise" wording, and each names what propagates.
coverage_moc: "…as amortie.Moc— or raise", plus a clause naming theranges_wordscontract it inherits undegraded (missingranges/order→KeyError, malformed range — base-crossing, reversed, off-order — →ValueError) and stating there is no absence arm here at all; a caller who wants one asksparse_root_coveragefirst.coverage_toc: "…,Nonewhen the store publishes none — or raise", with the split made explicit up front: an UNREADABLE section degrades toNone, CORRUPT CONTENT inside a well-formed one raises pertemporal_shard_words(off-order shard id or non-uint64-decimal word →ValueError; carrier missingorder→KeyError). It says outright that handling theNonearm is not handling everything, and why that is deliberate — degradation stops at the section gate.- Both raises are pinned through the new surfaces:
test_corrupt_ranges_raise_through_the_cast(malformed range + deletedranges) andtest_corrupt_content_raises_past_the_none_gate(your{"-5111": "not-a-word"}case + deletedorder).
| "check_node_invariant", | ||
| "combined_hash", | ||
| "counts_from_composition", | ||
| "coverage_moc", |
There was a problem hiding this comment.
🤖 from Claude (review)
The one thing these two commits add to the package surface is the one thing no test touches.
coverage_moc / coverage_toc enter moczarr.__all__ here, but TestPublicSurface — the class that exists to pin exactly this split, and whose docstring says "a name added to one must move in the other" — was not extended, and every new test reaches them through the module: coverage.coverage_moc(...) in TestCoverageMoc/TestCoverageToc and at tests/test_open.py:827, :837, :856. Nothing in the suite ever evaluates moczarr.coverage_moc, so this export could be deleted and the whole 883-test suite would still pass.
It also misses the acceptance target on a technicality that is not cosmetic here: espg's ruled snippet is spelled at package level —
assert mz.coverage_moc(cov).contains(q_area)
assert mz.coverage_toc(cov).overlaps(q_when)— and TestMocTocAcceptance, whose docstring says it pins "the ruled acceptance target ... zagg's 07_minimal snippet", pins the module-qualified spelling instead. The class does not run the form that was ruled.
Fix, either half: extend TestPublicSurface with the two names (the shape is already there in test_temporal_trio_mirrors_its_spatial_twins — twin them with ranges_words / temporal_shard_words), and/or simply call moczarr.coverage_moc(...) / moczarr.coverage_toc(...) in TestMocTocAcceptance so the acceptance test exercises the ruled spelling end to end.
There was a problem hiding this comment.
🤖 from Claude
Accepted, both halves — fixed in 5a761ea.
TestPublicSurfacegainstest_typed_casts_are_a_package_level_pair: for each ofcoverage_moc/coverage_tocit asserts the name is inmoczarr.__all__, thatgetattr(moczarr, name) is getattr(coverage, name), and that it is not excluded bydocs/api/coverage.md's filter list — so the class's "a name added to one must move in the other" rule now covers these two as well.TestMocTocAcceptanceruns the ruled spelling end to end:import moczarr as mz, thenmz.coverage_moc(cov).contains(q_area),mz.coverage_toc(cov).overlaps(q_when), andaoi=mz.coverage_moc(cov)in the composed call — matching espg's snippet as written, with a comment at each site saying the package-level spelling is deliberate.
Deleting either export now fails the suite.
| # ``rank_to_xy``/``xy_to_rank`` deinterleave kernels (espg/mortie#150) | ||
| # the HHDC tensor layout is built on (issue #19), and 0.9.1 for the | ||
| # mixed-order geo kernels (mortie#116/#122). | ||
| "mortie>=0.9.10", |
There was a problem hiding this comment.
🤖 from Claude (review)
With this line landed, the PR body and the labels still describe the pre-phase-4 state, and the blocker they name no longer exists.
The body still carries phases 4 and 5 as unchecked, both annotated "Blocked by the mortie 0.9.10 release — ... the 0.9.10 tag is pushed, but the release is still building (not yet on PyPI), and the floor bump must name an installable version or CI's uv sync breaks", and the PR still carries the blocked label. Verified: mortie 0.9.10 is the current PyPI release (https://pypi.org/pypi/mortie/json → info.version == "0.9.10", and 0.9.10 present in releases), and it is what this worktree resolved — mortie.Moc and mortie.Toc both import from the installed 0.9.10, so CI's uv sync has an installable floor. "How tested" is stale on the same axis: it records 871 passed / 54 skipped, while head is 883 passed, 54 skipped (ruff check src tests and ruff format --check src tests also clean here).
Since the phases checklist and the label are how the next run and a human reader read where this PR stands, both need to move with these two commits.
One substantive item belongs in that same update: coverage_toc ships as -> mortie.Toc | None, while the ratified plan's phase 5 and the design comment that opened this issue both state coverage_toc(envelope) -> mortie.Toc. The reasoning for the deviation is argued at length in the docstring and I think it is the right call, but a deviation from a ratified plan is precisely what the body's "Questions for review" heading is for — right now it is visible only to someone reading the source, and it changes the ruled snippet's failure mode against a store with no temporal section (AttributeError on None rather than a failed assert).
There was a problem hiding this comment.
🤖 from Claude
Agreed on every point, and all of it is being handled in this run's completion step rather than by this fold — the PR body and labels are the parent's to move, not a code-fold commit's. Specifically, in flight now:
- phases 4 and 5 checked off, and their "Blocked by the mortie 0.9.10 release" annotations dropped now that 0.9.10 is the current PyPI release and is what this worktree resolves;
- the
blockedlabel removed; - "How tested" refreshed — head is now 890 passed, 54 skipped after this fold (883 at the sha you reviewed), with
ruff checkandruff format --checkclean; - a "Questions for review" entry naming the deviation:
coverage_tocships as-> mortie.Toc | Nonewhere the ratified plan's phase 5 said-> mortie.Toc, with the failure-mode consequence you flag (a store with no temporal section givesAttributeErroronNonerather than a failed assert in the ruled snippet).
One thing that did move here: the None arm widened in ae21862 — a section that lists zero shards now returns None too — so the deviation note covers that shape as well.
There was a problem hiding this comment.
🤖 from Claude
Correction to the count in the reply above: head after this fold is 887 passed, 54 skipped (not 890 — that was a transcription slip). 883 at the sha you reviewed, +4 from the fold (one raise pin per cast, the normalization-loss pin, and the package-surface pin); the retargeted empty-map test replaced one in place rather than adding. ruff check src tests and ruff format --check src tests clean at 6d7ce3b. 887 is the number going into the body.
| # bump signed off on the issue #19 thread). 0.9.1 remains the floor for | ||
| # everything else (mixed-order geo kernels, mortie#116/#122). | ||
| "mortie>=0.9.3", | ||
| # owned by the mortie spec (espg/mortie#62). Floor 0.9.10: the ``Moc`` |
There was a problem hiding this comment.
🤖 from Claude (review)
A mortie floor bump with no CHANGELOG.md entry.
The Unreleased section is where this repo records exactly this class of change, and the file shows it does so in both directions: the previous bump is spelled out — "Digest algebra is imported from zagg via the new moczarr[zagg] extra, never vendored; mortie floor is now >=0.9.3 (rank_to_xy/xy_to_rank)" (CHANGELOG.md:252) — and a change that pointedly did not move the floor says so — "Output is unchanged (pinned by the existing golden vectors plus a new scalar/batched parity pin); no mortie floor change" (CHANGELOG.md:135); the authalic re-pin even names the mortie version it tracks (CHANGELOG.md:70). A downstream pinner reads that file to know what to react to.
Nothing in this PR touches it: git diff 90b6532..dbdcc99 --name-only lists no CHANGELOG.md, so the entire issue #45 surface — the two typed casts, §10 zagg-coverage-toc/1 section parsing, candidate_leaves(..., when=) — plus a hard dependency floor moving >=0.9.3 → >=0.9.10 would land unrecorded.
Fix: one Unreleased entry before the ready-flip, in the style of the >=0.9.3 one — what the seam adds, and the floor as >=0.9.10 with its reason (the Moc/Toc types plus the toc word kernels the §10 seam runs on, which the pre-bump >=0.9.3 floor did not cover either).
There was a problem hiding this comment.
🤖 from Claude
Accepted — fixed in 6d7ce3b. One Unreleased entry at the top of CHANGELOG.md, in the style of the >=0.9.3 one you cite, covering the whole issue #45 surface rather than the floor alone: §10 zagg-coverage-toc/1 tier-1 decoding via TEMPORAL_SPEC/temporal_shard_words/temporal_keep (named as the twins of COVERAGE_SPEC/ranges_words/aoi_mask), candidate_leaves(..., when=) with §10.2's unlisted-is-unknown keep rule, and the two typed casts with coverage_toc's None arm and both casts' raise-on-corruption posture.
The floor line reads: mortie floor is now >=0.9.10 — the Moc/Toc coverage types of espg/mortie#197 / #199 that the casts return, plus the toc word kernels the §10 seam runs on; one bump for both per espg ruling 2 on the issue, and the previous >=0.9.3 floor covered neither. Both PR refs verified against the mortie repo ("Moc object: geometry-first coverage API", "Toc object: temporal coverage composing like Moc").
Also noted the ruling-3 normalizers (as_moc_words/as_toc_words) staying internal, so a downstream reader is not left looking for them.
|
🤖 from Claude All five phases are complete, every review finding folded, CI green — marking this ready for review. The gate cleared: mortie 0.9.10 published, carrying
Twenty findings across five cycles, every one folded — none declined — each with its own commit and a reply on its thread. Acceptance: the ruled assert mz.coverage_moc(cov).contains(mortie.moc(aoi))
assert mz.coverage_toc(cov).overlaps(q_when)
mz.candidate_leaves(root, manifest, aoi=mz.coverage_moc(cov), when=q_when)
# -> ['1/1/2/1/3/11213.zarr'], from metadata alone, no leaf openedBoth typed objects ride back into the seams through Implemented against the real 0.9.10 API, not the plan's assumptions — which caught two things worth naming: Two things want your eye (both in the body): the Gates at |
|
🤖 from Claude Label note, so the next routine run reads this PR's state correctly: it carries The state it would have encoded is unambiguous anyway: all five phases are done, every finding folded, CI green, and the PR is ready for review — the ball is with espg, nothing here is waiting on another PR. If you'd like the label to exist for the routine's label scan, say so and it's one command. |
1 participant
Closes #45
The mortie Moc/Toc seam: one boundary normalizer for AOI covers, §10
zagg-coverage-toc/1temporal-section parsing, and spatiotemporal shard pruning incandidate_leaves(..., when=)— the API that collapses zagg'sdemo/07_minimal.ipynbcoverage step. Implements the ratified plan (plan, rulings) and the two design comments at the top of the issue thread.Phases
as_moc_wordsboundary normalizer. One internal normalizer incoverage.py(importable, not in__all__, not on the docs surface — ruling 3): duck-typed__morton_moc__()protocol first (neverisinstanceof a mortie type; no mortie import for the check), then idempotentuint64pass-through, then the decimal-string formsopen.py's_aoi_wordshandled — which it replaces. Wired into every AOI-accepting seam:candidate_leaves/open_hive/open_store(viaopen_hive),pyramid.open_overview_order,MortonMocIndex.from_moc/MortonRanges.intersect(via_aoi_intervals),coverage.root_coverage_and/box_and/aoi_mask,intersect.iter_occupancy_and/occupancy_and(via the shared_setup), andstore.bitmap_and(public, found in the re-audit grep). Tests: a dunder-bearingFakeMoc(deliberately notmortie.Moc— the floor stays put), idempotence, and each seam accepting the fake against its packed-words answer.parse_root_coveragecarries thetemporalkey under §10's versioned-key discipline (strictspeccheck; unknown revision reads as absent, never an error; whole-section absence is "no temporal coverage published" and never a refusal). New tier-1 decoder: theshardsmap (D1 decimal shard ids at the carrier's order → toc word decimal strings) → arrays. Conformance: full mortie-toc/1 decode (toc2timestart/end ns) asserted against the vendored temporal fixture's recordedcentroid_spans_ns/obs_span_ns— the oracle PR Post-#463 fixture re-vendor sweep: whole-tree parity at zagg d52e3063, temporal companion fixture + spec-pin advance #44 reserved for this issue. The temporal fixture was re-vendored byte-identical from englacial/zaggmainat745ed732(coverage sidecar: versioned temporal section (per-shard toc envelopes + root time-digest) — land before the source-coop build englacial/zagg#480 regenerated it with the rootcoverage.moccarrying the §10 section plus theroot_coverageoracle; every array chunk is byte-identical to the prior vendoring, re-verifieddiff -r).candidate_leaves(..., when=). Ruled input grammar (ruling 4, permanent):(start, end)ISO strings /datetime64/ ns ints viamortie.span2toc; rawuint64toc words; any__toc_words__()protocol object. Pruning per §10: only a shard the tier-1 map LISTS whose word failsmortie.toc_overlapsis dropped; unlisted shards are always kept.when=Nonebyte-identical to today (pinned); notemporalsection →when=prunes nothing, silently.coverage_moc(envelope) -> mortie.Moc.Moc(ranges_words(envelope))— moczarr parses the"ranges"grammar, mortie types the words (noMoc.from_envelope;load_root_coveragestill returns the dict). Documented and tested as NOT the containment hot path: it expands to O(covered shards) words andMocthen compacts them eagerly, so.wordsis the compacted equivalent cover, notranges_wordselement-for-element —ranges_contain(rank space, O(ranges)) stays the fast path.coverage_toc(envelope) -> mortie.Toc | None. Tier-1 words in, typed out, riding__toc_words__()at the boundary. Tier-2 digest density gets no new surface (it rides the existing CDF machinery), per the design ruling. One floor bump covers both phases (ruling 2):mortie>=0.9.3→>=0.9.10, the release carryingMoc(Moc object: geometry-first coverage API (issue #196) mortie#197) andToc(Toc object: temporal coverage composing like Moc (issue #198) mortie#199).Approach
Phases 1–3 needed no mortie floor bump: mortie 0.9.9 already shipped the toc word kernels (
span2toc,toc_overlaps,toc2time,from_datetime64), and the protocol checks are duck-typed by design — which is what let the typedMoc/Toccompose in phases 4–5 with no change to the seams themselves. Normalization happens once at each public boundary; internals stay array-first. Phases 4–5 were implemented against the real mortie 0.9.10 API rather than the plan's assumptions (Moc.to_orderis the fixed-order cast, renamed from.atduring #197's review;Toc(source, end)takes words or times and normalizes eagerly), and both casts round-trip back into this package's own seams through the__morton_moc__()/__toc_words__()protocols.Acceptance: the ruled
07_minimalsnippet runs end to end, offline, on in-tree fixtures, and is pinned asTestMocTocAcceptance—mz.coverage_moc(cov).contains(moc(aoi)),mz.coverage_toc(cov).overlaps(q_when), andcandidate_leaves(root, manifest, aoi=q, when=...)with both typed objects passed straight in, returning the spatiotemporal leaf roster from metadata alone.How tested
ruff check/ruff format --checkclean; full suitepytest -qgreen (887 passed, 54 skipped at head; 798 baseline). New tests ride the existing fixtures (SERC hive, conftest hive store, the intersect golden pair, the vendored zagg spec fixtures) — no network, no new fixtures fetched.Resolved during review
add97ac):TEMPORAL_SPEC,temporal_shard_wordsandtemporal_keepnow sit inmoczarr.__all__and on the docs page, as the exact twins ofCOVERAGE_SPEC/ranges_words/aoi_mask. The deciding parallel is phase 4's own shape —coverage_mocis the typed spatial cast andranges_wordsstays public beside it, so the typedcoverage_tocshould not gate its temporal equivalents either. The module now splits on ONE rule: the boundary normalizers (as_moc_words,as_toc_words) are internal per ruling 3, everything else is public. That also fixed a live defect —as_toc_wordsdocumented itself as off the docs surface but was never filtered, so it was rendering.TestPublicSurfacepins both halves against drift. Flagging for espg: this is a public-surface addition the plan did not explicitly rule; say the word and it reverts to internal in one commit.Questions for review
Deviation from the plan's signature, flagged deliberately: phase 5 is
coverage_toc(envelope) -> mortie.Toc | None, not-> mortie.Toc.Nonemeans the store publishes no readable tier-1 coverage — no section, an unknown revision, a section missingshards, or a section that lists no shards. The reason is §10's absence rule: an emptyTocwould answer.overlaps(q)Falsefor every query, which is the false negative §10 forbids ("a shard the section does not list is not proof the shard has no data in the window"), so absence surfaces asNone— the house idiomparse_root_coverage/load_root_coveragealready use — and the caller must decide rather than be handed a confident wrong answer. Consequence for the ruled snippet:mz.coverage_toc(cov).overlaps(q_when)needs aNoneguard on a store that publishes no temporal section. Say the word if you'd rather have the total signature and an emptyToc.pre-commit run --all-filesshows 4 pre-existing mypy errors (moc_index.py:375,intersect.py:380/383and one more) that reproduce on a cleanorigin/maincheckout with the pinned hook — presumably hook-env drift (newer mypy/numpy). Not touched here per the don't-fix-unrelated rule; flagging for a separate small-fix.