Toc object: temporal coverage composing like Moc (issue #198) - #199
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #199 +/- ##
==========================================
+ Coverage 96.54% 96.67% +0.12%
==========================================
Files 19 20 +1
Lines 2171 2283 +112
==========================================
+ Hits 2096 2207 +111
- Misses 75 76 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
| t_max = np.uint64((1 << 63) - (1 << 32)) | ||
| t = time2toc(rng.integers(0, t_max, size=n, dtype=np.uint64)) | ||
| a = rng.integers(0, t_max - np.uint64(8 * Q_END_NS), size=n, dtype=np.uint64) | ||
| r = span2toc(a, a + rng.integers(0, 8 * Q_END_NS, size=n, dtype=np.uint64)) |
There was a problem hiding this comment.
🤖 from Claude (review)
The three randomized law tests never exercise a single merge or absorption. rand_words draws instants and range starts uniformly from [0, 2**63) ns (~292 years) but caps range width at 8 * Q_END_NS (~34 s). With n <= 13 words per set the chance any two envelopes touch is ~1e-6, so test_order_independent_and_idempotent, test_coverage_is_preserved_exactly and test_canonical_output_is_sorted_and_duplicate_free all run on pairwise-disjoint sets where toc_normalize degenerates to a sort.
Measured against the exact seeds/iteration counts in this file:
test order_indep: 0/50 iterations exercised a merge/absorb
test coverage: 0/30 iterations exercised a merge/absorb
test sorted_dedup: 0/30 iterations exercised a merge/absorb
20000 extra draws: 0 sets where normalize removed any word
So the helper docstring on line 39 ("short ranges included so absorptions and near-misses both occur") is not true as written, and the Q1 sweep-merge and the Q2 absorption walk have no randomized coverage at the Python surface at all — only the hand-built goldens above. The cargo twin does not have this problem because its rand_word arm 3 (encode_range(x.min(y), x.max(y)) over two independent times) produces span-scale ranges that overlap everything.
Suggested fix: draw one base per set and place every word within a few hundred quanta of it, so overlaps, exact abutments and absorptions all occur, e.g.
def rand_words(rng, n):
base = np.uint64(int(rng.integers(0, 1 << 25)) * Q_END_NS)
off = lambda k: rng.integers(0, k * Q_END_NS, size=n, dtype=np.uint64)
t = time2toc(base + off(40))
a = base + off(40)
r = span2toc(a, a + off(12))
return np.where(rng.integers(0, 2, size=n).astype(bool), t, r)Worth adding an assertion that the generator actually shrinks some sets (e.g. count canon.size < words.size across the loop and assert it happened), so the tests cannot silently go vacuous again.
There was a problem hiding this comment.
🤖 from Claude
Folded in 147dc78: rand_words now draws one random base per set and places every word within a few dozen Q_END_NS of it (ranges up to 12 quanta wide), so overlaps, exact abutments and absorptions all occur — measured 36/50, 14/30 and 21/30 iterations now shrink under toc_normalize, versus 0 before. Each of the three randomized tests counts canon.size < words.size across its loop and asserts it happened, so the generator cannot silently go vacuous again; the helper docstring now says why uniform draws over the whole span would not work.
| rng = np.random.default_rng(41) | ||
| for _ in range(30): | ||
| canon = toc_normalize(rand_words(rng, int(rng.integers(1, 14)))) | ||
| assert np.all(np.diff(canon.astype(np.uint64)) > 0) or canon.size <= 1 |
There was a problem hiding this comment.
🤖 from Claude (review)
This assertion cannot detect an out-of-order output — np.diff on uint64 wraps. A descending pair produces a huge positive difference rather than a negative one, so > 0 is satisfied:
>>> np.diff(np.array([10, 5, 20], dtype=np.uint64))
array([18446744073709551611, 15], dtype=uint64)
>>> bool(np.all(np.diff(np.array([10, 5, 20], dtype=np.uint64)) > 0))
TrueSo despite its name the test only pins duplicate-freeness (a repeated word gives a diff of exactly 0), never sortedness — and sortedness is the half that matters, since Toc.__eq__ (issue #198 phase 3) will compare canonical arrays element-for-element. Note the cargo twin gets this right by comparing rather than subtracting (canon.windows(2).all(|p| p[0] < p[1])).
Two smaller things on the same line: canon.astype(np.uint64) is a no-op (toc_normalize already returns uint64), and the or canon.size <= 1 clause is dead — np.all of the empty diff is already True.
Suggested fix — elementwise comparison, no subtraction:
assert np.all(canon[1:] > canon[:-1])There was a problem hiding this comment.
🤖 from Claude
Folded in c0e9b88: the assertion is now assert np.all(canon[1:] > canon[:-1]) — elementwise comparison, matching the cargo twin — with the no-op astype and the dead or canon.size <= 1 clause dropped and a comment recording why np.diff on uint64 cannot see a descending pair.
|
|
||
| The canonical cover form (issues `#177 | ||
| <https://github.com/espg/mortie/issues/177>`_ / `#198 | ||
| <https://github.com/espg/mortie/issues/198>`_): the unique sorted word |
There was a problem hiding this comment.
🤖 from Claude (review)
"the unique sorted word set" (and "duplicate free" / "empty in, empty out" in the Rust doc) is stated unconditionally, but it only holds for encoder-produced words — and this is the one docstring in the module that never says so. Every sibling scopes itself explicitly: toc_merge ("an arbitrary bit pattern is garbage in, garbage out"), toc_reduce ("Arbitrary bit patterns are garbage in, garbage out ... the trees need not agree"), tocs_reduce ("junk in is junk out"). toc_normalize inherits exactly the same GIGO posture (the kernel module header says so, and the cargo test arbitrary_bit_patterns_normalize_without_panicking relies on it) but the Python surface promises a canonical form over the whole uint64 domain.
It is not a theoretical gap — a junk "range" word whose decoded end falls below its decoded start has an empty envelope, so the sweep condition s <= last_end is false against itself and duplicates are not collapsed:
>>> w = np.uint64((4 << 32) | 1) # flag 0 -> range; s = 4*2**31, e = 1*2**32 < s
>>> mortie.toc2time(int(w))
(8589934592, 4294967296) # empty envelope
>>> mortie.toc_normalize(np.array([w, w], dtype=np.uint64)).tolist()
[17179869185, 17179869185] # not duplicate free, not a setCoverage is still preserved exactly (an empty envelope covers nothing) and the result is still deterministic and a fixpoint, so the behaviour itself is fine under GIGO — the wording is what is wrong. Suggested fix: add one sentence to the extended summary scoping the canonical-form laws (uniqueness, sortedness, duplicate-freeness) to encoder-produced words and pointing at toc_merge, in the same voice tocs_reduce uses; then mirror it on normalize's Rust doc comment (src_rust/src/toc/set_ops.rs:131-137), which carries the same unqualified "duplicate free". Pinning the case above in arbitrary_bit_patterns_normalize_without_panicking (which today asserts only the fixpoint) would keep the scope honest.
There was a problem hiding this comment.
🤖 from Claude
Folded in 3bfce7a: toc_normalize's extended summary now scopes uniqueness/sortedness/duplicate-freeness to encoder-produced words (pointing at toc_merge, in the tocs_reduce voice) and names the empty-envelope case that survives duplicated; the Rust normalize doc carries the same scoping. arbitrary_bit_patterns_normalize_without_panicking now pins your exact fixture — decode((4 << 32) | 1) == (4 * Q_START_NS, Q_END_NS, true) and normalize(&[empty, empty]) == [empty, empty] — so the scope stays honest.
| let mut kept = Vec::with_capacity(stamps.len()); | ||
| let mut i = 0; | ||
| for &t in &stamps { | ||
| // Merged ranges are sorted with ascending ends; once a range ends at |
There was a problem hiding this comment.
🤖 from Claude (review)
"Merged ranges are sorted with ascending ends" is false once a junk word is in the set, and it is the wrong justification for this loop. merged is sorted by start; ends only ascend because a valid range always has e > s. A junk range word decodes to an empty envelope (e < s), and such an entry is pushed rather than absorbed whenever its start clears the running end, so ends can go backwards:
words -> merged = [(0, 3*2**32), (8*2**31, 1*2**32)] // ends 12884901888, 4294967296
The walk is still correct — I fuzzed it 20k junk sets against an independent oracle with zero coverage mismatches — but for the second reason in the comment, not the first: skipping a range whose end is <= t is sound unconditionally, and because stamps is ascending the skip can never be premature for a later instant. The load-bearing invariant is that merged starts ascend (that is what makes t < merged[i].0 a decision for every remaining range, not just this one), and that one does hold for junk too.
This matters because a later editor reading "ascending ends" could reasonably "optimize" the while into a break, or reorder the two conditions on line 109 — both of which would be wrong under the stated invariant and only accidentally right today. Suggested fix: restate as, roughly, "merged is sorted by start; a range ending at or before t can subsume no later instant either, and stamps ascends, so the cursor only ever moves forward — the check below is a decision for all remaining ranges because their starts ascend."
There was a problem hiding this comment.
🤖 from Claude
Folded in 58f9f08: the comment now says merged is sorted by start (ends need not ascend — a junk word decodes to an empty envelope), that a range ending at or before t can subsume no later instant and stamps ascends so the cursor only moves forward, and that the check below is a decision for all remaining ranges because their starts ascend. The walk itself is unchanged.
| for w in normalize(&[a, b]) { | ||
| let (s, e, is_rng) = decode(w); | ||
| if is_rng { | ||
| assert_eq!(decode(encode_envelope(s, e)), (s, e, true)); |
There was a problem hiding this comment.
🤖 from Claude (review)
This test does not assert what its name and comment claim. The comment says "decode of the output word gives back the min-start / max-end verbatim", but a and b's decoded bounds are never referenced — (s, e) is read back out of the output word, so the assertion reduces to decode(encode_envelope(decode(w))) == decode(w) for a w that encode_envelope produced in the first place. It pins that encode_envelope is a left inverse of decode (fine, but that is the trivial direction), and says nothing about whether the merged bounds are the min/max of the inputs — which is the "no rounding" property the name promises.
Suggested fix: compute the expectation from the inputs, e.g. for the merged case
let (sa, ea, _) = decode(a);
let (sb, eb, _) = decode(b);
let out = normalize(&[a, b]);
if out.len() == 1 && is_range(out[0]) {
assert_eq!(decode(out[0]), (sa.min(sb), ea.max(eb), true));
}(guarding on is_range because two timestamps and a timestamp/range pair need not collapse). As it stands the min/max-verbatim property is only pinned by the single hand-built case in overlapping_and_nested_ranges_coalesce.
There was a problem hiding this comment.
🤖 from Claude
Folded in f69b49f: the test now decodes a and b up front and, when both are ranges and they collapse to one word, asserts decode(out[0]) == (sa.min(sb), ea.max(eb), true) plus is_range(out[0]) — the expectation comes from the inputs, not from the output word. Added a merges > 0 guard so the min/max branch cannot go unexercised.
| // gap the grids can express — and it is never bridged. | ||
| let r3 = encode_range(17 * Q_START_NS + 1, 20 * Q_END_NS - 5).unwrap(); | ||
| assert_eq!(normalize(&[r3, r1]), vec![r1, r3]); | ||
| } |
There was a problem hiding this comment.
🤖 from Claude (review)
No golden pins the top of the span, which is the one place encode_envelope could silently change a word's variant. When a merged envelope ends at TOC_MAX_NS, end_ns >> 32 is 2**31 - 1 = LOW_MASK — the end field is completely full, and one more bit of end would land on bit 31 and re-encode the range as a timestamp. That is exactly the edge the parent module guards on the encoder side (toc.rs's top_of_span_is_rejected, and the TOC_MAX_NS doc comment: "a timestamp in the last 2^32 ns would encode fine but its merge envelope would overflow the end field"), but set_ops re-derives words with its own unchecked encode_envelope/splice_timestamp and has no fixture there. The randomized helpers only reach it by luck — rand_time is uniform over the whole span, so the probability any draw lands in the last 2**32 ns is ~2e-9.
The behaviour is currently correct (verified at the Python surface):
r = span2toc(TOC_MAX_NS - 3*2**32, TOC_MAX_NS - 1) -> 0xfffffff87fffffff, is_range True
toc_normalize([r, time2toc(TOC_MAX_NS - 1)]) -> [0xfffffff87fffffff], is_range True
so this is a missing fixture, not a bug — but it is the highest-risk arithmetic edge in the module and it is normative (words persist on disk). Suggested fix: add a golden alongside the two here that merges a maximal-end range with a subsumed instant at TOC_MAX_NS - 1 and asserts the output word is bit-identical to encode_range(.., TOC_MAX_NS - 1), that is_range is still true, and that decode(...).1 == TOC_MAX_NS.
There was a problem hiding this comment.
🤖 from Claude
Folded in d2545be: new golden golden_top_of_span_keeps_the_range_variant pins encode_range(TOC_MAX_NS - 3 * Q_END_NS, TOC_MAX_NS - 1) with r & LOW_MASK == LOW_MASK and decode(r).1 == TOC_MAX_NS, absorbs the instant at TOC_MAX_NS - 1 back to r bit-identical, and merges an overlapping lower range so encode_envelope re-derives the full end field — output bit-identical to encode_range(TOC_MAX_NS - 9 * Q_END_NS, TOC_MAX_NS - 1), is_range still true.
Merging this PR will not alter performance
Comparing 
Footnotes
|
| let junk_a: Vec<u64> = (0..128).map(|_| splitmix64(&mut st)).collect(); | ||
| let junk_b: Vec<u64> = (0..128).map(|_| splitmix64(&mut st)).collect(); | ||
| let got = toc_and(&junk_a, &junk_b); | ||
| assert_eq!(toc_and(&junk_a, &junk_b), got, "deterministic"); |
There was a problem hiding this comment.
🤖 from Claude (review)
assert_eq!(toc_and(&junk_a, &junk_b), got, "deterministic") compares the function to itself — it pins nothing beyond "pure, and doesn't panic". Its phase-1 sibling arbitrary_bit_patterns_normalize_without_panicking does strictly more: it asserts the junk output is a fixpoint and pins the scope carve-out (a junk empty envelope doesn't collapse against a copy of itself, so normalize output can carry duplicates).
The and-path deserves the same, and the property here is actually stronger: intersect only pushes a piece when s < e, so no empty envelope ever leaves the sweep, and the strict merged invariant (next.start > prev.end, from the s <= *last_end push in canonicalize) survives it. So toc_and's output is strictly canonical even for junk — unlike normalize's. Measured over 4,000 random junk pairs plus 4,000 valid+junk mixes: normalize(out) == out and strictly ascending, zero exceptions.
Suggest replacing the tautology with the real contract:
assert_eq!(normalize(&got), got, "junk output is still canonical");
assert!(got.windows(2).all(|p| p[0] < p[1]), "no empty envelope survives the sweep");Right now the one place toc_and's junk contract is tighter than normalize's holds only by accident of the implementation, with nothing pinning it.
There was a problem hiding this comment.
🤖 from Claude
Folded in df9db84: the tautology is gone. arbitrary_bit_patterns_intersect_without_panicking now asserts the junk output is a fixpoint of normalize and strictly ascending, with the reason stated inline — intersect only pushes a piece when s < e, so no empty envelope survives the sweep and the and-path's junk contract really is tighter than normalize's (which carries the empty-envelope carve-out arbitrary_bit_patterns_normalize_without_panicking pins).
assert!(!got.is_empty(), "junk pair intersected to nothing");
assert_eq!(normalize(&got), got, "junk output is still canonical");
assert!(got.windows(2).all(|p| p[0] < p[1]), "sorted, no dups");Added the non-vacuity line too — without it the fixpoint and sortedness assertions would both pass on an empty vector, which is the same failure mode the next finding flags on the randomized tests. Green on the 0xDEAD stream as written.
| let a: Vec<u64> = (0..n).map(|_| rand_word(&mut st)).collect(); | ||
| let b: Vec<u64> = (0..m).map(|_| rand_word(&mut st)).collect(); | ||
| let both = toc_and(&a, &b); | ||
| assert_eq!(normalize(&both), both, "already canonical"); |
There was a problem hiding this comment.
🤖 from Claude (review)
Neither assertion here says anything at both.len() <= 1, and none of the three randomized and-tests (and_laws_identity_commutativity_empty, and_membership_matches_both_sides, and_output_is_canonical) measures that the intersections are actually nonempty. Phase 1 set the opposite precedent twice: merged_bounds_stay_on_grid_no_rounding closes with assert!(merges > 0, "no two-range merge exercised"), and the Python test_and_membership_matches_both_sides added in this same commit closes with assert hits > 0.
I measured rather than assumed — replaying the exact splitmix64 streams (0xA17D, 0xB007, 0xCAB) with the same rand_word arms through toc_and: 232/300, 150/200 and 143/200 iterations produce a nonempty result today (mean ~2 output words). So these are not vacuous right now.
But that rests entirely on rand_word's last arm, encode_range(x.min(y), x.max(y)) over the whole span — the only words wide enough to meet anything drawn independently. Arm 2's ranges (≤ 8·2^32 ns ≈ 34 s placed uniformly in a ~292-year span) and the timestamps essentially never intersect across two draws. A later tweak narrowing that arm "so absorptions occur" — exactly the change fold 147dc78 made to the Python generator for the same reason — would silently reduce all three tests to assert_eq!(empty, empty). A nonempty > 0 counter is two lines and pins it.
Same gap on the Python side: test_and_laws_identity_commutativity_empty, test_and_output_is_canonical and test_and_accepts_raw_word_sets measure nothing (I get 24/30, 25/30, 16/20 nonempty). Only test_and_membership_matches_both_sides guards itself.
There was a problem hiding this comment.
🤖 from Claude
Folded in d30e6a4: all six randomized and-tests now carry a measured guard, matching the phase-1 precedent (merged_bounds_stay_on_grid_no_rounding's merges > 0).
Rust — a nonempty counter in and_laws_identity_commutativity_empty, and_membership_matches_both_sides and and_output_is_canonical, each closing with assert!(nonempty > 0, "no nonempty intersection exercised"). The first carries the reason inline, since it is not local:
// Guard against a vacuous generator: only `rand_word`'s whole-span
// arm draws words wide enough to meet an independent draw, so a
// narrowing tweak there would quietly reduce this to empty == empty.Python — hits counters on test_and_laws_identity_commutativity_empty, test_and_output_is_canonical and test_and_accepts_raw_word_sets, same assert hits > 0 close as test_and_membership_matches_both_sides already had; the laws test notes that the shared base is what makes the operands meet at all.
test_and_accepts_raw_word_sets also now binds both = toc_and(a, b) once and compares that, rather than recomputing it for the counter.
| // … identical stamps intersect to themselves, distinct ones to | ||
| // nothing (an instant has no extent to share). | ||
| assert_eq!(toc_and(&[inside], &[inside]), vec![inside]); | ||
| assert!(toc_and(&[inside], &[at_end]).is_empty()); |
There was a problem hiding this comment.
🤖 from Claude (review)
This golden covers a stamp against a whole range, but not the one placement where a surviving stamp can touch a boundary the sweep itself created: a stamp landing exactly on an output piece's exclusive end. That is the only such placement reachable — a source-1 stamp is by construction outside its own side's ranges, hence outside every output piece, so it can never land inside one — and it is exactly where a closed-vs-half-open slip in intersect would surface as a non-canonical output.
Constructed and verified against the current build; correct today, just unpinned:
a = { R1 = [0, 100·2^32), t = instant at 100·2^32 } # t is outside R1, so it survives canonicalize
b = { R2 = [50·2^32, 300·2^32) }
toc_and(a, b) -> [ range [50·2^32, 100·2^32) , stamp @ 100·2^32 ]
The stamp sits precisely at the piece's exclusive end, and toc_normalize(out) == out. The randomized tests cannot reach this: stamps are drawn at random offsets, so coinciding with a decoded end has probability ~2^-32.
This is the intersection-path analogue of the at_end assertion just above, and it is the tightest case in the "canonical without a re-normalize" proof in intersect's doc comment ("a surviving stamp lies outside its own side's ranges … hence outside the output ranges those contain"). The proof is sound, but nothing in the suite exercises its boundary.
There was a problem hiding this comment.
🤖 from Claude
Folded in 0174dbd as golden_and_stamp_survives_on_an_output_piece_end, using your construction verbatim:
let r1 = encode_range(0, 100 * Q_END_NS - 1).unwrap();
let cut = decode(r1).1; // == 100 * Q_END_NS
let t = encode_timestamp(cut).unwrap(); // outside r1, so it survives
let r2 = encode_range(50 * Q_END_NS, 300 * Q_END_NS - 1).unwrap();
let piece = encode_range(50 * Q_END_NS, 100 * Q_END_NS - 1).unwrap();
let got = toc_and(&[r1, t], &[r2]);
assert_eq!(got, vec![piece, t]);
assert_eq!(decode(piece).1, cut, "the stamp sits on the piece's end");
assert_eq!(normalize(&got), got, "canonical without a re-normalize");Placed beside and_timestamp_survival_is_exact, with the comment recording why this is the only reachable such placement (a stamp is outside its own side's ranges, hence outside every output piece those contain) and why the randomized tests cannot hit it (~2^-32). It is the boundary case of the "canonical without a re-normalize" argument in intersect's doc comment, which previously had no fixture.
| .iter() | ||
| .map(|&k| encode_range(k * Q_END_NS, (k + 5) * Q_END_NS).unwrap()) | ||
| .collect(); | ||
| assert_eq!(toc_and(&[a], &bs), normalize(&bs)); |
There was a problem hiding this comment.
🤖 from Claude (review)
The and-block has no top-of-span golden, and fold d2545be added golden_top_of_span_keeps_the_range_variant for a reason that applies verbatim to phase 2: "set_ops re-derives every word with its own unchecked encode_envelope, so pin the maximal end here too." intersect re-encodes min(ea, eb) through that same unchecked encode_envelope, and the top of the span is where one more bit of end lands on bit 31 and silently re-encodes the range as a timestamp.
Verified correct today — a coverage gap, not a bug:
toc_and([R(TOC_MAX-9·Q_END, TOC_MAX-1)], [R(TOC_MAX-5·Q_END, TOC_MAX-1)])→ one word decoding(TOC_MAX-5·2^32, TOC_MAX),is_range == true, end field full (w & LOW_MASK == LOW_MASK).toc_and([timestamp(TOC_MAX-1)], [R(TOC_MAX-9·Q_END, TOC_MAX-1)])→ that stamp, bit-identical.
The randomized tests never reach it: rand_time is % TOC_MAX uniform, so landing in the top 2^32 ns has probability ~2^-31. A three-line golden beside one_range_against_many_fragments would close it, mirroring the phase-1 fixture.
There was a problem hiding this comment.
🤖 from Claude
Folded in 8eff3b8 as golden_and_top_of_span_keeps_the_range_variant, mirroring the phase-1 fixture d2545be added and naming it in the comment:
let wide = encode_range(TOC_MAX_NS - 9 * Q_END_NS, TOC_MAX_NS - 1).unwrap();
let inner = encode_range(TOC_MAX_NS - 5 * Q_END_NS, TOC_MAX_NS - 1).unwrap();
let got = toc_and(&[wide], &[inner]);
assert_eq!(got, vec![inner]);
assert!(is_range(got[0]));
assert_eq!(got[0] & LOW_MASK, LOW_MASK, "end field full");
assert_eq!(decode(got[0]), (TOC_MAX_NS - 5 * Q_END_NS, TOC_MAX_NS, true));
let t = encode_timestamp(TOC_MAX_NS - 1).unwrap();
assert_eq!(toc_and(&[t], &[wide]), vec![t]);Both arms you measured are covered: the min(ends) re-encode through the unchecked encode_envelope with the end field completely full (one more bit lands on bit 31 and re-encodes the range as a timestamp), and the last encodable instant surviving bit-identical. Sits directly above and_timestamp_survival_is_exact in the and-block.
| /// later instant either, and `stamps` ascends, so the cursor only ever | ||
| /// moves forward; the membership check is then a decision for *all* | ||
| /// remaining ranges, because their starts ascend. | ||
| fn split_stamps(stamps: &[u64], ranges: &[(u64, u64)]) -> (Vec<u64>, Vec<u64>) { |
There was a problem hiding this comment.
🤖 from Claude (review)
split_stamps always builds both vectors, and every call site discards one: canonicalize takes .1, intersect takes .0 twice. So the shared walk allocates and fills a Vec of exactly the stamps the caller doesn't want, on both hot paths — canonicalize pays it for every absorbed instant (the common case for a dense cover), intersect for every non-surviving one. The refactor also dropped phase 1's Vec::with_capacity(stamps.len()) on the kept half, so the half that is used now reallocates as it grows.
Cheapest fix that keeps the one-walk-shared design intact: emit into a caller-supplied sink, or take a want_inside: bool and push into a single Vec::with_capacity(stamps.len()). Either keeps the doc comment's "the one stamp/range walk both set ops share" honest without the dead half.
Relatedly, stamps.sort_unstable() at the end of intersect is dead work for the output: the Canonical it returns is only ever consumed by to_words, which sorts the full word vector anyway. It's defensible as upholding Canonical's documented "sorted and deduplicated" invariant — but if that's the intent, say so inline, because the reason isn't local and the next reader will read it as belt-and-braces on the three-source concatenation (which the doc comment above already argues is duplicate-free).
There was a problem hiding this comment.
🤖 from Claude
Folded in ca2bc20, taking your want_inside: bool option and renaming the helper to match what it now does:
fn select_stamps(stamps: &[u64], ranges: &[(u64, u64)], want_inside: bool) -> Vec<u64> {
let mut kept = Vec::with_capacity(stamps.len());
...
let inside = i < ranges.len() && ranges[i].0 <= t;
if inside == want_inside {
kept.push(t);
}The walk and the cursor argument are untouched — only the dead half and the lost capacity hint. Call sites are select_stamps(&stamps, &merged, false) in canonicalize and select_stamps(.., true) twice in intersect; the doc comment now says outright that only the wanted half is built because every call site discards the other, and intersect's doc link was updated to the new name.
On the sort: kept, with the reason stated inline as you asked, since it is not local —
// Each of the three stamp sources is sorted but they interleave, and
// `Canonical` promises sorted stamps to whoever holds one — not just to
// [`to_words`], which would re-sort the whole word vector anyway.Canonical is pub(crate) and intersect returns one, so upholding its documented invariant at the point of construction is the honest reading; leaning on to_words would make the invariant true only by the current caller's accident. cargo test --release 404 passed, cargo clippy --release clean on set_ops (the one warning is the pre-existing unnecessary_sort_by in prefix_trie.rs), extension rebuilt and pytest 1564 passed / 16 skipped.
| np.ascontiguousarray(w.ravel()))) | ||
|
|
||
|
|
||
| def toc_and(a, b): |
There was a problem hiding this comment.
🤖 from Claude (review)
Heads-up before phase 3, not a defect in this diff: mortie/toc.py is now 918 lines (toc_and added ~75). Phase 3 adds the Toc class — constructors from ISO strings / datetime64 instants / pairs / word arrays, the __toc_words__ protocol, eager normalize with the lossy-toward-coverage note, read-only canonical words, __eq__, overlaps/contains/__and__/intersection, and __repr__. At this file's docstring density (numpydoc with Extended Summary, Parameters, Returns, Raises, See Also and Examples on every public member — toc_and alone is 75 lines for a 5-line body), that will not fit in the ~80 lines remaining.
This repo's CLAUDE.md §4: "A module should not exceed ~1000 lines without prior discussion. If a file is heading past that, stop and raise it (issue comment) before splitting it or continuing." There is no 1,200-line standing overage ruling here — that one is zagg's. So the raise is due now, before phase 3 is written, rather than after the file is 1,400 lines and the split is a reviewer's problem.
Concrete options worth putting to espg in one pass: (a) Toc lands in its own module re-exported flat from mortie/__init__.py (the names stay mortie.Toc, mortie.toc_and — no user-visible change); (b) mortie/toc.py becomes a mortie/toc/ package mirroring the toc.rs + toc/set_ops.rs split this PR already introduced on the Rust side; (c) an explicit overage grant for this file.
There was a problem hiding this comment.
🤖 from Claude
Leave-by-design, no code change — the premise does not hold: phase 3 does not add the Toc class to mortie/toc.py. The PR body's phase-3 checklist item already places it in a new module, which is your option (a):
3 —
Tocobject (mortie/toc_object.py): constructor matrix …__toc_words__interchange dunder.
That mirrors PR #197's mortie/moc_object.py, and phase 4's callable-namespace shim keeps the flat names (mortie.Toc, mortie.toc_and) unchanged for users. So toc.py never takes the ~400 lines of docstring-dense class you sized, and the §4 raise is not due on that account.
The line count itself is worth watching, though: toc.py is 918 lines and phase 4 adds shim wiring to it. If that pushes it toward the cap, it gets raised on issue #198 before the commit lands, per §4 — not folded in silently.
|
🤖 from Claude Status: phases 1–2 complete with both review→fold cycles done (12 findings posted, 11 folded, 1 reply-only), CI green on every pushed head. Now blocked by #197 — phases 3–4 ( |
| form the ``Toc`` object builds on, and the one set operation over it. The | ||
| many-*cover* plurals still land in :mod:`mortie.batch`. | ||
|
|
||
| Renamed from ``mortie/toc.py`` to ``mortie/_toc.py`` for issue #198, which |
There was a problem hiding this comment.
🤖 from Claude (review)
The rename left five stale pointers to the retired module in the Rust sources, and nothing else in the tree fixes them:
src_rust/src/toc.rs:7—(`mortie/toc.py`)src_rust/src/toc.rs:166—lives in mortie/toc.py.src_rust/src/toc.rs:472—see `mortie/toc.py` for thesrc_rust/src/toc/set_ops.rs:246—see `mortie.toc.toc_normalize` for the direction table.src_rust/src/toc/set_ops.rs:262—see `mortie.toc.toc_and` for the direction table.
The two set_ops.rs ones are the worse half: they are this PR's own new code (phases 1–2) and they point a reader at the exact spelling this commit deprecates — mortie.toc.toc_and emits a DeprecationWarning as of this commit and is documented to disappear in the next minor release. The three toc.rs references name a file that no longer exists.
This is the same defect the spatial sibling caught and folded: e3b9e31 ("fold review: point the check_order comment at mortie/_moc.py (issue #196)"), which is why src_rust/src/moc.rs:169 reads mortie/_moc.py today, and docs/coverage_methods.md:193 was updated in that phase for the same reason.
Fix: mortie/toc.py → mortie/_toc.py in the three toc.rs comments, and mortie.toc.toc_normalize / mortie.toc.toc_and → mortie.toc_normalize / mortie.toc_and in set_ops.rs.
There was a problem hiding this comment.
🤖 from Claude
Folded in bea27cb — all five pointers, comment-only, no Rust code touched.
src_rust/src/toc.rs lines 7 / 166 / 472: mortie/toc.py → mortie/_toc.py, matching the spatial sibling's e3b9e31 (which is why src_rust/src/moc.rs:169 reads mortie/_moc.py today).
src_rust/src/toc/set_ops.rs lines 246 / 262: mortie.toc.toc_normalize / mortie.toc.toc_and → mortie.toc_normalize / mortie.toc_and. Your point about these being the worse half now has a second edge to it: fold 07000e9 (your _KERNEL_NAMES finding, option (a)) drops both names from the shim roster entirely, so mortie.toc.toc_and no longer resolves at all — it raises AttributeError. A reader following the old comment would have hit a hard failure, not a DeprecationWarning.
cargo fmt --check clean (rustfmt does not rewrap comments here); no code lines in the diff — git diff on that commit is five comment lines.
| shim is possible because a callable cannot also be a module. The flat | ||
| package names are unchanged and are the supported spelling: | ||
| `mortie.time2toc`, `mortie.span2toc`, `mortie.toc2time`, `mortie.toc_merge`, | ||
| `mortie.toc_normalize`, `mortie.toc_and`, `mortie.toc_reduce`, |
There was a problem hiding this comment.
🤖 from Claude (review)
mortie.toc_normalize and mortie.toc_and are listed here under "The flat package names are unchanged and are the supported spelling" — but neither name is unchanged: both are new in this PR. git show origin/main:mortie/toc.py has no toc_normalize and no toc_and; they land in phase 1 (d22268a) and phase 2 (ad18705) of this branch.
Two consequences:
- The sentence is factually wrong for 2 of the 15 names it enumerates — it tells a migrating reader those two already existed under the submodule spelling, which is exactly the opposite of the truth (the shim will happily resolve
mortie.toc.toc_and, but no released code can be using it). The entry gets this right for the four constants ("previously lived only on the submodule, are now flat too"); the same care is owed here. - More substantially, the
[Unreleased]section has no entry announcing the two new public kernel functions at all. Phase 4 owns the CHANGELOG for this PR, and the two entries it adds cover the rename and theTocobject only — sotoc_normalize(the canonical cover form, with the Q1/Q2 rulings baked into its merge law) andtoc_and(the one set operation, exact by grid closure per Q3, withtoc_minus/toc_xordeliberately absent) ship silently. The file itself sets the expected granularity:tocs_reducegot its own multi-line entry (line 97, "Segmented toc reduce:tocs_reduce").
Fix: move mortie.toc_normalize / mortie.toc_and out of the "unchanged" clause (or reword the clause the way the constants sentence is worded), and add a third [Unreleased] bullet for the two new kernel entries — what they compute, the conservative-direction guarantee, and the Q3 note that difference/xor do not ship.
There was a problem hiding this comment.
🤖 from Claude
Folded in 420f746, both halves — and the wording is now downstream of the roster fold (07000e9), which took your option (a) on the sibling finding.
(1) The "unchanged" clause. The enumeration is the 13 names that actually were unchanged; mortie.toc_normalize / mortie.toc_and moved out into their own clause:
... and
mortie.toc_normalize/mortie.toc_andare new in this release, flat from the start: they never had a submodule spelling, so they are not in the shim's roster andmortie.toc.toc_andwas never reachable.
That last clause is now a statement about shipped behaviour, not just history — after 07000e9 the shim raises AttributeError for those two.
(2) The missing announcement. New [Unreleased] bullet, Toc set algebra: toc_normalize and toc_and (issues #177 / #198), modelled on the tocs_reduce entry's granularity and placed after the Toc bullet to keep the section's descending-phase order. It carries: the canonical cover form (coalesce iff decoded envelopes overlap or abut exactly, a surviving gap never bridged because outward rounding only shrinks apparent gaps, subsumed instants absorbed, free instants bit-identical); the sweep emitting [max(starts), min(ends)) with a timestamp surviving iff genuinely covered on both sides; the conservative directions (normalize coverage-identical with no rounding arm, since merged bounds are min/max of on-grid values — intersection exact by grid closure, max of starts on the 2^31 ns grid and min of ends on 2^32, never under-covering); Q3's ruling that union is construction and difference/xor deliberately do not ship because conservative covers under-cover on subtraction; and the GIL release plus toc_merge's GIGO scope.
Wording checked against the two docstrings in mortie/_toc.py rather than restated from the issue, and the GIL claim against rust_toc_normalize / rust_toc_and — both wrap in py.allow_threads.
| "to_datetime64", | ||
| "to_gps_ns", | ||
| "toc2time", | ||
| "toc_and", |
There was a problem hiding this comment.
🤖 from Claude (review)
toc_and (and toc_normalize, line 90) are enrolled in the deprecated namespace, but mortie.toc.toc_and / mortie.toc.toc_normalize were never a reachable spelling in any released mortie: both functions are added by this PR's phases 1–2 (d22268a, ad18705), and this same PR retires the submodule. git show origin/main:mortie/toc.py contains neither.
That is a divergence from the ruled _MocNamespace pin, not just a wording difference. mortie/moc_object.py:61 describes its roster as "the 0.9.9 surface" — the released surface, which for issue #196 coincided with the rename-time surface because that PR added no kernel functions. Here the two diverge, and this roster takes the rename-time one. Effects:
dir(mortie.toc)advertises two names no consumer can possibly be using;- the shim message promises they are "removed in the next minor release" — a full deprecation cycle for an API that never shipped;
- the class docstring immediately below (line 511) says the shim resolves "the kernel functions and constants the submodule used to hold", which is untrue of these two, as is the roster comment's framing at line 67 ("The public surface of the former
mortie.tocsubmodule"). The test's own comment (mortie/tests/test_toc_object.py:514) is the only place that admits the difference, parenthetically.
Two concrete options:
(a) Drop "toc_and" and "toc_normalize" from _KERNEL_NAMES here and from _RETIRED_SUBMODULE_SURFACE (mortie/tests/test_toc_object.py:519-525). Nothing external can depend on them, the roster then means exactly what _MocNamespace's means, and the docstrings above become true as written. The parametrized resolve test and the frozen-surface pin carry over unchanged at 17 names.
(b) Keep all 19 and state in the roster comment why the rename-time surface is the right pin here where the released surface is not, so the deviation from the issue #196 precedent is deliberate and on the record rather than incidental.
There was a problem hiding this comment.
🤖 from Claude
Folded in 07000e9 as option (a) — the roster is the 0.9.9 released surface now, matching the ruled _MocNamespace pin exactly.
"toc_and" and "toc_normalize" are out of _KERNEL_NAMES (mortie/toc_object.py) and out of _RETIRED_SUBMODULE_SURFACE (mortie/tests/test_toc_object.py); the roster is 17 names — 13 module-level functions plus the four grid/epoch constants. Verified against the release rather than reasoned about: git show 7f747e0:mortie/toc.py (tag 0.9.9) defines exactly those 13 defs and 4 constants, and contains neither new name.
Both comments now say what the pin is. mortie/toc_object.py:
# ... A frozen historical roster (the 0.9.9 surface: thirteen
# module-level functions plus the four grid/epoch constants), not a live view
# ... ``toc_normalize`` and ``toc_and`` are deliberately absent: born
# in this same PR, they never had a ``mortie.toc.<name>`` spelling any release
# could be using, so there is nothing there to deprecate.
and the test's independent copy cites the rev and the reason in the same terms, replacing the parenthetical admission you flagged at line 514.
Consequences you listed all close: dir(mortie.toc) is the 17 released names, no deprecation cycle is spent on an API that never shipped, and the _TocNamespace docstring's "the kernel functions and constants the submodule used to hold" is true as written. mortie.toc.toc_and now raises AttributeError with the shim's "is the Toc constructor (issue #198), not the old submodule" message — the correct answer for a name no consumer can hold.
The parametrized resolve test, the frozen-surface pin, the dangle test and the dir() test all carry over unchanged and pass — mortie/tests/test_toc_object.py is 100 passed. The two names remain imported into toc_object.py for the class's own use (_words, overlaps/contains/intersection); only the deprecation roster shrank.
| " `toc2time`, `toc_merge`, `toc_reduce`, `toc_is_range`, `toc_overlaps`,\n", | ||
| " `toc_contains`, `from_datetime64`, `to_datetime64`, `from_gps_ns`,\n", | ||
| " `to_gps_ns`; the constants live on the module (`mortie.toc.Q_START_NS`,\n", | ||
| " `to_gps_ns`; the constants live flat on the package (`mortie.Q_START_NS`,\n", |
There was a problem hiding this comment.
🤖 from Claude (review)
Smaller, but this is the docs phase and it edits this exact bullet: the "API is flat on the package" enumeration is now incomplete. It lists time2toc, span2toc, toc2time, toc_merge, toc_reduce, toc_is_range, toc_overlaps, toc_contains, from_datetime64, to_datetime64, from_gps_ns, to_gps_ns — missing toc_normalize and toc_and, which this same PR adds as public flat names, and tocs_reduce, flat since PR #192.
It matters because docs/api/toc.md presents this notebook as the worked example that "walks the type end-to-end", and this bullet is the notebook's canonical enumeration of the surface — a reader who takes it as complete never learns the two set-algebra entries exist. docs/api/toc.md's own members list already carries all 19 names.
Fix: add toc_normalize, toc_and, tocs_reduce to the list, and consider one line pointing at mortie.Toc / docs/api/toc_object.md as the object layer — this notebook is now the one place a reader meets mortie.toc after it stopped being a module, and it says nothing about what the name means today.
For the record, the rest of the notebook edit checks out: I executed every code cell against this commit (matplotlib stubbed) and it runs clean end to end with all asserts passing and the printed values matching the stored outputs, and no toc.-submodule reference survives the edit anywhere in the file.
There was a problem hiding this comment.
🤖 from Claude
Folded in 8c51792 — markdown-cell-only, no re-execution.
The enumeration is complete: toc_normalize, toc_and and tocs_reduce added, so the bullet now lists all 15 functions plus the four constants, matching docs/api/toc.md's members list.
New second bullet, taking your suggestion:
mortie.tocis theTocconstructor now, not a module (#198): the kernel moved tomortie/_toc.py, andmortie.toc(...)builds the object layer over these words — see docs/api/toc_object.md.
One edit beyond what you asked, because completing the list created a contradiction three bullets down: the closing bullet said the interval-set algebra "is deferred to #177", which stops being true the moment the same cell advertises toc_normalize and toc_and. It now reads that the entries the call-site audit ruled in ship as those two, and that difference and xor deliberately do not because conservative covers under-cover on subtraction — keeping the "flat-array elementwise surface only" scope sentence intact. Flagging it explicitly since it was not in your finding.
Edited the nbformat JSON in place (source arrays, line-terminated); json.load parses and nbformat.validate passes, and the diff is confined to that one cell — 15 insertions, 8 deletions, no whitespace churn and no output or metadata movement.
1 participant
Closes #198. Refs #177 (rulings), #196 / PR #197 (the
Mocpattern sibling).A
Tocobject so temporal coverage composes like spatial coverage, kernel-firstper the approved #177 kernel plan
(all rulings espg-confirmed in the
Q2 decision record)
and the dispatch plan on #198.
The kernels (Rust,
src_rust/src/toc/set_ops.rs)toc_normalize— the canonical cover form: decode words to exact[start, end)ns intervals, coalesce ranges iff decoded envelopesoverlap or abut exactly (Q1: a surviving decoded gap is never bridged —
outward rounding shrinks apparent gaps, so a surviving gap is a floor on the
true gap), absorb timestamps subsumed by a range's decoded span, keep free
instants bit-identical as exact degenerate members (Q2), output sorted
maximal merges. The merge is exact: decoded starts sit on the 2^31 ns
grid, ends on 2^32, and min/max keep them there — no rounding arm exists.
toc_and— pairwise[max(starts), min(ends))over canonicalized wordsets: exact by grid closure (Q3 — 2^31 starts closed under max, 2^32 ends
closed under min), so conservatism is preserved by construction. Per Q3,
toc_minus/toc_xordo not ship (no audited call site).Both are total over arbitrary bit patterns (garbage in, garbage out — never a
panic), matching the posture of
toc.rs'smerge.Phases
toc_normalizekernel (d22268a). Rust kernel + cargo goldens(the toc coverage algebra: normalize / union / intersect / minus over toc-word sets (deferred until a consumer call site exists) #177 absorption example, both-grid abutment vs one-quantum gap,
order-independence, idempotence, exact coverage preservation), the
mortie.toc_normalizebatch entry with the conservative-direction tablein its docstring, exports, docs member, and
mortie/tests/test_toc_setops.py.toc_andkernel (ad18705). The stamp walk refactored into ashared
select_stampspartition (canonicalize keeps the outside half,intersect the inside; renamed and narrowed to one half in fold
ca2bc20); two-pointer range sweep emitting[max(starts), min(ends))pieces — exact by grid closure, outputcanonical without a re-normalize (argument in the
intersectdoc);mortie.toc_andwith the direction table and the Q3no-difference/xor note; 10 new cargo tests + 8 new Python tests
(grid-closure golden, abutment shares nothing, exact timestamp
survival at the exclusive end, raw-input equivalence, identity /
commutativity / empty laws, membership differential, canonical
output, junk determinism).
Tocobject (f3b209d).mortie/toc_object.py: theconstructor matrix (ISO strings / datetime64 instants and pairs via
time2toc/span2tocwith broadcasting, word arrays,__toc_words__()protocol objects — integers are always words, so abare number can never be misread as ns-since-1970), eager
toc_normalizewith the Q2 lossy-toward-coverage act stated in theconstructor docstring, read-only canonical words + full immutability
(slots, refused setattr, pickle/copy via
__reduce__),__eq__/hashon canonical form,
overlaps/contains/intersection/__and__eacha single delegation to
toc_and(the one audited set operation),span-count + outward-rounded extent
__repr__,__toc_words__interchange dunder. The PR Moc object: geometry-first coverage API (issue #196) #197 AST delegation pin is extended, not
paralleled: the shape-whitelist + denied-node machinery moved to
mortie/tests/delegation.py, shared by both test files, with Toc'sblessed shapes (
.size > 0,np.array_equal(<kernel>, _words(...)))opt-in per class and new synthetic-violation coverage.
mortie.toccallable-namespace shim (a993ec7).mortie/toc.py→mortie/_toc.py;tocis now theTocconstructorwith
_MocNamespace's exact ruled semantics (warn on every access,no dedup state — filters own policy; honest warning-semantics
docstring; frozen roster pinned to the 0.9.9 released surface — 13
functions + the 4 grid/epoch constants, which now also live flat on the
package).
TestMigrationShimmirrors the Moc pair including theevery-access + later-consumer falsifiable tests. CHANGELOG (BREAKING
rename with the statement-form import break called out, the
Tocfeature, and the new
toc_normalize/toc_andkernels), docs(
docs/api/toc_object.md, toc kernel page reframed with the renameadmonition, nav), numpydoc gate extended to keep
_toc.pylinted, andthe example notebook migrated to the flat constant spellings.
Sequencing: phases 1–2 were built from
mainbefore PR #197 merged; onthat merge
mainwas merged forward here (e7ce41e, clean — no conflicts)and phases 3–4 reuse #197's shim + AST-test machinery as planned.
How it was tested
pytest -v1771 passed / 16 skipped on the final head (102 intest_toc_object.py, 20 intest_toc_setops.py);ruff checkclean onall touched files;
flake8 mortie --select=E9,F63,F7,F82clean;numpydoc lintclean ontoc_object.pyand_toc.py(gate regexextended so the renamed private file stays linted); notebook JSON +
nbformat validated and all code cells re-executed clean against the new
surface.
cargo test --releasefull suite green (21set_opstests after phases1–2 + review folds);
cargo fmt --checkclean (phases 3–4 arePython-layer; the only Rust edits after phase 2 are five comment-line
pointer fixes from the phase-4 review).
phase 1: 6 findings, 6 folded (
147dc78..d2545be); phase 2: 6 findings,5 folded + 1 reply-only (
df9db84..ca2bc20); phase 3: 6 findings, 6folded (
7cebfb6..7ce585b) — protocol operands canonicalized in_words, repr end bound ceiled outward, N-D word sources rejected, thearray_equalblessing pinned structurally, window-kernel name collisiondisambiguated, every spelling of the empty cover accepted; phase 4: 4
findings, 4 folded (
bea27cb..8c51792) — stale Rust comment pointers,CHANGELOG accuracy for the new kernels, shim roster re-pinned to the
0.9.9 released surface, notebook notes completed.
(
8c51792) CI linked from the checks tab.Questions for review
(
mortie.Q_START_NS,Q_END_NS,TOC_MAX_NS,GPS_EPOCH_NS): they weredocumented public surface reachable only through the submodule spelling,
which the rename retires, so they joined the flat namespace rather than
losing their public path. Flagging since it is a (small) API addition.