Skip to content

Segmented toc reduce: tocs_reduce (issue #177 v1) - #192

Merged
espg merged 6 commits into
mainfrom
claude/177-tocs-reduce
Aug 17, 2026
Merged

Segmented toc reduce: tocs_reduce (issue #177 v1)#192
espg merged 6 commits into
mainfrom
claude/177-tocs-reduce

Conversation

@espg

@espg espg commented Aug 17, 2026

Copy link
Copy Markdown
Owner

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 of toc_reduce, named in the batch family's plural convention (mocs_and, mocs_to_orders).

>>> cells = mortie.tocs_reduce(shot_words, cell_offsets)   # one word per cell

Ragged in (arrow list layout: group i is words[offsets[i]:offsets[i+1]]), dense out — one uint64 per group, because the reduction is many→one per group so there are no output offsets to carry. Result i is bit-identical to toc_reduce on group i alone — over encoder-produced words, the scope toc_merge already 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 kernel beside rust_toc_reduce in src_rust/src/toc.rs: segmented_reduce() validates the layout serially, then folds groups in 2048-group chunks under py.allow_threads with rayon across groups — the common_ancestors shape (src_rust/src/decimal_morton/batch.rs:247) with common_ancestor swapped for toc::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.
  • Semantics inherited wholesale from 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 because merge is exactly associative, commutative and idempotent over encoder-produced words, which the existing cargo law tests pin (they draw from rand_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_reduce folds sequentially within a group while rust_toc_reduce splits, 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.
  • An empty segment refuses, catchably, naming the group: the merge has no identity element, so many→one over no words has no answer. That is 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).
  • Panic-capture posture (moc.rs: to_order_count/to_order shift wraps mod 64, fabricating the budget estimate and panicking past except Exception #161 / small fixes: moc.rs densify shift wrap (#161) and the batch memory posture's missing input copy (#162) #185): every group's fold runs under run_group, which turns a panic into a ValueError naming the group. Stated plainly in the code: this is defensive, not load-bearingmerge is 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 later debug_assert or arithmetic edge surfaces as a catchable named error rather than a pyo3_runtime.PanicException, which derives from BaseException and escapes even except Exception. Pinned by an injected panicking kernel, the way moc/batch.rs's run_moc is.
  • Python skin in mortie/toc.py with the module's _as_u64 validation, plus a matching _as_offsets that requires integer-typed offsets — a float offsets array would otherwise cast silently, truncating 2.9 to a boundary at 2 rather than saying so. The same standard rejects a uint64 offset at or above 2**63 (the natural output of np.cumsum over unsigned counts), which the int64 cast would otherwise wrap negative — the error names the value passed in, not the wrapped copy. Exported from mortie/__init__.py beside the other toc names; See Also links both directions (toc_merge and toc_reduce each gained the back-reference).

Phases

  • Phase 1 — Rust kernel, binding, cargo tests (0613d49)
  • Phase 2 — Python skin, exports, pytest suite (3c0521e)
  • Phase 3 — docs page + CHANGELOG

How it was tested

Cargo (cargo test --lib, 382 passed), 7 new tests in src_rust/src/toc.rs:

test what it pins
segmented_reduce_matches_the_scalar_fold_per_group parity over 2085 randomized groups — past the 2048 chunk seam, so the parallel path is real
segmented_reduce_preserves_instants_and_singletons a group of equal timestamps stays a timestamp; a mixed group merges to a range
segmented_reduce_is_permutation_invariant_within_a_group commutativity/associativity through the segmented form
segmented_reduce_refuses_an_empty_group_by_index the no-identity refusal, named; an empty batch is still fine
segmented_reduce_layout_errors_name_the_group monotonicity, bounds, start at 0, end at the word count, empty offsets
segmented_reduce_lowest_index_offender_wins offenders at 7 / CHUNK-1 / CHUNK+3; the lowest is named, repeated 5× against a schedule-dependent answer
a_panicking_group_kernel_is_a_named_error_not_a_panic + arbitrary_bit_patterns_fold_without_panicking the capture mechanism, and junk-word totality

Python (pytest mortie/tests, 1544 passed, 16 skipped; test_toc.py alone is 48 passed, mortie/toc.py at 100% coverage), 14 new tests:

  • ..._parity_with_a_scalar_loop — 2185 groups vs a loop of toc_reduce, across the chunk seam.
  • ..._property_against_a_python_reference — 400 randomized groups vs a pure-Python fold of a reference py_merge written 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 resulting toc_is_range flags pinned).
  • ..._empty_segment_is_a_catchable_named_error — asserted through except Exception and an isinstance(..., ValueError), not only pytest.raises, because that is the handler shape a consumer writes and the exact shape a PanicException would slip past.
  • ..._arbitrary_bit_patterns_do_not_panic — junk words including 0, 1, 2**64-1 and the bare flag bit; the answer must be the sequential in-group fold of py_merge, the same oracle the cargo twin uses (arbitrary_bit_patterns_fold_without_panicking). Parity with toc_reduce is 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, so tocs_reduce (sequential) answers 2**31 where toc_reduce (split) answers 1. 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 — a uint64 offset of 2**63 + 5 is refused by value rather than wrapping to a negative int64 and being reported as a monotonicity failure; in-range uint64 offsets 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_foldthe 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 on toc.rs; the handful of pre-existing warnings elsewhere are untouched), flake8 mortie --select=E9,F63,F7,F82, flake8 --max-line-length=88 on the touched files, ruff check, and numpydoc lint mortie/toc.py (the lint.yml hard gate).

Questions for review

(1) mortie/toc.py vs mortie/batch.py — the one placement call, and it cuts against a stated convention. The plan comment says mortie/toc.py, and that is what this PR does. But mortie/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 — and mortie/toc.py's docstring promised, before this PR, that "the ragged many-cover plurals land in mortie.batch when the interval-set algebra (issue #177) activates". tocs_reduce is a ragged batch twin of a scalar, so on a literal read of #170 it belongs in batch.py.

The reason I kept it here anyway, and reworded both that line and docs/api/toc.md to match: tocs_reduce folds the word type itself, where every current batch.py resident (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 segmented tocs_overlaps, and whatever the algebra eventually needs — are the ones #170's promise was about, and they still point at batch.py. Moving it is a one-line import change if you disagree; say the word and I will.

(2) Naming: tocs_reduce vs toc_reduce_segmented. Flagged in the plan and not agonized over — the plural matches mocs_*. It does read slightly oddly next to toc_reduce (one letter apart, and the s is 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_unwind is ~15 lines guarding a fold that only shifts and compares. CLAUDE.md §4 says no speculative abstraction; the #161/#185 posture says never let a PanicException reach 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_offsets is stricter than the batch family. mocs_to_orders and friends do np.asarray(offsets, dtype=np.int64), which silently truncates a float offsets array. The new helper refuses one instead, matching toc.py's own _as_u64, which refuses float words. That is a deliberate local divergence from batch.py — the toc module is the stricter one throughout — but it does mean tocs_reduce and mocs_to_orders answer differently to the same bad input. Not proposing to change batch.py here; flagging it so the inconsistency is a choice on the record. The helper now also range-checks uint64 offsets before the int64 cast, for the same reason — batch.py still wraps silently there.

(5) Not shipped, deliberately. No benchmark (the plan's honest expectation is the bandwidth-bound mocs_to_orders band, 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.md covers no toc surface at all today, so only docs/api/toc.md needed the member entry.

@espg espg added the implement label Aug 17, 2026
@codecov

codecov Bot commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.36%. Comparing base (b7e35d6) to head (9f2b632).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            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              
Flag Coverage Δ
unittests 96.36% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
mortie/__init__.py 90.62% <ø> (ø)
mortie/toc.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7e35d6...9f2b632. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codspeed-hq

codspeed-hq Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will improve performance by 12.3%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
✅ 70 untouched benchmarks
⏩ 1 skipped benchmark1

Performance Changes

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)

Open in CodSpeed

Footnotes

  1. 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.

@espg
espg marked this pull request as ready for review August 17, 2026 01:19
Comment thread mortie/tests/test_toc.py Outdated
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}"

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Comment thread mortie/toc.py
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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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: merge is exactly associative, commutative and idempotent"
  • CHANGELOG.md"Result i is bit-identical to toc_reduce on group i alone"

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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 scope toc_merge carries: 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 while rust_toc_reduce splits, so the two may then differ, each deterministically."
  • CHANGELOG.md: "same fold-tree independence — over encoder-produced words, the scope toc_merge already 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 identical uint64"). 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).

Comment thread mortie/toc.py
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())

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant