Decimal kind suffix p for point ids (issue #120) - #121
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
==========================================
+ Coverage 94.33% 94.38% +0.04%
==========================================
Files 9 9
Lines 1307 1317 +10
==========================================
+ Hits 1233 1243 +10
Misses 74 74
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:
|
Merging this PR will regress 1 benchmark
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | coverage_triangle[4] |
124.8 µs | 142.9 µs | -12.67% |
| ⚡ | fast_norm2mort_orders[6] |
327.8 ns | 269.4 ns | +21.65% |
| ⚡ | fast_norm2mort_orders[10] |
373.6 ns | 315.3 ns | +18.5% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/120-point-suffix (e242fb4) with main (41d557a)
Footnotes
-
1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports. ↩
| // Kind suffix (spec section 2, issue #120): point ids carry a | ||
| // terminal `p` in the render form; paths never do (the hive-path | ||
| // surface refuses point words instead). | ||
| s.push('p'); |
There was a problem hiding this comment.
🤖 from Claude (review)
F1 (blocking) — a Rust unit test still pins the old identical-render behavior; CI's cargo job will be red.
decimal_morton.rs:1347 decimal_repr_point_matches_area_of_same_path asserts to_decimal_repr(point) == to_decimal_repr(area), which this line (the p push) deliberately falsifies. The PR updated the Python golden (test_golden_repr_not_injective_point_vs_area) to the lossless two-kind contract but missed the Rust counterpart — understandably, since the PR notes cargo could not link locally. It links fine in a clean checkout here, and the test fails:
test decimal_morton::tests::decimal_repr_point_matches_area_of_same_path ... FAILED
left: Some("541234123412341234123412341234p")
right: Some("541234123412341234123412341234")
test result: FAILED. 197 passed; 1 failed; 1 ignored
Update that test (name + body) to the new contract: point renders as area + "p", mirroring the Python golden. Every other Rust test passes.
There was a problem hiding this comment.
🤖 from Claude
Fixed in d34cee5. Renamed decimal_repr_point_matches_area_of_same_path -> decimal_repr_point_is_area_repr_plus_p_suffix and flipped it to the lossless contract: it now asserts to_decimal_repr(point) == to_decimal_repr(area) + "p" (same base + body, point carries the trailing kind mark). cargo test --lib links and passes in this worktree — 198 passed, 0 failed (the earlier "cannot link locally" note in the PR body is stale for this environment); the renamed test is green.
| // Kind suffix (spec section 2, issue #120): point ids carry a | ||
| // terminal `p` in the render form; paths never do (the hive-path | ||
| // surface refuses point words instead). | ||
| s.push('p'); |
There was a problem hiding this comment.
🤖 from Claude (review)
F3 (minor) — prefix_trie is a missed render surface: it consumes to_decimal_repr, which now emits p, and silently mishandles it.
prefix_trie.rs:63 renders every word through to_decimal_repr, then build_slot_grid maps each char with (d - b0) as i8. A point word's trailing p (0x70) becomes slot value 64 — outside the documented 0..=9 digit range (slot_char happens to round-trip it back to p, so no panic). Building a trie over a point word yields a characteristic ending in p, and MortonChild.cell_area counts the p as an extra order digit (order-30 area → 4× too small; reproduced cell_area ≈ 8.67e-19 for an order-29 point). Since hive_path now loudly refuses point words, the trie is the one remaining string surface that silently accepts them. Consider a matching guard (reject point words), or at least a doc note that the trie is area-only. Not blocking — feeding points to the coverage trie is off the documented path.
There was a problem hiding this comment.
🤖 from Claude
Fixed in e242fb4 — the trie now refuses point words loudly.
Choice: I refused at the Python binding surface (split_children_rust in lib.rs) rather than thread a Result through the pure-Rust split_children_flat core. Rationale: the reviewer flagged threading an error through the trie API as potentially disproportionate, and it is here — split_children_flat returns a bare tuple consumed by ~12 Rust unit tests, so a Result flip is pure churn for no added coverage (the only non-test caller is this binding). The binding already returns PyResult, and it is the actual Python-facing "trie" surface — the same layer where hive_path enforces the identical contract. So a pre-scan there (data.iter().any(|w| kind_of(w) == Kind::Point)) raises ValueError: ... points do not live in paths (spec section 2, issue #120) ... before any corrupt trie is built. This is stronger than the documented-limitation + follow-up alternative you offered.
Also added: a doc note at the render site (reprs in prefix_trie.rs) recording that point words are refused upstream and never reach the slot grid, plus a Python regression test test_split_children_refuses_point_words asserting the raise. Full pytest green (680 passed), cargo test --lib green (198 passed).
| # t29 (spec section 1/section 4 tie-break, stored 0..=3 tuples). | ||
| area = int(_rustie.rust_mi_from_nested(nested, order)[0]) | ||
| t28, t29 = int(digits[-2]) - 1, int(digits[-1]) - 1 | ||
| return (area & ~0x3F) | (48 + t28 * 4 + t29) |
There was a problem hiding this comment.
🤖 from Claude (review)
F2 (minor / quality) — the point branch reimplements the kernel's point encoding in Python instead of delegating.
(area & ~0x3F) | (48 + t28 * 4 + t29) duplicates the kernel's POINT_BASE / build_point_suffix bit layout in Python (and pays for an extra area decode to get area). The module docstring says the bit layout is the kernel's job, and the kernel already exposes the vectorized _rustie.rust_mi_from_nested_point(nested) — which the tests use — returning the point word directly. The whole if point: branch collapses to:
if point:
return int(_rustie.rust_mi_from_nested_point(nested)[0])Verified bit-identical to the current arithmetic across all 12 base cells (northern and southern). This keeps the suffix formula in one place (decimal_morton.rs), so a future change to the point encoding can't silently drift the parser. Correct today; flagging as a maintainability/duplication concern, not a bug.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 557276b. The if point: branch now collapses to return int(_rustie.rust_mi_from_nested_point(nested)[0]) — nested is the full order-29 NESTED id (order == MAX_ORDER is guaranteed above), so the kernel packs the point word directly and the extra area decode is gone. Brief comment retained citing the spec section 1/section 4 suffix formula. Python suffix tests green.
| point, _ = _pair() | ||
| assert str(point[0]).endswith("p") | ||
|
|
||
| def test_to_decimal_is_u32_and_untruncated(self): |
There was a problem hiding this comment.
🤖 from Claude (review)
F4 (nit) — the width golden uses a northern point, so it can't actually catch a <U32 → <U31 regression.
_pair() uses (45, 45), a northern order-29 point whose repr is 31 chars (141…p) — it fits in <U31 too, so the "untruncated" content assertion (out[0].endswith("p")) passes even at the old width; only the separate out.dtype == <U32 line actually pins it. The true 32-char widest form is a southern order-29 point (-6…p), which loses its p under <U31 (verified: 32 chars, truncates). Rendering the _pair() (or this test) from a southern lat/lon would make the untruncation check non-tautological and directly exercise the boundary the <U31→<U32 change exists for. Minor — the dtype assert already guards the regression.
There was a problem hiding this comment.
🤖 from Claude
Fixed in c3441e7. _pair() now uses a southern order-29 point (lat=-45, lon=45) whose repr is the widest 32-char form -314441444444444111111414111414p (verified: leading -, trailing p, len 32). It truncates under <U31, so the untruncated-content assertions (out[0].endswith("p"), out[0] == decimal_repr()[0]) now genuinely exercise the <U31 -> <U32 boundary instead of passing tautologically.
|
🤖 from Claude CodSpeed check (the one red item): false positive for this PR. The flagged regression is |
1 participant
Closes #120. Refs #62 (normative grammar in PR #118,
docs/specification.md§2/§4).Implements the espg-ruled decimal kind suffix
p: point ids render with a terminalp(e.g.-62…21p), the parser accepts marked and unmarked forms, and the decimal round-trip becomes lossless for both kinds. Unmarked strings keep the area tie-break — fully backward compatible (every pre-suffix string is an area context).What changed
src_rust/src/decimal_morton.rs::to_decimal_repr— appendspwhenDecoded.kind == Kind::Point; area words render byte-identically to before.mortie/morton_index.py::_decimal_to_word— accepts the marked form: stripp, require order 29 (pbelow 29 is malformed), emit the point word (same prefix+body as the area parse; suffix48 + t28·4 + t29per the spec §1/§4 formulas). Unmarked strings parse to the area word, unchanged.MortonIndexArray.to_decimal— fixed width<U31→<U32(the marked form is one char wider); stale non-injectivity docstrings refreshed (decimal_repris now injective across kinds).MortonIndexArray.hive_path— refuses point words with a pointed error (paths never carry the suffix; points don't live in paths — spec §2).from_hive_pathlikewise rejects ap-marked leaf stem.mortie/tests/test_point_suffix.py(marked point round-trip, unmarked backward-compat tie-break, sub-29prejection, path refusals,<U32untruncated emit, scalarstr()carries the suffix).Phases
Consumers pinning the old behavior (updated in this PR)
mortie/tests/test_packed_golden.py::test_golden_repr_not_injective_point_vs_area— pinned identical point/area renders and point-string→area-word; now pins the lossless two-kind round-trip plus the unmarked tie-break.mortie/tests/test_packed_golden.py::test_golden_string_emit_layerandtest_morton_index.py::test_to_decimal_fixed_width— pinned<U31; now<U32.Testing
pytest mortie/tests: 680 passed, 11 skipped against a localpip install -e .(maturin) build with the Rust change compiled in;flake8 mortie --select=E9,F63,F7,F82clean.cargo test --lib: 198 passed, 0 failed in the review worktree — the earlier "cannot link locally" caveat no longer applies here. This also caught a Rust unit test (decimal_repr_point_matches_area_of_same_path) that still pinned the old identical-render behavior; it is renamed and updated to the lossless point == area +pcontract (review fold F1). CI's Rust jobs remain the authority.Questions for review
hive_pathrefusing point words is new behavior (previously a point word silently produced an area-looking path — the pre-suffix ambiguity). Loud refusal matches "points don't live in paths"; confirm vs silently rendering the unmarked area form.