Freeze audit: refresh stale ≤18-era / signed-decimal docstrings (issue #68)#130
Freeze audit: refresh stale ≤18-era / signed-decimal docstrings (issue #68)#130espg wants to merge 1 commit into
Conversation
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of the 1.x-freeze audit. Verdict: looks good to advance — the diff is safe and the audit's claims hold up under spot-checking. No diff-scoped changes required.
Diff verification (non-behavioral, accurate)
- The only code change is 2 docstring param lines in
mortie/prefix_trie.py(232, 311):"Morton indices (signed integers)."→"Morton indices (packed uint64 words; base cells 7-11 set bit 63).".git diff main...shows one commit,2 insertions(+), 2 deletions(-), nothing else. - The new wording is correct:
split_childrencasts its input withnp.ascontiguousarray(np.asarray(morton_array, dtype=np.uint64))(prefix_trie.py:242), so the old "signed integers" was genuinely stale.morton_polygon_from_arrayroutes through the samesplit_children. - No public export removed / renamed / privatized, no signature change, no behavior change — this respects the freeze rule. Docstring-only, so the CLAUDE.md §4 "tests for behavioral change" gate does not apply.
Audit claims spot-checked — all confirmed
tools.py:643-653clip2order(print_factor=True)returns a hard-coded18 - clip_order. This is a genuine latent behavioral quirk (assumes source order 18 regardless of the words' actual order), and the docstring already flags it as legacy back-compat. Correctly left unfixed and raised for espg rather than silently "corrected" — agreed, changing it is a behavior/API call.prefix_trie.py:159-172cell_areadigit-count contract reads as described.- Rust is clean:
decimal_morton::MAX_ORDER = 29,moc.rsMAX_DEPTH = MAX_ORDER(31), and the doc-comments atmorton.rs:57,moc.rs:29,moc.rs:771all state0..=29/1..=29and explicitly note the old18was the retired decimal-i64 ceiling, not a u64 limit. No stale order cap insrc_rust/src/*.rs.
Adversarial sweep for anything missed — nothing found
- No remaining literal order-18 behavioral cap in
mortie/*.pyor the Rust sources; everyorder=18is a default parameter value, andlib.rsvalidates againstMAX_ORDER, not 18. - No stale "signed integers" / "orders 0-18" docstring describing the current encoding wrongly. The remaining
Morton indicesdocstrings (tools.py:719,766"same order",coverage.py:280,308,549,608"mixed order allowed") are accurate. All0..=18mentions I could find are correctly legacy-scoped (to_legacy_i64,from_legacy,decimal_repr_matches_legacy_*) and not in scope per the freeze audit. - No signed-southern-hemisphere assumption remaining in the public path; the
signedhits are legitimate (spherical signed-area geometry, thenormed/parentnote, unsigned-Z-order regression tests).
Standing note for espg (not a directive): the clip2order print_factor hard-coded 18 (tools.py:653) is the one live behavioral wart the audit surfaces. Confirm whether it should stay frozen as-is for 1.x back-compat or be reworked to a real per-word level count — that's a scope/API decision, correctly deferred to you rather than folded into this docstring PR.
No inline diff-line findings; the 2 changed lines are correct as written.
Generated by Claude Code
Merging this PR will not alter performance
Comparing Footnotes
|
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial review of the freeze-audit (issue #68 acceptance criterion is an accurate file:line findings list, so I checked the refs, not just the diff).
Diff itself — clean. Both docstring edits are accurate: input is cast to np.uint64 at prefix_trie.py:242, and the new wording ("packed uint64 words; base cells 7-11 set bit 63") matches the established language at tools.py:340. Docstring-only, no runtime behavior touched, no __all__ visibility changed. Good call leaving the clip2order order-18 anchor (return 18 - clip_order, real, at tools.py:806) as a raised behavioral finding rather than silently fixing it — that respects the no-unilateral-change scope.
One defect: several findings-list file:line refs in the PR body are wrong (the substance is correct, the coordinates are stale). Since the deliverable per #68 is the ref'd list, these should be corrected:
- Finding (iii)
clip2order— citedtools.py:643-645(docstring) andtools.py:653(return 18 - clip_order). Actual: docstring attools.py:795-798, return attools.py:806. (Lines 643-653 are the unrelated antimeridian helper.) - Section (ii) item 4 —
norm2uniqcitedtools.py:302, actualtools.py:388;uniq2geocitedtools.py:331, actualtools.py:417. (geo2uniq:121andunique2parent:56are correct.)
Everything else I spot-checked matched: res2display:27 (confirmed print-only, no return), heal_norm:74, MortonChild:40, split_children:226, split_children_geo:252, __all__ at __init__.py:72 / lazy += at 149-150, and morton.rs 0..=29 near :57. Findings are otherwise well-grounded and the clean/behavioral separation is sound.
Generated by Claude Code
Refs #68 (sub-issue of #48, dovetails with #60). Phase-1 audit for the 1.x freeze: a full pass over
mortie/*.pyandsrc_rust/src/*.rsfor ≤18-era order caps, signed-decimal assumptions, and stale docstrings, plus an enumeration of the public API surface.Bottom line: the codebase has already been largely modernized by the packed-
u64work (#48/#58/#60/#120). The encoding-range docstrings that once said "1–18" are already gone; the residual stale wording was in the prefix-trie module. Mechanical fixes are folded in below; everything touching runtime behavior or the public surface is raised for review rather than changed (per the freeze's no-unilateral-API-change rule — downstream like zagg imports these names).File:line refs below were re-verified against this branch's actual files after self-review (
mortie/tools.pyis 866 lines here); they are accurate forclaude/68-freeze-audit.Phases
clip2orderorder-cap decision below.(i) Mechanical fixes folded in (this commit)
Docstring-only, no behavior change:
mortie/prefix_trie.py:232and:311— parameter docstring said "Morton indices (signed integers)." The input is cast tonp.uint64atprefix_trie.py:242; the words are packeduint64(base cells 7‑11 set bit 63), not signed int64. Updated to "Morton indices (packeduint64words; base cells 7‑11 set bit 63)." to match the language intools.py.(ii) Public API surface — verdicts (
mortie/__init__.py __all__)No visibility changed. Enumerated
__all__(__init__.py:72-119) plus the lazy+=additions (:149-150). Most exports are clearly intended-frozen (geo2mort,mort2geo,mort2bbox,mort2polygon,infer_order_from_morton,validate_morton,mort2norm,norm2mort,order2res,unique2parent,mort2healpix,morton_buffer[_meters], themorton_coverage*/moc_*/common_ancestor/moc_min/split_base_cellsMOC family,linestring_coverage, thefrom_*/to_*geometry codecs,generate_morton_children,clip2order, and theMortonIndex*/ Arrow_ARROW_NAMES).Questionable — flagged for an @espg keep/privatize/reshape decision (I did NOT change them):
res2display(tools.py:27) — aprint()-only helper with no useful return value. Odd thing to freeze in a public API. Keep as-is, or return structured data / move to a docs example?MortonChild(prefix_trie.py:40) — exposes an internal compacted-trie node class (its__init__takes a raw shared char-array + mask). Freezing this locks the trie's internal representation into the public contract. Intended, or should the frozen surface be just the*_polygonfunctions that return lists of these?heal_norm(tools.py:74) — undocstringed low-level(base, order, addr_nest) -> addr_normhelper. Intended public, or an internal used only bynorm2mort/geo2uniq?geo2uniq(tools.py:121) /norm2uniq(tools.py:302) /uniq2geo(tools.py:331) andunique2parent(tools.py:56) —unique2parentis self-described as "such an ugly hack … will blow up with multi res" and raisesNotImplementedErroron mixed resolution. Freeze the UNIQ helpers as-is for 1.x, or mark them provisional?split_children/split_children_geo(prefix_trie.py:226/252) — low-level trie builders exposed alongside the higher-levelmorton_polygon*. Borderline; likely intended but worth confirming they're part of the frozen contract.(No dependency concern found:
heal_norm/geo2uniq/uniq2geouse the internal Rust-backedmortie._healpix(hp), not healpy — numpy stays the only runtime dep.)(iii) Stale order-cap / signed findings needing a design decision (NOT changed here)
clip2order(print_factor=True)is hard-anchored to order 18 —tools.py:643-644(docstring) andtools.py:653(return 18 - clip_order). The "levels dropped" count is computed against a fixed18, a ≤18-era assumption; for order‑19…29 words the returned count is wrong (e.g. clipping an order‑29 word to order‑20 reports18-20 = -2). This is a behavioral fix, so I did not touch it. Options: (a) use the word's actual decoded order instead of18; (b) useMAX_ORDER; (c) deprecateprint_factorfor the freeze. May warrant its own issue — needs @espg go-ahead.Confirmed clean (no change needed)
morton.rs:57(order range0..=29),moc.rs:29,771(MAX_DEPTHlifted 18→29 for morton_coverage/morton_coverage_moc cap order at 18 while geo2mort accepts up to 29 #60),decimal_morton.rsandmorton_index.py:543-559legacyto_legacy_i64references (the legacy decimal genuinely tops out at order 18 due to int64 overflow — correct, intentional compat). No.rsfiles were edited.cell_areadigit-count→order contract is correct (prefix_trie.py:159-172): thecharacteristicis the decode-through-kernel decimal repr (prefix_trie.rs:1-14,66-100), which is the order‑0…29 generalization — one sign column + one base‑4 digit per order — so counting digits still yields the order. Not stale.test_coverage.py:669,683). The only literal18comparisons in code are the intentional legacyto_legacy_i64cap and theclip2orderitem in (iii).How tested
maturin/editable build green;mortieimports.flake8 mortie --select=E9,F63,F7,F82— clean.pytest— 683 passed, 11 skipped, 1 failed. The single failure is the pre-existingtest_spec_page.py::TestDecimalParseTieBreak::test_order29_string_parses_to_area_word(issue main is red: test_spec_page tie-break test predates #120's p-suffix rendering #123, main is already red on it), unrelated to this docstring change..rschanges, socargogates not exercised.Questions for review
clip2orderorder‑18 anchor in (iii) — fix in this PR (phase 2), or split to its own issue?Refs, notCloses, for now.)🤖 Generated with Claude Code