small fixes: moc.rs densify shift wrap (#161) and the batch memory posture's missing input copy (#162)

[espg]

Closes #161
Closes #162

Two bundled small-fix issues (CLAUDE.md §5): one real correctness bug in the MOC densify kernel, one docs-accuracy fix on the batch memory posture.

Phases

• Phase 1 — moc.rs: to_order_count/to_order shift wraps mod 64, fabricating the budget estimate and panicking past except Exception #161 (79b20bc): to_order / to_order_count reject an out-of-range order in Rust instead of wrapping the shift mod 64; the bindings map it to ValueError.
• Fold of phase-1 review (52b1f59): per-MOC error prefix, run_moc and estimate doc accuracy.
• Phase 2 — Docs: the batch memory posture omits the mandatory input copy (coverage.py + coverage/batch.rs) #162 (9a35271): correct the batch memory posture to include the mandatory input copy, in mortie/batch.py and src_rust/src/coverage/batch.rs.
• Fold of phase-2 review (0322b2f): stale CHUNK peak claim, the conversion-copy floor, the run_moc serial-path gap.

Phase 1 — #161, the wrapping shift

src_rust/src/moc.rs computed the densify fan-out as

let shift = 2 * (order - depth) as u32;
let count = 1u64 << shift;

A shift of 64 or more is undefined for u64; a release build wraps it mod 64 rather than trapping. For a depth-6 input word that means order = 38 shifts by 0 (estimate 1 cell — under any budget, so moc_to_order's pre-emptive max_cells guard waved it through to a nested2mort panic) and order = 255 shifts by 50 (a fabricated 1125899906842624). The panic surfaced as pyo3_runtime.PanicException, whose MRO is (PanicException, BaseException, object) — caught by neither except ValueError nor except Exception.

The approach is the one the issue suggests: bound order in the kernel and return a Result the binding maps to ValueError, so the kernel is correct for any caller rather than only for those behind the Python fence PR #160 added.

The kernel signatures are now

fn check_order(order: u8) -> Result<(), String>;              // new, private
pub fn to_order(morton: &[u64], order: u8) -> Result<Vec<u64>, String>;
pub fn to_order_count(morton: &[u64], order: u8) -> Result<u64, String>;

• check_order refuses order above MAX_DEPTH (29 — already the crate constant tied to decimal_morton::MAX_ORDER), with the message Order must be between 0 and 29, got {order} — the same text mortie/moc.py's wrapper guard already raises.
• Returning a string error matches how neighbouring kernels report: moc::batch::validate_batch and dissolve::dissolve are both fallible the same way, and the bindings do PyValueError::new_err.
• rust_moc_to_order / rust_moc_to_order_count map the error to PyValueError, so the refusal is catchable by except Exception.
• Callers threaded through: moc::batch::validate_batch and mocs_to_orders use ? under the moc {i}: prefix those functions document. The infallible-by-construction sites keep the contract they already document — coverage::polygon_to_morton_coverage / multipolygon_to_morton_coverage assert order 1–29 before descending and are documented # Panics ... order ∉ 1–29, and dissolve's max_depth is decoded from the input words — so both .expect(...) with the invariant named.
• mortie/moc.py's wrapper guard is unchanged in behaviour; its docstring now says the kernel refuses too, so the wrapper is defence in depth rather than the only defence.

Tests

Rust (src_rust/src/moc.rs):

• test_out_of_range_order_is_an_error_not_a_wrapped_shift — both entry points return an error at orders 30/38/44/48/70/255 on a depth-6 word, the same band the Python test covers, including the two the issue measured (38 panicked, 255 fabricated the estimate).
• test_max_depth_order_still_densifies — the boundary is inclusive: order 29 on a depth-28 word still gives 4 cells and an exact estimate of 4.

Python (mortie/tests/test_coverage.py, in TestMocToOrderGuard):

• test_kernel_refuses_out_of_range_order_behind_the_wrapper calls _rustie.rust_moc_to_order / rust_moc_to_order_count directly, past mortie/moc.py's fence, and asserts each raises at the same orders under a bare except Exception — which a PanicException would escape — then checks isinstance(exc, ValueError) and the message.
• test_kernel_estimate_is_exact_at_the_boundary_order pins order 29 as in range through the bindings.

The pre-existing test_out_of_range_order_is_a_catchable_valueerror (PR #160's wrapper test) is unchanged and still passes.

Phase 2 — #162, the omitted input copy

Docstring-only, as the issue specifies. The claim corrected is "peak ≈ result + one chunk" in src_rust/src/coverage/batch.rs's module header and its Python twin.

Verified against the code first: rust_polygons_coverage_mocs (src_rust/src/lib.rs:973-975) does

let la = lats.to_vec()?;
let lo = lons.to_vec()?;
let off = offsets.to_vec()?;
let result = py.allow_threads(|| coverage::batch::polygons_to_morton_mocs(&la, &lo, &off, ...));

so all three inputs are copied whole before the GIL is released, and stay resident for the call. The kernel takes borrowed f64 slices, so the copy is the Ungil bound, not an oversight — it cannot go without giving up the GIL release.

Then measured, rather than reworded. Instrument is the sampled-residency one mortie/tests/test_wkb_batch_memory.py documents as the only valid one on Linux (ru_maxrss survives execve, so a watermark difference cancels): baseline is resident RSS after a gc.collect(), peak is a 1 ms /proc/self/statm poller over the call. Cold call, synthetic ~1° footprints, order 8:

n	input	result	peak growth	/ result	/ (input+result)
100,000	6.9 MiB	12.9 MiB	21.9 MiB	1.70x	1.11x
555,867	38.2 MiB	71.6 MiB	112.0 MiB	1.56x	1.02x

So the old wording understated the peak by 56–70%, and input + result + one chunk brackets it to within a chunk — the same model the MOC batch's header already carries after PR #160. Both docs now state the copy, say why it is unavoidable, give these numbers, and say to size a worker off input + result.

Two things the phase-2 review caught and this PR also fixes:

• src_rust/src/coverage/batch.rs's CHUNK doc still said "the peak still ~1.1x the result" 20 lines below the corrected header — the same pre-Docs: the batch memory posture omits the mandatory input copy (coverage.py + coverage/batch.rs) #162 claim, now re-stated against input + result (which is what that 1.1x actually was).
• mortie/batch.py's binding copy is a floor, not the whole cost: the wrapper's own np.ascontiguousarray(np.asarray(..., dtype=np.float64)) copies again for a list, float32, or a non-contiguous slice. The docstring now says so and tells callers to pass contiguous float64/int64 to pay it once.

How it was tested

Local, in a venv inside the worktree, at 0322b2f:

command	result
maturin develop --release	ok
cargo test	360 passed, 0 failed, 1 ignored
cargo fmt / cargo clippy --release	clean; the --all-targets warnings are pre-existing in test modules and untouched here
flake8 mortie --select=E9,F63,F7,F82	clean
flake8 mortie --max-line-length=88 (non-blocking style pass)	62 findings before the change, 62 after — none new
ruff check mortie	12 pre-existing findings, none in a file this PR touches
numpydoc lint mortie/*.py	clean
pytest -v	1436 passed, 16 skipped

Questions for review

1. A pre-existing uncatchable panic of exactly the moc.rs: to_order_count/to_order shift wraps mod 64, fabricating the budget estimate and panicking past except Exception #161 class, left unfixed. moc::batch::validate_batch's estimate decodes every word on the serial path, outside run_moc's catch_unwind. Because mortie.batch.mocs_to_orders defaults max_cells=_FLAT_COVER_WARN_THRESHOLD, that is the default path, so a malformed word splits by keyword:

   v   = np.array([0, 1152921504606846982], dtype=np.uint64)   # word 0 is malformed
   off = np.array([0, 2], dtype=np.int64)
   
   mortie.mocs_to_orders(v, off, 8)
   # -> pyo3_runtime.PanicException: Morton index cannot be zero   (escapes `except Exception`)
   
   mortie.mocs_to_orders(v, off, 8, max_cells=None)
   # -> ValueError: moc 0: Morton index cannot be zero            (catchable, MOC-named)

   Both verified on this branch. This is the same uncatchable-refusal failure Consolidated follow-ups from the June/July sweeps (#73, #88, #97) #108 / moc.rs: to_order_count/to_order shift wraps mod 64, fabricating the budget estimate and panicking past except Exception #161 exist to close, inverted by a keyword default — but it is a different input (a malformed word, not an out-of-range order) and neither issue asks for it, so I did not widen scope. The fix looks like one line — run the estimate under run_moc too — plus a test. Want it (a) folded into this PR, (b) as its own small-fix issue, or (c) left alone? I have flagged the gap in run_moc's doc comment in the meantime rather than letting the doc claim coverage it does not have.

2. .expect at the two already-asserted call sites. coverage::polygon_to_morton_coverage and multipolygon_to_morton_coverage return a plain vector and document # Panics ... order ∉ 1–29; they assert that range before descending, so to_order there cannot fail. I kept them infallible with .expect(...) naming the invariant rather than making them fallible, which would ripple into every binding that calls them. Say if you would rather they propagate.

3. The moc {i}: prefixes on the new map_err arms are belt-and-braces. validate_batch rejects an out-of-range order before either site runs, and that is to_order's only error, so neither arm is reachable today. I kept them so the two sites read the same and so a future caller reordering the checks still gets the documented per-MOC naming — but they are strictly unreachable, and I will drop them if you would rather not carry unreachable arms.

4. src_rust/src/moc.rs is now 1140 lines (1078 before this change, +62 net), past the ~1000-line line in CLAUDE.md §4. I did not split it, since §4 asks for prior discussion first. Moving the inline mod tests out to src_rust/src/moc/tests.rs would follow the shape coverage/tests.rs and dissolve/tests.rs already use and would take the module to ~340 lines; say the word and I will open a separate issue.

5. Docs: the batch memory posture omits the mandatory input copy (coverage.py + coverage/batch.rs) #162's Python location. The issue cites mortie/coverage.py:391, but that file carries no memory-posture claim (no peak/chunk text anywhere in it). The polygons_to_morton_mocs docstring the issue's scope paragraph describes is at mortie/batch.py:55, which did say "peak memory is about the returned values array plus one chunk" — that is what I corrected. Flagging in case you meant a third spot.

6. No committed harness for the phase-2 numbers. The table above is reproducible but the script is not in the tree, unlike benchmarks/measure_mocs_and.py --mem which mocs_and's docstring cites. I drafted a --mem mode for benchmarks/measure_batch_coverage.py and then reverted it, because Docs: the batch memory posture omits the mandatory input copy (coverage.py + coverage/batch.rs) #162 says "Fix is docstring-only in both places ... No code change". Happy to land it if you want the numbers reproducible from the tree.

[espg]

🤖 from Claude

Phase 1 (issue #161) is pushed. Full local suite finished green after the PR body was written: 1436 passed, 16 skipped in 250s (pytest -v, 96% coverage), alongside cargo test 360 passed / 0 failed / 1 ignored. cargo fmt, cargo clippy --release, flake8 mortie --select=E9,F63,F7,F82 and numpydoc lint mortie/*.py are all clean; the non-blocking --max-line-length=88 style pass reports the same 62 findings before and after the change, and ruff check mortie reports 12 pre-existing findings, none in a file this PR touches.

Phase 2 (issue #162) is next. Four items are standing for you under "Questions for review" in the body — the .expect calls at the two already-asserted coverage call sites, the message wording against moc::batch::validate_batch's shorter one, src_rust/src/moc.rs now sitting at 1090 lines (past CLAUDE.md §4's ~1000, and already 1078 before this change), and the fact that issue #162's cited mortie/coverage.py:391 carries no memory-posture claim — the polygons_to_morton_mocs wording it describes is at mortie/batch.py:55, which is where I plan to correct it.

---

Generated by Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.34%. Comparing base (6bb1939) to head (8060dad).
⚠️ Report is 39 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #185      +/-   ##
==========================================
+ Coverage   96.26%   96.34%   +0.07%     
==========================================
  Files          18       18              
  Lines        1954     1996      +42     
==========================================
+ Hits         1881     1923      +42     
  Misses         73       73              

Flag	Coverage Δ
unittests	96.34% <ø> (+0.07%)	⬆️

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

Files with missing lines	Coverage Δ
mortie/batch.py	100.00% <ø> (ø)
mortie/moc.py	100.00% <ø> (ø)

... and 9 files 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 42822a0...8060dad. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[espg]

🤖 from Claude (review)

Reviewed the phase-1 diff (10 files, to_order / to_order_count → Result). The fix itself is correct. I verified the bound and every call site:

• check_order's order > MAX_DEPTH is the right bound — decimal_morton::order_of is total and caps at 29, so depth <= 29 by decode, and with order <= 29 the densify shift maxes at 58 and the coarsen shift 2 * (depth - order) at 58 too. No shift can reach 64 on either arm.
• The two .expect(...) sites in coverage.rs are sound: polygon_descend (line 247) and validate_multi (line 191) both assert!((1..=29).contains(&order)) before the call. The two in dissolve.rs are sound for the stated reason — max_depth comes from mort2nested(w).1, which cannot exceed 29.
• No unconverted caller left: grep over src_rust/src finds every to_order / to_order_count site updated, and cargo check --all-targets is clean (benches included).
• The arithmetic in the new doc blocks checks out: at depth 6, order 38 → shift 2 * 32 = 64 ≡ 0, order 48 → 84 ≡ 20 (1048576, exactly 1 << 20, so not > the default budget — the "38–48 passes through" claim is right, including the boundary), order 255 → (255 - 6) as u32 = 249, 498 ≡ 50, 1 << 50 = 1125899906842624. Matches the PR body.

Local run on this branch: cargo check --all-targets clean, cargo test --release 360 passed / 0 failed / 1 ignored, cargo fmt --check clean. I also confirmed the new behaviour end-to-end against the built extension — rust_moc_to_order_count(depth6, 38) now raises ValueError: Order must be between 0 and 29, got 38, and order 29 on a depth-28 word still gives 4.

Three findings below, all in the same family: this PR closes the PanicException hole for the order argument, but the identical hole for a malformed morton word is still open — and on the batch path it is the default configuration. All three are pre-existing behaviour, so per CLAUDE.md §4 I am flagging rather than fixing; what is new in this diff is doc text that now claims the hole is closed.

Answers to the PR body's "Questions for review", from a reviewer's seat:

1. .expect at the two coverage sites — agree with keeping them infallible. Both functions already document # Panics ... order ∉ 1–29 and assert it, so widening to Result would ripple through the bindings for an unreachable arm.
2. Message wording — the divergence between the kernel's ...got {order} and validate_batch's prefix-only text is fine as a prefix relationship, but see the line-125 comment: the ? you added there means the kernel's longer message would surface unprefixed from a function documented to name the offending MOC. Unreachable today; worth aligning anyway.
3. moc.rs at 1090 lines — the split you describe (inline mod tests → src_rust/src/moc/tests.rs, matching coverage/tests.rs and dissolve/tests.rs) is the obviously right shape. Separate issue, agreed.

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

The PanicException leak this PR closes for order is still wide open, on this exact line, for a malformed word — and it is the default path.

to_order_count reaches mort2nested, which panic!("Morton index cannot be zero")s on the empty word. That call sits in validate_batch, which is not inside any catch_unwind (mocs_to_orders does let n_mocs = validate_batch(...)? at line 250, and rust_mocs_to_orders in lib.rs just allow_threadses the whole thing). mortie.batch.mocs_to_orders defaults max_cells=_FLAT_COVER_WARN_THRESHOLD, so if let Some(budget) is taken on every ordinary call and the panic escapes to Python unconverted.

Verified against the extension built from this branch:

vals = np.array([mortie.norm2mort(0, 0, 6), 0], dtype=np.uint64)
off  = np.array([0, 1, 2], dtype=np.int64)

mortie.mocs_to_orders(vals, off, 8)
# -> pyo3_runtime.PanicException: Morton index cannot be zero
#    MRO: (PanicException, BaseException, object)

mortie.mocs_to_orders(vals, off, 8, max_cells=None)
# -> ValueError: moc 1: Morton index cannot be zero

So the same input is a catchable, MOC-named ValueError with the budget off and an uncatchable BaseException with the budget on — the exact failure mode issue #108 / #161 are about, inverted by a keyword default.

Pre-existing (the ? here only changed how the order error propagates), so per CLAUDE.md §4 I am flagging rather than fixing — but it belongs in the issue #161 conversation, since the PR body's claim that the kernel "is correct for any caller" holds only for the order argument, not for the words.

Secondary point on this line: the ? returns to_order_count's bare string, dropping the moc {i}: prefix that this function's own doc block promises ("Per-MOC errors name the offending MOC"). Unreachable today because the 0..=29 pre-check at line 103 fires first, but if it ever becomes reachable it violates the documented contract — while line 261 adds that prefix in a path that is unreachable. Worth making the two consistent.

---

Generated by Claude Code

[espg]

🤖 from Claude

Fixed in 8060dad — espg ruled option (a) in session, so this is folded into this PR rather than left for issue #161's thread.

The estimate now goes through run_moc, so it is inside the same catch_unwind the parallel pass uses:

let estimated = run_moc(i, || to_order_count(&values[s as usize..e as usize], order))?
    .map_err(|msg| format!("moc {i}: {msg}"))?;

Verified on the rebuilt extension (maturin develop --release), the exact case you posted:

{}                      ValueError  moc 0: Morton index cannot be zero
{"max_cells": None}     ValueError  moc 0: Morton index cannot be zero

Before the commit the first line was pyo3_runtime.PanicException (MRO PanicException, BaseException, object), the second already the ValueError — reproduced locally before touching the code, so the inversion is pinned as real rather than taken on trust.

On the secondary point: the moc {i}: prefix is preserved on both arms — the ? on run_moc carries the panic-derived message already prefixed, and the map_err adds it to to_order_count's bare string. So the two spellings at lines 127 and (now) 273 stay consistent with the doc block's promise. The unreachable-arm / |e| shadowing points from your other comment are a separate thread and left standing there.

Tests: mortie/tests/test_moc_batch.py::test_malformed_word_is_a_named_value_error_at_both_budgets (default budget, explicit budget, max_cells=None, plus an except Exception handler and a bad word at index 1 for the lowest-index rule) and the Rust unit test moc::batch::tests::malformed_word_in_the_budget_estimate_is_named.

[espg]

🤖 from Claude (review)

This rewrite makes the doc less accurate, not more. It now asserts a clean split — densify "defensive", set ops "live" — but both halves are wrong for the densify:

1. to_order still panics. Removing the out-of-range-order panic did not make it panic-free: to_order calls mort2nested on every word (moc.rs:190), which panics on the empty word 0 exactly as the set-op kernels do. The densify's run_moc is a live path for the same malformed-word reason the next sentence gives for the set ops.

2. On the default path the panic never reaches run_moc at all. With max_cells set — the default in mortie/batch.py — validate_batch's to_order_count hits the same mort2nested first, outside any catch_unwind, and escapes as PanicException (see my comment on line 125).

The last sentence compounds it: "a malformed-word test drives the real rayon path across a chunk seam" — malformed_word_names_lowest_index_across_chunks (line 665) only exercises mocs_and / mocs_intersect. There is no malformed-word test for mocs_to_orders, which is precisely the entry point where the behaviour differs by max_cells.

Suggest either restoring the narrower framing (this is defensive for the order argument only) or, better, keeping the sentence honest that malformed words make the densify live too — and adding a mocs_to_orders malformed-word test at both max_cells settings, which would have caught (2).

---

Generated by Claude Code

[espg]

🤖 from Claude

Fixed in 8060dad, and the doc now states the true coverage rather than the gap.

On (1) and (2): rather than narrow the doc to the order argument, the underlying defect is gone — validate_batch's estimate is routed through run_moc, so a malformed word is a named ValueError on the default max_cells path too. The scalar bindings got the same treatment (rust_moc_to_order / rust_moc_to_order_count in src_rust/src/lib.rs now catch_unwind the way rust_mort2nested does). So the "capture is live for every kernel, densify included" framing is now accurate for the densify, and the paragraph that flagged the serial pass as uncovered is replaced by one saying it is covered.

On the test point: there is now a malformed-word test for mocs_to_orders at both max_cells settings — mortie/tests/test_moc_batch.py::test_malformed_word_is_a_named_value_error_at_both_budgets — plus the Rust unit test moc::batch::tests::malformed_word_in_the_budget_estimate_is_named, which asserts the budget-on and budget-off spellings produce the same string. The run_moc doc's "tested from both sides" sentence now cites that test alongside malformed_word_names_lowest_index_across_chunks, so it no longer leans on a set-op-only test as evidence for the densify.

[espg]

🤖 from Claude (review)

Same gap on the scalar path as in the batch (see moc/batch.rs:125): the added doc two lines up says "the guard's estimate can never be a fabricated one", but the estimate call can still take down the interpreter's exception contract for a different input class. to_order_count → mort2nested panics on the empty word, and nothing here converts it:

mortie.moc_to_order(np.array([0], dtype=np.uint64), 5)
# -> pyo3_runtime.PanicException: Morton index cannot be zero

(Verified against the extension built from this branch.) PanicException derives from BaseException, so this is uncatchable by the except ValueError / except Exception the docstring in mortie/moc.py promises will work — the same failure this PR fixes for order.

Pre-existing, so flagging rather than fixing per CLAUDE.md §4. But since both bindings' new doc blocks now make an unqualified "raises ValueError" claim, either qualify them to the order argument or wrap these two calls in catch_unwind + PyValueError the way rust_mort2nested already does at lib.rs:103.

---

Generated by Claude Code

[espg]

🤖 from Claude

Fixed on the scalar path too, in 8060dad — the option you list second (wrap in catch_unwind + PyValueError like rust_mort2nested), not the doc-qualification one.

Both bindings now capture:

let densified = py
    .allow_threads(|| std::panic::catch_unwind(|| moc::to_order(&data, order)))
    .map_err(|e| PyValueError::new_err(panic_msg(e, "moc_to_order panicked")))?
    .map_err(PyValueError::new_err)?;

and the same shape in rust_moc_to_order_count. Your repro on the rebuilt extension:

mortie.moc_to_order(np.array([0], dtype=np.uint64), 5)
# ValueError: Morton index cannot be zero            (was PanicException)
mortie.moc_to_order(np.array([0], dtype=np.uint64), 5, max_cells=None)
# ValueError: Morton index cannot be zero

Both max_cells settings matter here for the same reason as the batch: the default runs the estimate (rust_moc_to_order_count), None runs the densify (rust_moc_to_order), and each decoded words outside any capture.

The doc block flagged here is updated accordingly: the unqualified "raises ValueError" claim is now true for the word class as well, and the sentence that said a malformed word "is a separate matter — it still panics in mort2nested" is replaced by one saying the estimate runs under the same capture as the densify.

Test: mortie/tests/test_coverage.py::TestMocToOrderGuard::test_malformed_word_is_a_catchable_valueerror, which drives the wrapper at both max_cells settings through a bare except Exception (the handler shape a PanicException slips past) and then both _rustie bindings directly, past the wrapper fence — matching test_kernel_refuses_out_of_range_order_behind_the_wrapper two tests above it.

[espg]

🤖 from Claude (review)

Adversarial pass over 42822a0...9a35271 (phases 1 and 2), fresh context. Verified locally in this worktree: cargo check --all-targets clean, cargo test --lib moc:: 58 passed, and the bindings exercised directly against the built extension.

Phase 1 (#161) holds up. check_order is the right bound (MAX_DEPTH = 29 is also the ceiling decimal_morton::order_of can decode, so depth ≤ 29 and the shift is capped at 58), the threading through validate_batch / mocs_to_orders is type-correct, and the two .expect(...) sites are genuinely unreachable — polygon_descend / validate_multi assert order ∈ 1..=29 before to_order is called, and dissolve's max_depth is a max over decoded depths. Confirmed the new behaviour end to end: order 30/38/255 now raise ValueError from both bindings, order 29 on a depth-28 word still counts 4. On question (1) of the PR body: keeping those two infallible reads right to me — widening them to Result would ripple through every binding for a branch that cannot be taken.

Four findings, one of them worth acting on before phase 2 closes — the batch densify's default path still escapes as PanicException, and the run_moc doc rewritten in this commit asserts it does not. Details inline.

Not a diff finding, but visible from the PR page: the body still shows phase 2 unchecked and "Pending — lands next commit", and question (4) still asks where #162's Python claim lives, though 9a35271 already landed it at mortie/batch.py:55. Worth reconciling so the checklist reflects the branch (§2).

---

Generated by Claude Code

[espg]

🤖 from Claude (review)

The batch densify still emits an uncatchable PanicException on its default path, and the run_moc doc rewritten in this commit says it does not.

to_order_count decodes every word (mort2nested) right here, in the serial pre-validation pass — outside run_moc's catch_unwind. mortie/batch.py:334 defaults max_cells=_FLAT_COVER_WARN_THRESHOLD, so if let Some(budget) is the default path, and a malformed word in it escapes as pyo3_runtime.PanicException — BaseException-derived, so except Exception misses it. That is the exact failure class #161/#108 exist to remove. Verified against this branch's maturin develop --release build:

>>> v   = np.array([0, 1152921504606846982], dtype=np.uint64)
>>> off = np.array([0, 2], dtype=np.int64)
>>> mortie.mocs_to_orders(v, off, 8)                  # default budget -> this branch
pyo3_runtime.PanicException: Morton index cannot be zero
>>> mortie.mocs_to_orders(v, off, 8, max_cells=None)  # budget off -> reaches run_moc
ValueError: moc 0: Morton index cannot be zero

The new doc at lines 206–217 asserts the opposite — "the capture is live for every kernel here, densify included: layout validation does not screen the morton words themselves". Validation does decode them whenever a budget is set, and the test it cites as evidence, malformed_word_names_lowest_index_across_chunks (line 667), only drives mocs_and / mocs_intersect — never the densify. So the claim is unverified for the path it names and false for the default call.

to_order_count returning Result (this PR) makes the fix a one-liner: run the estimate under the same capture, and the doc becomes true as written.

Suggested change

	let estimated = to_order_count(&values[s as usize..e as usize], order)
	.map_err(|e| format!("moc {i}: {e}"))?;
	let estimated = run_moc(i, || to_order_count(&values[s as usize..e as usize], order))?
	.map_err(|e| format!("moc {i}: {e}"))?;

Worth a test alongside it: the malformed-word case with a budget set, asserting moc 0: Morton index cannot be zero rather than a panic.

---

Generated by Claude Code

[espg]

🤖 from Claude

Applied your suggestion verbatim in 8060dad — validate_batch's estimate now runs under run_moc, so the doc at lines 206-217 becomes true as written rather than needing to be walked back.

let estimated = run_moc(i, || to_order_count(&values[s as usize..e as usize], order))?
    .map_err(|msg| format!("moc {i}: {msg}"))?;

Reproduced your before/after on a maturin develop --release build of the branch. Before: default budget gave pyo3_runtime.PanicException, max_cells=None gave ValueError: moc 0: Morton index cannot be zero. After: both give the ValueError, same string.

Doc changes that follow from it: the paragraph declaring the serial pre-validation outside the capture is replaced by one saying it is inside; validate_batch's own doc gains a sentence noting the estimate decodes every word and therefore runs under run_moc; and the "tested from both sides" sentence now names the densify test rather than resting on malformed_word_names_lowest_index_across_chunks, which is still cited for the rayon/chunk-seam half.

Tests, as you suggested: Rust moc::batch::tests::malformed_word_in_the_budget_estimate_is_named asserts moc 0: Morton index cannot be zero with a budget set and that the budget-on and budget-off errors are the identical string; Python test_malformed_word_is_a_named_value_error_at_both_budgets covers the default, an explicit budget, max_cells=None, an except Exception handler, and a bad word at index 1 for the lowest-index rule. The scalar path got the same fix and its own test (see the thread on lib.rs:1048).

Verification on the branch head: cargo test --lib 364 passed / 0 failed / 1 ignored, cargo fmt --check clean, cargo clippy --all-targets reporting nothing on either touched file, and pytest mortie/tests 1408 passed / 40 skipped / 0 failed (a fresh venv missing shapely, pandas and pyarrow accounts for every earlier failure; all green once the optional extras are installed).

[espg]

🤖 from Claude (review)

Two small things on this line.

1. The map_err arm is unreachable. mocs_to_orders opens with validate_batch(...)?, which rejects !(0..=29).contains(&order) before anything parallel runs, and out-of-range order is the only Err to_order can return. Same for the one at line 126. Not harmful, but it is a dead arm no test can cover (§4's "no dead code") — if it is meant as defence in depth against a future caller that skips validate_batch, a one-line comment saying so would keep the next reader from hunting for the input that trips it.

2. |e| shadows the end offset e bound on line 262 and used in &values[s..e] in the same expression. It compiles to the right thing (the closure param wins inside format!), but a reader checking whether the message names an offset or an error string has to work it out. |err| format!("moc {i}: {err}") reads unambiguously.

---

Generated by Claude Code

[espg]

🤖 from Claude

espg ruled in session (2026-08-16) to fold this rather than leave it standing. #185 had already been rebase-merged by then, so the fix lands in #191 (branch claude/small-fixes-2026-08-16, off post-merge main) — a push here no longer reaches main.

Verified both points against post-merge main first, since 1d7a412 landed between your review and the merge:

(1) The unreachable arm — stands, fixed in 3d95c6f. Still unreachable after the panic-capture change: validate_batch opens with the 0..=29 refusal, and an out-of-range order remains the only Err to_order / to_order_count return. It is defence in depth, exactly as you guessed, so both sites now say so instead of leaving the next reader to hunt for the input that trips it:

// `run_moc`'s `?` is the live arm (a malformed word).  The inner
// one is defence in depth: `to_order_count` fails only on an
// out-of-range `order`, refused by the check opening this function
// — prefixed anyway so both spellings of this call keep the
// documented per-MOC contract (issue #162 review).

with the parallel-pass arm carrying the matching note. Comment-only, no behavior change.

(2) The |e| shadowing — stale, no action. Already fixed by 1d7a412 before the merge: both sites read .map_err(|msg| format!("moc {i}: {msg}")), so the closure parameter no longer shadows the end offset e bound on the line above. Your suggested spelling, arrived at independently in that fold.

Note the arm at what was line 126 now also sits behind run_moc (the panic-capture fix, 47d9e8a on main), so its live arm is a malformed word rather than nothing at all — which is what the new comment names.

[espg]

🤖 from Claude (review)

Two doc-accuracy points, in a phase whose whole job is doc accuracy.

1. The parenthetical is attached to the wrong side. As written, "than on [crate::moc::batch]'s densify, where coarsening reaches 60x (vertices are f64 and no coarsen direction can shrink a cover to nothing)" reads as explaining the densify — which takes u64 morton words and has no vertices at all, and whose 60x row is precisely a case where coarsening does shrink the result to near nothing. The two clauses are the reason the copy is gentler here, not there. Something like "…gentler here than on crate::moc::batch's densify, which reaches 60x when coarsening: here the input is f64 vertices and no direction shrinks a cover to nothing, so the ratio stays near 1" says what issue Docs: the batch memory posture omits the mandatory input copy (coverage.py + coverage/batch.rs) #162's scope paragraph says. The same sentence is mirrored at mortie/batch.py:68.

2. A stale copy of the corrected claim survives 20 lines below, in this file, in the CHUNK doc (lines 55–59): "…put 2048 at the pre-chunking throughput with the peak still ~1.1x the result." That is the pre-Docs: the batch memory posture omits the mandatory input copy (coverage.py + coverage/batch.rs) #162 model — the module header you just wrote measures 1.70x and 1.56x of the result alone for the same code path. A reader who lands on CHUNK gets the claim this issue exists to retire. Either re-express it against input + result (where 1.1x is still right: 21.9/19.8 = 1.11) or point it at the header.

---

Generated by Claude Code

[espg]

🤖 from Claude

espg ruled in session (2026-08-16) to fold the remaining threads. Verified both points against post-merge main — both were already folded by 1d7a412 ("fold review: stale CHUNK peak claim, conversion-copy floor, run_moc serial gap"), which landed after this review and before the merge. No further change; quoting the current text so the resolution is checkable rather than asserted.

(1) Misattached parenthetical — fixed. The module header now reads:

…1.70x and 1.56x the result alone, but 1.11x and 1.02x of input + result. Those ratios stay near 1 because this path has no coarsen direction that can shrink the result to nothing — crate::moc::batch's densify does, and reaches 60x there — but near 1 is not the same as negligible…

Both clauses now attach to here, and the densify is named as the contrast rather than as the thing being explained — the shape you asked for. The mirror at mortie/batch.py got the same treatment.

(2) Stale CHUNK claim — fixed. That doc now reads "…put 2048 at the pre-chunking throughput with the peak still ~1.1x of input + result (the module header's model; that sweep quoted it against the result alone, which omits the input copy — issue #162)". Re-expressed against input + result as you suggested, and your arithmetic holds: 21.9 / (6.9 + 12.9) = 1.106.

One live item did come out of the sibling thread on mortie/batch.py — the measurement had no committed harness — and that is fixed in #191, which also adds a citation to this header at the table.

[espg]

🤖 from Claude (review)

Same misattached parenthetical as src_rust/src/coverage/batch.rs:31-33: read literally, "(vertices are f64 and no coarsen direction can shrink a cover to nothing)" describes :func:mocs_to_orders, which takes uint64 morton words — no vertices — and whose 60x figure is exactly a coarsen that does shrink the result to nothing. Both clauses are the reason the copy is gentler here.

Two smaller notes while this docstring is open:

• The numbers check out — I reproduced 6.9 MiB of input and a 12.9 MiB result for 100k order-8 footprints (values 12.12 MiB + out_offsets 0.76 MiB), and the 555,867 row scales exactly. But unlike :func:mocs_and, which cites benchmarks/measure_mocs_and.py --mem, this measurement has no committed harness — benchmarks/measure_batch_coverage.py is timing-only. A --mem case there (mirroring measure_mocs_and.py:106) would make the table re-runnable rather than a claim on trust.
• "the vertex arrays are a full second resident copy" is the floor, not the ceiling: the wrapper's own np.asarray(lats, dtype=np.float64) at line 153 copies again for any input that is not already contiguous f64 (a list, or an f32 column). Worth a half-sentence, since "size a worker off input + result" under-sizes for a float32 caller.

---

Generated by Claude Code

[espg]

🤖 from Claude

Three points here; verified each against post-merge main. Two were already folded by 1d7a412; the third is live and lands in #191 (#185 was rebase-merged before these threads were addressed, so a push to its branch no longer reaches main).

Parenthetical — already fixed. The docstring now reads "Those ratios stay near 1 because covering has no coarsen direction that can shrink the result to nothing; :func:mocs_to_orders does, and reaches 60x there." Both clauses attach to this function, and the 60x is the contrast, not the thing being explained.

"Full second resident copy" as a floor — already fixed. Now: "That is a floor, not the whole cost: passing anything this wrapper has to convert — a list, float32, a non-contiguous slice — adds the conversion's own copy on top, so hand it contiguous float64/int64 arrays to pay the copy only once." Your np.asarray point, including the float32 caller who would otherwise be under-sized.

No committed harness — live, fixed in fb4e175. You are right that this was the one measurement in the module quoted without a way to re-run it, where mocs_and cites benchmarks/measure_mocs_and.py --mem. benchmarks/measure_batch_coverage.py now takes --mem N [order] and reports input, result and peak with both ratios:

$ python benchmarks/measure_batch_coverage.py --mem 20000 8
n=20000 order=8 cells=318029  [ru_maxrss watermark (lower bound; no /proc/self/statm)]
input  :      1.4 MiB   result:      2.6 MiB
peak   :      6.3 MiB   =  2.43x the result alone,  1.59x of input + result

One deliberate divergence from measure_mocs_and.py:106: that one uses ru_maxrss, whose own docstring concedes it is a watermark lower bound that reads as zero growth whenever an earlier phase out-peaked the call — which would not reproduce a peak table. This docstring names /proc/self/statm sampling as the method that produced the numbers, so the harness samples statm in a thread across the call and keeps ru_maxrss only as a fallback where /proc is absent, printing which one it used. Caveat for review: this machine is macOS, so only the fallback path has actually been run — flagged under "Questions for review" on the new PR.

Both docs now cite the harness at the table.

[codspeed-hq]

Merging this PR will improve performance by 10.51%

⚠️ 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

⚡ 2 improved benchmarks
✅ 69 untouched benchmarks
⏩ 1 skipped benchmark¹

Performance Changes

	Benchmark	BASE	HEAD	Efficiency
⚡	fast_norm2mort_orders[6]	298.6 ns	269.4 ns	+10.82%
⚡	fast_norm2mort_orders[10]	315.3 ns	286.1 ns	+10.19%

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/small-fixes-2026-08-10 (8060dad) with main (6321e61)²}

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

2. No successful run was found on main (afae935) during the generation of this report, so 6321e61 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[espg]

🤖 from Claude

Both phases are landed and both adversarial self-reviews are folded. What each review found and what I did with it:

Phase-1 review → 52b1f59

finding	disposition
validate_batch's ? drops the moc {i}: prefix its own doc promises	fixed — .map_err(|msg| format!("moc {i}: {msg}"))?, matching the mocs_to_orders site
the rewritten run_moc doc asserted densify = defensive / set ops = live, wrong for densify	fixed — the capture is live for every kernel in the parallel pass, and the cited test is now named accurately (malformed_word_names_lowest_index_across_chunks covers the set ops only)
rust_moc_to_order_count's new doc said the estimate "can never be a fabricated one", unqualified	fixed — qualified to the order argument, with the malformed-word panic called out as separate
a malformed word escapes except Exception on mocs_to_orders' default budget path	not fixed — see question (1) in the body

Phase-2 review → 0322b2f

finding	disposition
stale "peak still ~1.1x the result" in coverage/batch.rs's CHUNK doc, contradicting the corrected header	fixed — re-expressed against input + result, which is what that 1.1x actually measured (21.9 / 19.8 = 1.11)
the "vertices are f64 / no coarsen direction" parenthetical attached to the densify, which it does not describe	fixed in both the Rust header and the Python docstring — the clauses now explain why the ratio stays near 1 here
the binding copy is a floor: the wrapper's own np.ascontiguousarray(np.asarray(..., dtype=np.float64)) copies again for a list / float32 / non-contiguous input	fixed — the docstring says so and tells callers to pass contiguous float64/int64 to pay it once
|e| shadowed the end offset e used in &values[s..e] in the same expression	fixed — renamed to |msg| at both sites
the map_err arms are unreachable today	kept, and disclosed — see question (3) in the body
no committed harness for the measured table	not added — see question (6); #162 says docstring-only, so I reverted the --mem mode I had drafted

Green locally at 0322b2f: cargo test 360 passed / 0 failed / 1 ignored, pytest 1436 passed, 16 skipped, cargo fmt / cargo clippy --release / flake8 --select=E9,F63,F7,F82 / numpydoc lint mortie/*.py all clean, and ruff check mortie reports only the 12 pre-existing findings (none in a file this PR touches). On CI the ruff and numpydoc validation jobs are already green; the test matrix is still running.

Six items are standing for a decision under "Questions for review" in the body. The one worth reading first is (1): the batch densify still turns a malformed word into an uncatchable PanicException on its default max_cells path while giving a catchable ValueError with max_cells=None — the same failure class as issue #161, but a different input, so I flagged it instead of widening scope. It is a one-line fix plus a test if you want it folded here.

Leaving this in draft pending the test matrix and those answers.

---

Generated by Claude Code

[espg]

🤖 from Claude

CI status update on 0322b2f — the test matrix I said was still running has finished. All 24 checks are complete with zero failures: test (3.10), test (3.11), test (3.12), arro3-no-pyarrow, ruff, numpydoc validation, Build (verify only), the sdist and all five wheel builds (ubuntu / windows / macos / macos-15-intel / Linux ARM64), Python and Rust benchmarks, CodSpeed, and both codecov/patch and codecov/project are green; the release-only jobs (PyPI / TestPyPI / GitHub Release / changelog / mike deploy) correctly skipped.

That matches the local run: cargo test 360 passed / 0 failed / 1 ignored and pytest 1436 passed / 16 skipped.

Leaving it in draft deliberately, not because CI is red. Both phases are done, but the six items under "Questions for review" need your call before this is ready — the ball is in your court, which is what the waiting label records. The one to read first is (1): mocs_to_orders still turns a malformed word into an uncatchable PanicException on its default max_cells path, while max_cells=None gives a catchable ValueError — same failure class as issue #161, different input, so I flagged it rather than widening scope.

---

Generated by Claude Code

[espg]

🤖 from Claude

Folded the uncatchable-panic finding into this PR — espg ruled option (a) in session (2026-08-16) on the standing scope question, so the PanicException leak the review flagged as pre-existing is fixed here rather than deferred to issue #161's thread.

Fix: 8060dad.

The defect. moc::batch::validate_batch's budget estimate decoded every morton word on the serial path, outside run_moc's catch_unwind. Because mortie.batch.mocs_to_orders defaults max_cells=_FLAT_COVER_WARN_THRESHOLD, that estimate is the default path, so a malformed word escaped as pyo3_runtime.PanicException — BaseException-derived, so neither except ValueError nor except Exception sees it — while max_cells=None gave the documented ValueError. Same input, opposite exception contract, selected by a keyword default. The scalar bindings had the same gap for the same reason.

The fix (one line per path): route the estimate through the existing capture, preserving the moc N: prefix.

// src_rust/src/moc/batch.rs
let estimated = run_moc(i, || to_order_count(&values[s as usize..e as usize], order))?
    .map_err(|msg| format!("moc {i}: {msg}"))?;

and in src_rust/src/lib.rs, rust_moc_to_order / rust_moc_to_order_count now catch_unwind + PyValueError exactly as rust_mort2nested already did.

Before / after, measured on maturin develop --release builds of the branch at each side of the commit:

call	before	after
mocs_to_orders(v, off, 8) (default budget)	PanicException: Morton index cannot be zero	ValueError: moc 0: Morton index cannot be zero
mocs_to_orders(v, off, 8, max_cells=None)	ValueError: moc 0: …	unchanged
moc_to_order(np.zeros(1, np.uint64), 5)	PanicException	ValueError: Morton index cannot be zero
moc_to_order(…, max_cells=None)	PanicException	ValueError

Docs updated to the now-true coverage. The run_moc block's paragraph declaring the serial pre-validation uncovered is replaced by one saying it is covered; validate_batch gains a sentence on why (the estimate decodes every word, so it runs under run_moc like any kernel); rust_moc_to_order_count's "a malformed word is a separate matter — it still panics" is replaced; and the "tested from both sides" sentence now cites a densify test instead of resting on a set-op-only one.

Tests added

• moc::batch::tests::malformed_word_in_the_budget_estimate_is_named (Rust) — names moc 0: with a budget set, and pins that budget-on and budget-off produce the identical string.
• mortie/tests/test_moc_batch.py::test_malformed_word_is_a_named_value_error_at_both_budgets — default, explicit budget, max_cells=None, a bare except Exception handler, and a bad word at index 1 for the lowest-index rule.
• mortie/tests/test_coverage.py::TestMocToOrderGuard::test_malformed_word_is_a_catchable_valueerror — the scalar wrapper at both max_cells settings, plus both _rustie bindings directly, past the wrapper fence.

Local verification: cargo fmt --check clean; cargo clippy --all-targets reports nothing on either touched file; cargo test --lib 364 passed / 0 failed / 1 ignored; pytest mortie/tests 1408 passed / 40 skipped / 0 failed.

Replies are on all four standing threads. The separate points on the unreachable map_err arm and the |e| shadowing are on their own thread and left standing there.

[espg]

🤖 from Claude

Follow-up now that this merged: the three review threads still standing at merge time have been answered, and the two live findings in them land in #191 (draft, claude/small-fixes-2026-08-16 off post-merge main) — espg ruled in session (2026-08-16) to fold them rather than leave them standing.

• r3749046199 — the unreachable map_err arms are now named as defence in depth (3d95c6f). The |e| shadowing half was already fixed here by 1d7a412.
• r3749047866 — both doc points were already folded by 1d7a412; verified against post-merge main and quoted there, no change needed.
• r3749049206 — the parenthetical and the conversion-copy floor were already folded; the missing --mem harness is added in fb4e175, and both docs now cite it at the table.

Nothing else from this PR is outstanding.