small fixes 2026-06-27: densify size guard (#80) + morton_polygon determinism tests (#83)#85
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
==========================================
+ Coverage 93.61% 93.74% +0.12%
==========================================
Files 25 25
Lines 3728 3803 +75
==========================================
+ Hits 3490 3565 +75
Misses 238 238
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of the Phase 1 diff for the pre-emptive flat-cover size guard on moc_to_order (issue #80), reviewed against the acceptance criteria and CLAUDE.md. Overall the design matches the pinned option (2): a max_cells= budget defaulting to 1 << 20, raise-by-default ValueError, max_cells=None opt-out, applied only to the densify path. morton_coverage's post-hoc _warn_large_flat is correctly left untouched. The Rust estimate (Σ 4^(order-depth), saturating) is a sound O(n) upper bound, the binding wiring is correct, and tests cover the four required scenarios. A few findings below — only (1) is a correctness/accuracy concern; the rest are smaller.
(1) [diff-scoped] The "exact over a canonical MOC" claim is wrong when the MOC contains cells finer than order — to_order_count over-counts there. (src_rust/src/moc.rs lines 157-178, doc + finer branch)
to_order (lines 147-150) coarsens a cell finer than order to its ancestor (nested2mort(nested >> up, order)) and then sort_unstable + dedup (lines 152-153). So N finer cells sharing one order-level ancestor collapse to 1 flat cell, and a finer cell whose ancestor coincides with an at-order cell already present is dropped too. But to_order_count's else branch (depth >= order) adds 1 per such cell, so for N>1 sibling/descendant finer cells under one ancestor the estimate is N while the real flat count is 1.
A normalized ("canonical") MOC can absolutely contain cells finer than the requested target order (normalize is independent of the target order), so the docstring/comment claim "exact over a canonical (sibling-merged, ancestor-pruned) MOC" (lines 161-164) is too strong — it's exact only when no input cell is finer than order. For a guard this over-count is safe (never under-counts → never lets an OOM slip through), but it means the guard can falsely raise on a MOC whose true flat count is under budget. Suggest: (a) soften the comment/docstring to "exact when no input cell is finer than order; otherwise a safe over-count (the flat path coarsens+dedups finer cells, so the real count is ≤ the estimate)", and (b) ideally drop the else { 1 } over-count by deduping coarsened-ancestor contributions — or at minimum reflect the over-count in the wording of (1). The Python message already hedges with ~{estimated}, which is good and consistent with an over-count, but the Rust doc/moc.rs comment and the acceptance-criterion "estimate matches the actual flat count" are stated as exact.
(2) [diff-scoped] Test gap vs the over-count case in (1). (mortie/tests/test_coverage.py, TestMocToOrderGuard)
test_estimate_matches_actual_flat_count and the Rust test_to_order_count_matches_flat both use only at-or-coarser cells (order-8 cover densified to order 8; a cover of cells at depth ≤ order plus one finer that happens to be the only descendant of its ancestor). Neither exercises multiple finer cells collapsing to one ancestor, which is exactly where estimate ≠ actual. Add a case with ≥2 finer siblings under one order-order ancestor and assert the estimate is an upper bound (estimate >= actual) rather than equality — that both documents the real contract and guards against a future "tighten the estimate" change silently breaking it.
(3) [diff-scoped / minor] Empty-input and finer-only edge cases are untested. An empty morton should yield estimate 0 (no raise; densifies to empty) and a MOC entirely finer than order should not raise under the default — neither is asserted. One-line additions; cheap insurance for the acceptance criterion's edge cases.
(4) [note — matches the pinned decision, not a blocker] max_cells semantics/default differ from morton_coverage_moc's same-named param. morton_coverage_moc(..., max_cells=None) is a soft refine target (line 198-223); the new moc_to_order(..., max_cells=1<<20) is a hard raise budget. Same name, different default and different behavior. This is consistent with the @espg-pinned raise-by-default decision for issue #80, so flagging only so the divergence is intentional and (optionally) called out in the docstring's See Also.
**(5) [diff-scoped / nit] Docstring says the pass is "O(n)" — fine — but the prose "computed from the compact MOC alone" assumes a compact input; the function accepts any mixed-order set (including a non-normalized one with overlaps), where the estimate over-counts even for coarser cells. Consider "from the input set alone" to avoid implying the caller must pre-normalize.
No CLAUDE.md violations spotted: commit message is title-only (#80 ...), no dependency added, no CI/workflow touched, Rust change carries a cargo test, and the public surface stays Python→Rust with no fallback. Cannot confirm a green build from here (the extension isn't built in this checkout) — flagging that maturin develop --release + cargo test/cargo clippy + pytest should be confirmed green before clearing draft, per §4.
Generated by Claude Code
Merging this PR will not alter performance
Comparing Footnotes
|
|
🤖 from Claude Folded the diff-scoped findings from the Phase 1 self-review into the Phase 2 commit (
(4) (the Local gates green: Generated by Claude Code |
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial review of the Phase 2 diff (commit 2b93a21, "#83 pin morton_polygon tie-break determinism + fold #80 self-review"), scoped to the new TestMortonPolygonDeterminism class in mortie/tests/test_prefix_trie.py and confirming the Phase-1 folds don't regress. Verdict: the tests are correct, well-constructed, and satisfy issue #83's option (1) ("pin current behavior + golden test"). I rebuilt the Rust ext (maturin develop --release) and ran the suite: all 3 new tests pass, and the full test_prefix_trie.py + test_coverage.py run is green (179 passed, 5 skipped), so the moc.rs / lib.rs / coverage.py folds don't break anything. Findings below are confirmations plus two minor, optional hardening notes — none blocking.
1. (confirmed) Logic is genuinely unchanged — diff-scoped. mortie/prefix_trie.py is not in commit 2b93a21 (git show --stat lists only coverage.py, test_coverage.py, test_prefix_trie.py, moc.rs). morton_polygon's (-efficiency, seq, node) heap key is untouched. Matches @espg's pinned option (1): tests only, no tie-break edit.
2. (confirmed) The tie-heavy input genuinely exercises a tie-break — diff-scoped. Traced budget 6 on _TIE_HEAVY: roots 1,2 (eff 0.5) expand, then the frontier 11,12,21,22 all have identical eff 0.5 (exact dyadic floats — depth-0/1/2 effs are 0.5/0.125/0.03125, identical across every sibling, so no float-equality fragility). With remaining budget = 2, only two of those four can expand; seq (2,3 vs 4,5) makes 11,12 win and leaves 21,22 unexpanded → ['111','112','121','122','21','22']. The budget cutoff genuinely falls mid-tie and seq is the deciding factor, exactly the determinism this pins. Good — not a trivially-unique ordering.
3. (confirmed) Golden is properly pinned — diff-scoped. test_golden_tie_break asserts both the explicit list and a sha256; I recomputed the digest from the list and it matches f87e4659...e276d. Solid double-pin.
4. (confirmed) PYTHONHASHSEED test is meaningful — diff-scoped. test_stable_across_pythonhashseed spawns 4 separate interpreters via subprocess.run([sys.executable, "-c", script], env={..., "PYTHONHASHSEED": seed}, check=True) over seeds 0/1/12345/99999, collects stdout into a set, and asserts len == 1. Genuinely varies the seed across real processes and compares — it would actually catch a reintroduced set/dict ordering leak. Not a no-op.
5. (minor, optional) The multi-budget loop adds no tie-break coverage beyond budget 6 — diff-scoped, non-blocking. In test_repeatable_within_process, for budget in [4, 6, 8, 10, 12]: assert self._chars(budget) == self._chars(budget) only compares two calls of the same budget (a real fresh-node-identity check, which is fine). But budgets 4/8/12 land on full tie boundaries (whole levels expand), so they don't exercise a mid-tie cutoff — only 6 and 10 do. The loop is correct, just doesn't broaden the tie-break exposure the way its placement might suggest. No change required.
6. (minor, optional) Golden + hashseed pins a single budget (6) — diff-scoped, non-blocking. Both the golden and the subprocess test fix n_cells=6, so a regression that only manifests on a deeper mid-tie cut (e.g. budget 10, which the trace shows also resolves a tie asymmetrically at the 1111/1112/... level) wouldn't be caught by the golden. Adding a second golden at budget 10 would harden the pin against deeper-level tie-order regressions. Purely optional given option (1)'s minimal scope.
No CLAUDE.md violations: no noqa/lint-disable, no weakened/skipped tests, no @-mentions, all four new imports (os, subprocess, sys, textwrap) are used, no F401, and no E501 in the new line range (398-490) at --max-line-length=88.
Generated by Claude Code
|
🤖 from Claude Phase 2 self-review came back with only non-blocking confirmations and two optional hardening notes. Folded the worthwhile one (
All other Phase 2 findings were confirmations (logic genuinely unchanged, tie genuinely exercised, golden properly pinned, hash-seed test cross-process). No side-effecting findings. Both phases are now complete. Local gates green: Generated by Claude Code |
|
🤖 from Claude Correction to my note above: leaving this as a draft (this routine run is constrained to draft-only and does not mark PRs ready). Both phases are complete, all self-review findings are folded, and CI is green — so the ball is in your court for review. Applying the Generated by Claude Code |
Bundled
small-fixPR for two issues.Closes #80
Closes #83
Phases
moc_to_ordermorton_polygontie-break determinism (golden + repeatability tests)Phase 1 — #80: pre-emptive densify size guard
Per the @espg decision: option (2) — raise-by-default with an opt-out flag — applied only to the densify path
moc_to_order.morton_coverage's existing post-hoc warn behavior is unchanged.What/approach. The densify path is the real OOM hazard: a tiny compact MOC can densify to billions of flat cells before any warning is reachable. The densified count is computable from the compact MOC before allocation, since each morton word self-encodes its depth — a cell coarser than
orderexpands to4**(order-depth)leaves (src_rust/src/moc.rsto_order), soestimated = Σ 4**(order-depth)(cells at/belowordercontribute 1). This is an O(n_moc) pass with no flat allocation.moc::to_order_count(morton, order)mirrorsto_order's arithmetic and returns the flat count, saturating so a pathological input can't overflow the guard. Bound to Python asrust_moc_to_order_count(src_rust/src/lib.rs). The Rust estimate is used (not a Python loop) to keep it cheap over large arrays.moc_to_order(morton, order, max_cells=1<<20)(mortie/coverage.py) estimates first and raisesValueErrorby default when the estimate exceedsmax_cells, naming the estimate and the budget.max_cells=Noneopts out and densifies unconditionally. Default budget reuses the existing_FLAT_COVER_WARN_THRESHOLD = 1 << 20constant.Estimate accuracy. The bound is exact when no input cell is finer than
order(the common densify case). When the input holds cells finer thanorder,to_ordercoarsens them to their ancestor and dedups, so finer siblings collapse to one flat cell while the count adds 1 each — a safe over-count, so the guard never lets more thanmax_cellsthrough (it can only conservatively raise under budget). Docstrings and tests state this precisely (folded from the Phase 1 self-review — see the PR comment below).How tested.
cargo test --release(176 pass) —test_to_order_count_exact_when_no_finer_cells(estimate == real flat length, exact value 66),test_to_order_count_overcounts_finer_siblings(4 finer siblings collapse to 1; estimate is an upper bound),test_to_order_count_empty.pytest mortie/tests/test_coverage.pyTestMocToOrderGuard— above-budget raises by default with the estimate named;max_cells=Noneproceeds; under-budget unaffected; estimate brackets the actual flat count on a canonical MOC; empty input; finer-cell over-count is a safe upper bound.cargo fmt,cargo clippy --release(no new warnings reference the added fn; remaining are pre-existing in unrelated functions),flake8 mortie --select=E9,F63,F7,F82clean.Phase 2 — #83: pin
morton_polygontie-break determinismPer the @espg decision: option 1 — pin the current behavior + golden test. No change to the tie-break logic —
morton_polygonis already deterministic (Pythonheapqkeyed by(-efficiency, seq, node)with a unique monotonicseq,mortie/prefix_trie.py), so ties resolve by insertion order with no hash/dict/set dependence. Tests only.What/approach. Added
TestMortonPolygonDeterminisminmortie/tests/test_prefix_trie.py. The input is a deliberately symmetric, tie-heavy set (four identical-shape subtrees under base cells 1 and 2), so at budget 6 the budget cutoff falls in the middle of a tie and theseqtiebreak decides which symmetric subtree expands — exactly the exposed decision.test_repeatable_within_process: 50 calls on the tie-rich input return a byte-identical characteristic list (and several budgets are each rebuilt-and-compared).test_stable_across_pythonhashseed: runs the cover in subprocesses underPYTHONHASHSEED∈ {0, 1, 12345, 99999}; the emitted list is identical across all — the one real run-to-run exposure if hash-ordered containers were ever reintroduced on this path.test_golden_tie_break: pinned golden (list + sha256) for budget 6, in the style of the existingtest_prefix_trie.pygoldens.How tested.
pytest mortie/tests/test_prefix_trie.py(all pass, incl. the subprocess hash-seed test); full suitepytest -q→ 451 passed, 8 skipped;flake8 mortie --select=E9,F63,F7,F82clean.Questions for review
Two choices on #80 that were not explicitly pinned (defaults applied, non-blocking):
ValueError, consistent withcoverage.py's existing1 <= order <= 29/ mutually-exclusive-arg raises. Alternative:MemoryError(signals the actual hazard more directly). Easy to switch if preferred.1 << 20constant (the existing warn line) as the defaultmax_cells. Alternative: a separate, higher hard-guard line — the warn is a nudge, an OOM wall could reasonably sit higher.This PR's work was authored by an automated Claude routine run on the
claude/small-fixes-2026-06-27branch.