Segmented toc reduce: tocs_reduce (issue #177 v1) - #192
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #192 +/- ##
==========================================
+ Coverage 96.34% 96.36% +0.02%
==========================================
Files 18 18
Lines 1996 2008 +12
==========================================
+ Hits 1923 1935 +12
Misses 73 73
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Merging this PR will improve performance by 12.3%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_coverage_triangle_order4 |
378 µs | 336.6 µs | +12.3% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/177-tocs-reduce (9f2b632) with main (581310f)
Footnotes
-
1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports. ↩
| offsets = np.arange(0, 65, 8, dtype=np.int64) | ||
| got = tocs_reduce(junk, offsets) | ||
| for i in range(8): | ||
| assert int(got[i]) == toc_reduce(junk[8 * i:8 * (i + 1)]), f"group {i}" |
There was a problem hiding this comment.
🤖 from Claude (review)
This assertion is unsound: merge is not associative on arbitrary bit patterns, so segmented (sequential) and toc_reduce (rayon) can disagree on junk words. The test passes only because seed 1772 happens not to produce a discriminating group.
Repro against the built extension on this branch, with an 8-element group — the same group size this test uses:
>>> import numpy as np, mortie
>>> g = np.array([0, 1, 1, 1, 1, 1, 1, 2**64 - 1], dtype=np.uint64)
>>> mortie.toc_reduce(g)
1
>>> int(mortie.tocs_reduce(g, np.array([0, 8], dtype=np.int64))[0])
2147483648(and at 200k words in one group the two also disagree, 1 vs 2147483648).
Mechanism: codes() (src_rust/src/toc.rs:131) maps a junk timestamp to e = (w >> 33) + 1, which for w = 2**64 - 1 is exactly 2**31. merge then emits (s << 32) | e — and e = 2**31 lands on FLAG_BIT, so the merged word reads back as a timestamp and re-decodes to e = 1. Fold order therefore changes the answer. A direct search over 9 junk words finds 240 non-associative ordered triples, including (0, 1, 2**64-1) — which is literally junk[0], junk[1], junk[2] in this test's group 0.
This is only a junk-word problem: I checked 3001 randomized valid triples spanning TOC_MAX_NS - 1 and both encoders, and the max end code is 2**31 - 1, so merge never overflows the flag and associativity holds — consistent with merge's own caveat at src_rust/src/toc.rs:148-151 and with merge_is_commutative_and_associative (src_rust/src/toc.rs:698) drawing only from rand_word, i.e. valid words.
Note the cargo twin gets this right: arbitrary_bit_patterns_fold_without_panicking (src_rust/src/toc.rs:888) compares against chunk.iter().copied().reduce(merge) — a sequential fold, same order, so no associativity is assumed. The fix here is the same shape: compare junk groups against a sequential Python fold of py_merge (already defined at mortie/tests/test_toc.py:203), and keep the toc_reduce parity assertion for the valid-word tests where it is actually pinned.
There was a problem hiding this comment.
🤖 from Claude
Fixed the test, not the kernel — option (b), the cargo twin's shape. 92a73ed.
test_tocs_reduce_arbitrary_bit_patterns_do_not_panic (mortie/tests/test_toc.py) now compares each junk group against reduce(py_merge, ...) — the sequential fold, the same oracle arbitrary_bit_patterns_fold_without_panicking uses at src_rust/src/toc.rs — instead of toc_reduce. segmented_reduce folds words[s..e].iter().copied().reduce(merge) within a group, so sequential is the order it actually performs; nothing about associativity is assumed. Verified the reshaped assertion holds on the seed-1772 junk (8 groups, all match).
Added test_tocs_reduce_junk_fold_is_tree_dependent alongside it, pinning your repro as documented behavior rather than leaving the disagreement unrecorded: [0, 1, 1, 1, 1, 1, 1, 2**64 - 1] folds sequentially to 2**31, the assertion checks that word carries FLAG (the mechanism — end code 2**31 landing on the flag bit), and both entry points are called repeatedly to pin determinism. It deliberately does not assert toc_reduce != sequential, since that would pin rayon's split for an 8-element slice on one machine.
Docs caveat is separate — 99d4024, replied on the other thread.
| batch family's plural convention (``mocs_and``, ``mocs_to_orders``): the | ||
| whole ragged group set crosses the Python/Rust boundary once, the GIL is | ||
| released for the batch, and Rust parallelizes across groups. Result ``i`` | ||
| is bit-identical to ``toc_reduce(words[offsets[i]:offsets[i + 1]])`` — same |
There was a problem hiding this comment.
🤖 from Claude (review)
The bit-identity guarantee is stated unconditionally, but it only holds for encoder-produced words — the same scope caveat merge already carries.
Counterexample on this branch:
>>> import numpy as np, mortie
>>> g = np.array([0, 1, 1, 1, 1, 1, 1, 2**64 - 1], dtype=np.uint64)
>>> mortie.toc_reduce(g), int(mortie.tocs_reduce(g, np.array([0, 8], np.int64))[0])
(1, 2147483648)merge's own doc comment already says it (src_rust/src/toc.rs:148-151): "an arbitrary bit pattern is garbage in, garbage out (an out-of-domain 'timestamp' past TOC_MAX_NS can even merge to a word with the timestamp flag set)" — and once the flag bit is set, codes() re-reads the end code as (w >> 33) + 1, so the fold tree stops being irrelevant. The associativity that this guarantee rests on is pinned only over rand_word, i.e. valid words (src_rust/src/toc.rs:698).
I verified the valid domain is safe: over 3001 randomized triples including TOC_MAX_NS - 1 timestamps and full-span ranges, the max end code is 2**31 - 1, so merge never overflows into the flag and every fold tree agrees. So this is a wording scope, not a kernel bug — but the same unqualified claim is repeated in three normative-ish places and each wants the caveat:
- here (
mortie/toc.py:302) src_rust/src/toc.rs:392-394— "the fold tree is irrelevant:mergeis exactly associative, commutative and idempotent"CHANGELOG.md— "Resultiis bit-identical totoc_reduceon groupialone"
There was a problem hiding this comment.
🤖 from Claude
Qualified all three sites, plus toc_reduce's own docstring — 99d4024.
mortie/toc.py(tocs_reduce): the bit-identity sentence now ends "That identity is a guarantee over encoder-produced words, the scopetoc_mergecarries: an out-of-domain "timestamp" can merge to a word with the timestamp flag set, and past that point the two functions' fold trees may disagree — each deterministic, neither wrong, since junk in is junk out."src_rust/src/toc.rs(segmented_reduce): same caveat, made concrete about which trees — "this fold is sequential within a group whilerust_toc_reducesplits, so the two may then differ, each deterministically."CHANGELOG.md: "same fold-tree independence — over encoder-produced words, the scopetoc_mergealready carries (an arbitrary bit pattern is garbage in, garbage out, and the two fold trees may then differ, each deterministically)."- Also
toc_reduce's docstring 40 lines up, which carried the identical unqualified claim ("every fold tree produces the identicaluint64"). Leaving one qualified and its neighbour not would have read as a distinction that isn't there — flagging it since it is one line past the three you named.
The PR body's two statements of the claim are qualified the same way. The counterexample itself is now a test rather than only prose (test_tocs_reduce_junk_fold_is_tree_dependent, 92a73ed). No kernel change; cargo check / cargo doc --no-deps clean (the only doc warning is the pre-existing CHUNK_BYTES one in src_rust/src/wkb/batch.rs).
| if arr.dtype.kind not in "iu": | ||
| raise ValueError( | ||
| f"offsets must be integer-typed, got dtype {arr.dtype}") | ||
| return np.ascontiguousarray(arr.astype(np.int64).ravel()) |
There was a problem hiding this comment.
🤖 from Claude (review)
_as_offsets exists to stop a silent numeric cast, but astype(np.int64) performs exactly that silent corruption on the uint64 half of the "iu" kinds it accepts.
uint64 offsets are the natural output of np.cumsum over unsigned counts, and any value at or above 2**63 wraps to a negative int64 here without a word:
>>> import numpy as np, mortie
>>> w = mortie.time2toc(np.array([1, 2, 3, 4], dtype=np.uint64))
>>> mortie.tocs_reduce(w, np.array([0, 2**63 + 5], dtype=np.uint64))
ValueError: group 0: offsets must be monotonically non-decreasing (-9223372036854775803 < 0)The offsets passed in are monotone; the message describes the wrapped copy, not the input. It does fail closed today only by accident: validate_ragged pins offsets[0] == 0, so every wrapped value is negative and trips the monotonicity check — but the message sends the reader looking for a bug in their offsets rather than at an out-of-range one.
The docstring's stated rationale is "a float offset array would otherwise cast silently, truncating 2.9 to a group boundary at 2 rather than saying so" — the same standard applied to uint64 would be a range check before the cast (e.g. reject arr.max() > np.iinfo(np.int64).max) or a message that names the real problem.
There was a problem hiding this comment.
🤖 from Claude
Fixed — 9f2b632. _as_offsets now range-checks before the cast, mirroring _as_u64's sign guard:
if arr.dtype.kind == "u" and arr.size:
too_big = arr > np.iinfo(np.int64).max
if too_big.any():
raise ValueError(
f"offsets must fit in int64, got {int(arr[too_big][0])}")Your repro now says what happened, naming the value passed in:
ValueError: offsets must fit in int64, got 9223372036854775813
The lowest-index offender is named, matching the convention the group errors use. Docstring rationale updated to state the rule (the cast cannot represent it, and the Rust validator would otherwise describe the wrapped copy); monotonicity and bounds stay Rust's job.
Regression test test_tocs_reduce_uint64_offsets_out_of_int64_range covers both halves — 2**63 + 5 refused by value, and in-range uint64 offsets still accepted, since this is a range check and not a rejection of the dtype np.cumsum hands you. pytest mortie/tests/test_toc.py 48 passed, mortie/toc.py still 100% covered; flake8 --select=E9,F63,F7,F82, flake8 --max-line-length=88 and numpydoc lint mortie/toc.py clean.
Noted in "Questions for review" (4) that batch.py's offsets handling still wraps silently here — not changed, since that is outside this PR's surface.
Refs #177 — the v1 scope only. The interval-set algebra (
normalize/ union / intersect / minus) stays deferred, so this does not close the issue: the three rulings the issue parks (canonical form, adjacency, the difference trap) are untouched and still want a consumer. See the plan comment for the split.What this adds
One function,
tocs_reduce(words, offsets) -> words— the segmented sibling oftoc_reduce, named in the batch family's plural convention (mocs_and,mocs_to_orders).Ragged in (arrow list layout: group
iiswords[offsets[i]:offsets[i+1]]), dense out — oneuint64per group, because the reduction is many→one per group so there are no output offsets to carry. Resultiis bit-identical totoc_reduceon groupialone — over encoder-produced words, the scopetoc_mergealready carries (junk in, junk out: an out-of-domain "timestamp" can merge onto the flag bit, past which the two fold trees may differ, each deterministically).Approach
rust_toc_reduceinsrc_rust/src/toc.rs:segmented_reduce()validates the layout serially, then folds groups in 2048-group chunks underpy.allow_threadswith rayon across groups — thecommon_ancestorsshape (src_rust/src/decimal_morton/batch.rs:247) withcommon_ancestorswapped fortoc::merge. Each chunk's outcomes are materialized and walked in index order before any is allowed to fail, so the reported index is the lowest-index offender under any rayon schedule.toc_reduce, per group: the same join, instant preservation (a group of bitwise-equal timestamps comes back as that timestamp, not as its range envelope), and fold-tree independence — safe becausemergeis exactly associative, commutative and idempotent over encoder-produced words, which the existing cargo law tests pin (they draw fromrand_word). The scope matters:merge's own doc comment notes an out-of-domain "timestamp" can merge to a word with the flag bit set, and once that happens the fold tree stops being irrelevant —segmented_reducefolds sequentially within a group whilerust_toc_reducesplits, so on junk the two may disagree. Documented at all three sites (mortie/toc.py,src_rust/src/toc.rs, CHANGELOG) and pinned by test rather than asserted away.toc_reduce's existing ruling (src_rust/src/toc.rs,"the merge has no identity element"), inherited rather than re-decided; zagg's fold sites never present an empty cell (they short-circuit before the fold).run_group, which turns a panic into aValueErrornaming the group. Stated plainly in the code: this is defensive, not load-bearing —mergeis total over arbitrary bit patterns (it only shifts and compares), so there is no malformed toc word the way there is a malformed morton word, and nothing a caller can pass panics today. It is here so a laterdebug_assertor arithmetic edge surfaces as a catchable named error rather than apyo3_runtime.PanicException, which derives fromBaseExceptionand escapes evenexcept Exception. Pinned by an injected panicking kernel, the waymoc/batch.rs'srun_mocis.mortie/toc.pywith the module's_as_u64validation, plus a matching_as_offsetsthat requires integer-typed offsets — a float offsets array would otherwise cast silently, truncating2.9to a boundary at 2 rather than saying so. The same standard rejects auint64offset at or above2**63(the natural output ofnp.cumsumover unsigned counts), which theint64cast would otherwise wrap negative — the error names the value passed in, not the wrapped copy. Exported frommortie/__init__.pybeside the other toc names;See Alsolinks both directions (toc_mergeandtoc_reduceeach gained the back-reference).Phases
0613d49)3c0521e)How it was tested
Cargo (
cargo test --lib, 382 passed), 7 new tests insrc_rust/src/toc.rs:segmented_reduce_matches_the_scalar_fold_per_groupsegmented_reduce_preserves_instants_and_singletonssegmented_reduce_is_permutation_invariant_within_a_groupsegmented_reduce_refuses_an_empty_group_by_indexsegmented_reduce_layout_errors_name_the_groupstart at 0,end at the word count, empty offsetssegmented_reduce_lowest_index_offender_winsCHUNK-1/CHUNK+3; the lowest is named, repeated 5× against a schedule-dependent answera_panicking_group_kernel_is_a_named_error_not_a_panic+arbitrary_bit_patterns_fold_without_panickingPython (
pytest mortie/tests, 1544 passed, 16 skipped;test_toc.pyalone is 48 passed,mortie/toc.pyat 100% coverage), 14 new tests:..._parity_with_a_scalar_loop— 2185 groups vs a loop oftoc_reduce, across the chunk seam...._property_against_a_python_reference— 400 randomized groups vs a pure-Python fold of a referencepy_mergewritten from the bit layout, so the parity is against an independent implementation rather than the same kernel...._permutation_invariant_within_a_group,..._preserves_instants_per_group,..._mixed_instant_and_range_groups(instant-only / range-only / mixed in one call, with the resultingtoc_is_rangeflags pinned)...._empty_segment_is_a_catchable_named_error— asserted throughexcept Exceptionand anisinstance(..., ValueError), not onlypytest.raises, because that is the handler shape a consumer writes and the exact shape aPanicExceptionwould slip past...._arbitrary_bit_patterns_do_not_panic— junk words including0,1,2**64-1and the bare flag bit; the answer must be the sequential in-group fold ofpy_merge, the same oracle the cargo twin uses (arbitrary_bit_patterns_fold_without_panicking). Parity withtoc_reduceis deliberately not asserted on junk — see the next test...._junk_fold_is_tree_dependent— the counterexample behind that scope:[0, 1, 1, 1, 1, 1, 1, 2**64-1]merges onto the flag bit, sotocs_reduce(sequential) answers2**31wheretoc_reduce(split) answers1. Both deterministic, neither a panic — which is the whole junk-domain promise...._offsets_guards— non-monotone, out-of-range, bad endpoints, empty offsets, float offsets, float words, negative words...._uint64_offsets_out_of_int64_range— auint64offset of2**63 + 5is refused by value rather than wrapping to a negative int64 and being reported as a monotonicity failure; in-rangeuint64offsets still work...._lowest_index_offender_across_the_chunk_seam,..._empty_batch_and_group_of_one,..._deterministic_across_runs(10 identical results over 5000 groups)...._consumer_shape_per_cell_fold— the plan's consumer smoke: 4096 shot words folded to ~250 cells equals the scalar loop, and then one pyramid level up (4 cells per parent, envelope of envelopes) equals folding the leaves directly. That second half is the ATL03 overview claim from the zagg#410 plan — associativity carried through the segmented form.Lints, all clean:
cargo fmt --check,cargo clippy --all-targets(no findings ontoc.rs; the handful of pre-existing warnings elsewhere are untouched),flake8 mortie --select=E9,F63,F7,F82,flake8 --max-line-length=88on the touched files,ruff check, andnumpydoc lint mortie/toc.py(thelint.ymlhard gate).Questions for review
(1)
mortie/toc.pyvsmortie/batch.py— the one placement call, and it cuts against a stated convention. The plan comment saysmortie/toc.py, and that is what this PR does. Butmortie/batch.py's own docstring says "Every function here is the batch twin of a scalar that lives elsewhere in the package", consolidated by arity under issue #170 — andmortie/toc.py's docstring promised, before this PR, that "the ragged many-cover plurals land inmortie.batchwhen the interval-set algebra (issue #177) activates".tocs_reduceis a ragged batch twin of a scalar, so on a literal read of #170 it belongs inbatch.py.The reason I kept it here anyway, and reworded both that line and
docs/api/toc.mdto match:tocs_reducefolds the word type itself, where every currentbatch.pyresident (mocs_and,mocs_to_orders,common_ancestors,children_of,from_wkbs,polygons_to_morton_mocs) operates over covers or geometry. The deferred many-cover plurals — a segmentedtocs_overlaps, and whatever the algebra eventually needs — are the ones #170's promise was about, and they still point atbatch.py. Moving it is a one-line import change if you disagree; say the word and I will.(2) Naming:
tocs_reducevstoc_reduce_segmented. Flagged in the plan and not agonized over — the plural matchesmocs_*. It does read slightly oddly next totoc_reduce(one letter apart, and thesis the only signal), which is the one argument for the suffix form. Happy to rename; it is mechanical.(3) The panic capture on a kernel that cannot panic.
run_group+catch_unwindis ~15 lines guarding a fold that only shifts and compares. CLAUDE.md §4 says no speculative abstraction; the #161/#185 posture says never let aPanicExceptionreach Python. I sided with the posture and documented the tension in the doc comment rather than silently picking one. If you would rather this be a plain fold with no capture, it is a small deletion.(4)
_as_offsetsis stricter than the batch family.mocs_to_ordersand friends donp.asarray(offsets, dtype=np.int64), which silently truncates a float offsets array. The new helper refuses one instead, matchingtoc.py's own_as_u64, which refuses float words. That is a deliberate local divergence frombatch.py— the toc module is the stricter one throughout — but it does meantocs_reduceandmocs_to_ordersanswer differently to the same bad input. Not proposing to changebatch.pyhere; flagging it so the inconsistency is a choice on the record. The helper now also range-checksuint64offsets before theint64cast, for the same reason —batch.pystill wraps silently there.(5) Not shipped, deliberately. No benchmark (the plan's honest expectation is the bandwidth-bound
mocs_to_ordersband, not #154's 19.9× — a toc word decodes in two shifts, so there is nothing to hoist out of the loop), and no notebook change.docs/specification.mdcovers no toc surface at all today, so onlydocs/api/toc.mdneeded the member entry.