Public decimal→word parse API (issue #114) - #132
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
==========================================
- Coverage 94.60% 94.49% -0.11%
==========================================
Files 9 9
Lines 1371 1380 +9
==========================================
+ Hits 1297 1304 +7
- Misses 74 76 +2
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 not alter performance
Comparing 
Footnotes
|
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial self-review of phase 1 (f93a6f5), fresh context. The parser is correct — I tried hard to break it and could not. One confirmed user-visible defect, one measured design concern that phase 2 will freeze, and some cleanup.
Scope note: 72481f5 ("phase 2 of issue #114") landed on the branch while this review was running. Everything below is scoped to f93a6f5; src_rust/ is byte-identical between the two commits, so the findings still apply. I checked phase 2's mortie/tests/test_decimal_parse.py before writing, so I am not raising "the binding has no Python-level tests" — phase 2 covers empty input, object arrays, non-string rejection, and first-bad-id error ordering.
What I verified (no defect)
I extracted the origin/main _decimal_to_word (mortie/morton_index.py:35) as a standalone reference — necessary because phase 2 rewired the private name to delegate to the new Rust binding, which would have made a naive differential test compare Rust against itself — and fuzzed 380,720 inputs: exhaustive over 12 base cells x orders 0–29 x both kinds, plus random strings over -0123456789p +.²٣x\t\n'"\ and structured near-misses to order 31.
0 value mismatches. 0 exception-type mismatches. Specifically confirmed equivalent: unicode digits (², ٣, ３ all rejected — the is_ascii_digit guard correctly reproduces Python's isdigit() and isascii() pair), + signs, leading zeros, "-0", embedded and leading/trailing whitespace, "P" casing, NUL bytes, non-BMP chars, lone surrogates (UnicodeEncodeError at the PyO3 boundary in both implementations), empty order-digit runs, and the order-29 boundary.
Also checked and clean:
- No panic on multi-byte input.
&s[..s.len() - 1]is sound; reasoning inline on:796. - No overflow. The
order > MAX_ORDERcheck at:812precedes the accumulator loop, sowithinis ≤ 58 bits andbase << (2 * order)peaks at 62 bits (base 11 at order 29). Thefrom_nested/from_nested_pointasserts really are unreachable, as the doc claims. - Spec conformance. Grammar matches
docs/specification.md§2 exactly; the §4 tie-break (marked ⇒ POINT, unmarked ⇒ AREA) is implemented and pinned at the bit level, including the non-injectivity assertion.prestricted to order 29 only, per §2 and §4. - Binding coercion is well-behaved across
list/tuple/<U32array / object array / empty. Notably a barestris rejected (Can't extract 'str' to 'Vec') rather than silently parsed per-character — the footgun that would have madedecimals_to_words("3123")return four order-0 words. Worth a phase-2 test to keep that guarantee. - Your CI claims check out.
cargo test205/0/1,cargo fmt --checkclean, and the single new clippy warning is exactly the known#[pyfunction]useless_conversionfalse positive atsrc_rust/src/lib.rs:1155.
Findings
-
Error messages no longer escape control characters (
:758and its four sibling arms) — the one confirmed behavioral divergence, and broader than your "Questions for review" item (3), which considers only quotes.{s!r}escaped;'{}'does not. A malformed id containing\nnow produces a multi-lineValueError, and malformed ids here arrive from untrusted channels (zarr leaf paths, status-channel keys, CLI shard selection).s.escape_debug()fixes all five arms and makes the "wording carried over verbatim" claim true for every input. Details and repro inline. -
Vec<String>extraction dominates, and it answers your item (1) (lib.rs:1155). Measured at N=200k: ~84% of the<U32array path isnp.str_boxing plus theStringcopy under the GIL, beforeallow_threadsruns. So keep it serial —par_iteris Amdahl-capped near 1.2x — but note the<U32array path is 2x slower than a plain list (52.3 ms vs 25.0 ms at order 29), and<U32is exactly whatto_decimal()emits and what phase 2's round-trip uses. Reading the fixed-width UCS-4 buffer directly (symmetric with the emit side'sPyReadonlyArray1) is a bigger win than any parallelism. Flagging now because phase 2 freezes the calling convention. -
Dead field:
ParseError::PointSuffixOrder'susizeis constructed twice, discarded inDisplay, never read (:750). It forces the test at:1787to recompute the parser's own arithmetic to satisfyassert_eq!. Drop it, or render it — the message never says what order was actually seen. -
Four test gaps (
:1746), all currently correct behavior, so regression pins rather than bugs: uppercase"P"; theOrderTooDeep-before-PointSuffixOrdercheck ordering; a trailing multi-byte char; and"-0"/ signed leading zeros. Also,from_decimal_repr_round_trips_every_base_and_orderdraws one sample per (base, order) — 360 paths — which is less than the "whole domain" the PR body claims.
Convention notes
- CLAUDE.md §4, ~1000-line module limit.
src_rust/src/decimal_morton.rsgoes 1577 → 1815 lines. The overage is pre-existing (mainis already over ondecimal_morton.rs,lib.rsat 1218, andcoverage.rsat 1205) and the non-test source here is only 836 lines, so this is not phase 1's doing. But §4 says to raise it rather than keep growing silently, and three more phases are queued — worth an explicit ruling on whether the limit counts#[cfg(test)]bodies before phase 4 lands. - The
split_children_rusthunk inlib.rsis a formatting-only reflow unrelated to issue #114. Harmless, but it is diff noise in an otherwise tightly scoped PR. - No new dependencies (§4 satisfied). Style matches the surrounding module — the doc-block-above-a-section-banner pattern, the
spec section N/issue #Ncitations, and theResult-not-panic contract are all consistent with the existing kernel code.
Nothing here blocks advancing to the next phase; item (1) is the only one I would want fixed before this leaves draft, and item (2) is the one worth deciding before phase 2 freezes the signature.
Generated by Claude Code
| impl std::fmt::Display for ParseError { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| match self { | ||
| ParseError::Malformed(s) => write!(f, "malformed decimal Morton id '{}'", s), |
There was a problem hiding this comment.
🤖 from Claude (review)
This is the one confirmed behavioral divergence from _decimal_to_word, and it is wider than "Questions for review" item (3) describes.
I ran a differential fuzz of from_decimal_repr against the origin/main _decimal_to_word (mortie/morton_index.py:35, extracted standalone so it still exercises the old Python path): 380,720 inputs — exhaustive over all 12 base cells x orders 0–29 x both kinds, plus random strings over -0123456789p +.²٣x\t\n'"\ and structured near-miss ids up to order 31.
Result: 0 value mismatches, 0 exception-type mismatches. The parse is correct. The only divergence class, hit 153,242 times, is this line and its four siblings.
{s!r} in Python escapes non-printables; '{}' here does not. So a malformed id containing a control character now injects it raw into the ValueError:
id = '\t9'
py msg: "malformed decimal Morton id '\\t9'"
rust msg: "malformed decimal Morton id '<literal TAB>9'"
id = ' 5\n466'
py msg: "malformed decimal Morton id ' 5\\n466'"
rust msg: "malformed decimal Morton id ' 5<literal LF>466'" # now a multi-line exception
Backslashes likewise stop being doubled. This matters more than the quote case you flagged: malformed ids arrive from untrusted channels here — zarr leaf paths, status-channel keys, CLI shard selection — so an embedded newline turns one log line into several, and a caller doing except ValueError as e: log.warning(e) gets a multi-line record.
Cheap fix that also restores Python's exact quoting (single quotes, escaped payload):
ParseError::Malformed(s) => {
write!(f, "malformed decimal Morton id '{}'", s.escape_debug())
}applied to all five arms. The PR body says error wording is "carried over verbatim from the Python original so the user-visible ValueError text does not change" — with escape_debug that claim becomes true for every input, not just printable ASCII.
Generated by Claude Code
| /// More order digits than [`MAX_ORDER`]. | ||
| OrderTooDeep(String, usize), | ||
| /// A terminal `p` on anything but a full order-[`MAX_ORDER`] id. | ||
| PointSuffixOrder(String, usize), |
There was a problem hiding this comment.
🤖 from Claude (review)
The usize payload on this variant is dead. Every reference in the tree:
750: PointSuffixOrder(String, usize), // decl
769: ParseError::PointSuffixOrder(s, _) => write!( // discarded
816: return Err(ParseError::PointSuffixOrder(s.to_string(), order));// constructed
1793: Err(ParseError::PointSuffixOrder(bad.to_string(), order)) // test only
Display binds it to _, and the only consumer of the whole enum is e.to_string() at src_rust/src/lib.rs:1159 — so the order never reaches a human. Contrast OrderTooDeep, whose usize is rendered.
The cost shows up in the test at :1787, which has to reverse-engineer the value purely to satisfy assert_eq!:
let order = bad.len() - 1 - usize::from(bad.starts_with('-')) - 1;That line encodes the parser's own arithmetic into the test, so it can't independently confirm anything — and it is wrong for any id with a multi-byte char (len() is bytes), which happens to be unreachable only because such ids fail as Malformed first.
Suggest dropping the field to a plain PointSuffixOrder(String). The test then just asserts the variant, and CLAUDE.md §4's "no dead code" applies. If you'd rather keep the order for future callers, render it — the message currently says "legal only on full order-29 point ids" without ever saying what order was actually seen, which is the more useful message anyway.
Generated by Claude Code
| /// short string scan, and a serial fold keeps the reported error deterministic | ||
| /// (the *first* bad id, not whichever thread failed first). | ||
| #[pyfunction] | ||
| fn rust_mi_from_decimal(py: Python<'_>, decimals: Vec<String>) -> PyResult<PyObject> { |
There was a problem hiding this comment.
🤖 from Claude (review)
Measured answer to "Questions for review" item (1): keep it serial — but not for the reason given, and Vec<String> is the thing actually costing you.
Vec<String> extraction allocates and copies every id into Rust memory while holding the GIL, before py.allow_threads on the next line ever runs. So allow_threads is wrapping the cheap part.
I measured this — 200,000 ids, release build, this binding, varying only the id length so the per-element fixed cost separates from the per-character parse cost:
| input | order 0 (1 char) | order 10 | order 29 (30 chars) |
|---|---|---|---|
Python list |
12.1 ms | 16.4 ms | 25.0 ms |
numpy <U32 array |
44.0 ms | 47.9 ms | 52.3 ms |
The order-0 column is almost pure extraction overhead — there is nothing to parse. So for the <U32 array at order 29, ~84% of the time (44 of 52.3 ms) is np.str_ boxing plus the String copy under the GIL, and only ~16% is the parse that allow_threads releases for.
Two consequences:
-
par_iteris not worth it. Amdahl caps it at about 1.2x on the array path even with infinite cores. Your determinism argument stands and now has a throughput argument behind it — worth folding into the doc comment so the next reader doesn't re-litigate it. -
The
<U32array path is the slow one, and it is the path phase 2 actually uses.to_decimal()returns"<U32"(mortie/morton_index.py:563), so the natural round-tripdecimals_to_words(arr.to_decimal())— exactly whatmortie/tests/test_decimal_parse.py:140exercises — is 2x slower than passing a plain Python list (52.3 ms vs 25.0 ms). That is backwards from what a caller would expect, and this binding's whole justification is per-shard hot-path throughput.
For symmetry with the emit side (rust_mi_decimal_repr, :1121, which takes PyReadonlyArray1<u64>), consider accepting the fixed-width UCS-4 buffer directly instead of Vec<String>: a <U32 array is a contiguous 32 * 4-byte-per-element block, so the elements can be read without constructing a Python object per id. That attacks the 84%, and it is a strictly larger win than any parallelism.
For reference, versus the old Python loop over _decimal_to_word at order 29 and N=200k: 1299 ms → 25.0 ms (list, 40x) / 52.3 ms (array, 21x). Either way this is a large win — the point is only that the array form leaves half of it on the table.
Not a blocker for phase 1; flagging now because phase 2 freezes the calling convention.
Generated by Claude Code
| let point = s.ends_with('p'); | ||
| let text = if point { &s[..s.len() - 1] } else { s }; |
There was a problem hiding this comment.
🤖 from Claude (review)
I went after &s[..s.len() - 1] as a panic on a multi-byte final char and it is sound — recording the argument since nothing in the code says why, and byte-slicing a &str is the kind of line a later refactor breaks silently.
It holds because UTF-8 continuation bytes are all >= 0x80, so no multi-byte character can end in the byte 0x70. ends_with('p') therefore implies the final byte is a standalone ASCII 'p' and s.len() - 1 is always a char boundary. &text[1..] on line 799 is safe for the same reason ('-' is single-byte). Confirmed empirically: "3é", "3😀", "3ṕ", and lone surrogates all take the non-p branch and fall out as Malformed (surrogates raise UnicodeEncodeError at the PyO3 boundary before Rust is reached — identical to the Python original, which also parses through the extension).
Two suggestions:
- Add a one-line comment here stating the invariant (
ends_with('p')implies an ASCII final byte, so the slice is on a char boundary). Right now the doc block above justifies the grammar in detail but not the one line that could actually panic. - Pin it.
strip_suffix('p')expresses the same thing without a manual index and cannot go out of bounds:
let (text, point) = match s.strip_suffix('p') {
Some(t) => (t, true),
None => (s, false),
};The existing tests never feed a trailing multi-byte char — from_decimal_repr_rejects_malformed at :1746 has "3\u{00b2}1", but the ² is in the middle, so the boundary case is untested. Worth adding "3é" / "3😀" to that table whichever way you go.
Generated by Claude Code
| fn from_decimal_repr_rejects_malformed() { | ||
| // Body is not a run of ASCII digits (empty, bare sign, bare/doubled | ||
| // suffix, non-digit, and a unicode numeric that Python's `isdigit` | ||
| // would otherwise accept). | ||
| for bad in ["", "-", "p", "-p", "31111pp", "x123", "3\u{00b2}1"] { | ||
| assert_eq!( | ||
| from_decimal_repr(bad), | ||
| Err(ParseError::Malformed(bad.to_string())), | ||
| "expected malformed for {bad:?}" | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🤖 from Claude (review)
The six new tests are genuinely domain-covering rather than vector-picking, and I confirmed they pass (cargo test: 205 passed, 0 failed, 1 ignored; cargo fmt --check clean). Four gaps, all of which I verified currently behave correctly — so these are regression pins, not bug reports:
-
Uppercase
"P"."3…1P"falls through toMalformedbecausePis not an ASCII digit. That is right (spec §2 grammar sayskind-suffix = "p"), and it matches the old Python, but it is load-bearing and accidental-looking: it works only because the suffix check is case-sensitive and the digit check rejects the leftover letter. One line in this table locks in that"P"is not a kind suffix. -
Check ordering when
pmeets an over-deep id."3" + "1"*30 + "p"yieldsOrderTooDeep(30), notPointSuffixOrder— because the order check at:812runs before the suffix check at:815. Matches Python. Nothing pins the ordering, so swapping those twoifs would be a silent user-visible message change. -
Trailing multi-byte char — see my note on
:796."3\u{00b2}1"puts the non-ASCII char in the middle, so the char-boundary case the byte-slice depends on is not exercised. -
"-0"and leading zeros."-0"→BaseDigit(0),"01"/"-01"→BaseDigit(0).from_decimal_repr_rejects_out_of_range_componentsat:1760covers"0123"but not the signed form, and the sign column is the part with thelead + 5/lead - 1fold.
Separately, from_decimal_repr_round_trips_every_base_and_order at :1701 uses one sample_tuples draw per (base, order) — 360 paths total. That is a reasonable smoke sweep but it is not the "whole domain" the PR body claims. If you want the stronger property cheaply, to_decimal_repr(w) -> from_decimal_repr -> w over a seeded sweep of a few thousand random words per order would actually earn that wording.
For what it's worth, I ran the equivalent externally against the origin/main Python parser — all 12 bases x orders 0–29 x both kinds, plus ~380k fuzz inputs — and found zero value or exception-type divergences, so I have no reason to think any of the above is hiding a real defect.
Generated by Claude Code
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of phases 2–4 (f93a6f5..e626966) — Python and docs only; the Rust kernel is reviewed separately. Everything below was executed against the branch build, not read off the diff.
No blocking defects. The shape matches what was agreed on the issue: public scalar with the three-way dtype flag, vectorized decimals_to_words, the pandas classmethod, the retained private alias, and the order-29 non-injectivity documented and pinned. Findings are one correctness wart, one weak test, and some polish.
Verified sound (things I tried hard to break and could not)
_decimal_to_wordstring equivalence. Differential harness, old pure-Python body vs new delegation, 107 string inputs — valid ids at every order and both signs,p-marked forms, and malformed cases including unicode digits (٣123,３123,²), embedded NULs, whitespace,--3123, over-order-29. Zero differences in return value or exception type.- No
ValueErrorleak in the dtype guard. The bareexcept TypeErrorwas my main suspicion, since a strayValueErrorwould be indistinguishable from a malformed-id error. ~60 values throughnp.dtype()(None,"","uint65","bad,spec",[("a",)],1.5,{},"V1","m8[s]","a10", …) — every failure was aTypeError. Clean. - "First malformed id, in input order" holds. The crate uses rayon, so I checked determinism with bad ids at indices 3 / n/2 / n-1 at n = 10; 1,000; 100,000; 1,000,000, 15 runs each: always the earliest, every time.
- The pandas-blocked subprocess test is load-bearing and not vacuous. Plain
import mortiedoes import pandas here, so themeta_pathblocker does real work;assert out.returncode == 0means an import failure fails the test rather than passing silently. Also confirmed the parse surface works with pandas and pyarrow both blocked. - Docs are runnable and accurate. I executed every new code block verbatim, chained onto the
arrdefined earlier in the page.mortie.morton_index.MortonIndexScalarresolves; thefrom_decimal(arr.to_decimal())._data == arr._dataassert passes; the<U32claim is right (the widest possible id,-6+ 29 digits +p, is exactly 32 chars — no truncation); the "all three raiseValueErrornaming the first malformed id" claim holds for all three entry points; thep-marked point round-trip works end to end. from_decimalvalidates. 2-D input correctly rejected viacls(), consistent withfrom_words; empty input constructs a valid length-0 array.- Performance genuinely improves — no regression hiding behind the Rust move. Scalar
-31123: 2.38 µs → 0.97 µs; order-29 id: 5.31 µs → 1.05 µs; 100k ids vectorized: 535 ms → 40.5 ms. - Hygiene.
flake8 --select=E9,F63,F7,F82clean; the non-blocking--max-line-length=88pass clean on all three Python files. Full suite 743 passed, 11 skipped.morton_index.pyis 743 lines, inside the §4 ~1000-line limit. No new dependencies.
Findings
decimals_to_words'size == 0early return makes the type guard data-dependent — the one I'd actually fix. The branch keys on size, not on dtype provenance, so any wrong dtype is accepted when empty:np.array([], dtype=np.int64)andnp.zeros((0,3), dtype=complex)both returnuint64arrays, whilenp.array([1, 2])raises. Fails only once there is data. Inline with a two-line fix._decimal_to_wordnon-strinputs changed exception type —AttributeError→TypeErrorforint/None/float/list/np.uint64/ndarray. The new error is better; the point is that it is a behavior change on the name retained for downstream compatibility, and it is neither documented nor tested. Two error-message changes ride along (np.str_rendering, control-char escaping).test_uint64_spellings_acceptedis type-blind — it compares values, so it would pass against an implementation returning a lossynp.float64(np.float64(ref) == refisTruewhileint(np.float64(ref)) != int(ref)). It is meant to pin a return shape.decimals_to_wordssilently accepts a bare string, returning a 0-d array; and non-string elements of object arrays surface a raw PyO3 message (cannot be converted to 'PyString') with no function, element, or index — jarring next to the carefully worded guard immediately above.- dtype dispatch looseness — instances are accepted where types are meant (
dtype=np.uint64(0)succeeds), andMortonIndexScalarsubclasses silently downgrade to plainuint64with no diagnostic, because theischeck misses them andnp.dtype(subclass)then matches. from_hive_pathwas not moved to the new vectorized parse (line 671 still loops the now-deprecated_decimal_to_word), and its docstring cross-references the deprecated private name rather than the new public one. Outside the diff, so possibly deliberate.- The
__init__.pycomment mischaracterizes its own change —morton_indexis already eagerly imported three lines above, andimport mortiedoes pull in pandas and pyarrow when installed. The true and useful claim is that the two functions work without pandas, which I confirmed.
Unpinned dtype cases worth a test either way: None (currently rejected, though "None means default" is a common convention), bool, np.uint32.
Generated by Claude Code
| def _decimal_to_word(s): | ||
| """Deprecated private alias for :func:`decimal_to_word` (issue #114). | ||
|
|
||
| Kept through a deprecation cycle because downstream code (zagg's parse | ||
| boundary) imports this name; returns a Python ``int`` exactly as it | ||
| always has. New code should use the public :func:`decimal_to_word`. | ||
| """ | ||
| return decimal_to_word(s, dtype=int) |
There was a problem hiding this comment.
🤖 from Claude (review)
Back-compat: the exception type changed for non-str inputs, and nothing pins it.
I ran a differential harness (old pure-Python body from f93a6f5 vs the new delegation) over 107 string inputs — valid ids at every order, both signs, p-marked, plus malformed forms including unicode digits (٣123, ３123, ²), embedded NULs, whitespace, --3123, over-order-29. Zero behavioral differences: identical return values and identical exception types. The string contract is genuinely preserved, which is the important half.
The non-str half is not:
| input | old | new |
|---|---|---|
3123 (int) |
AttributeError: 'int' object has no attribute 'endswith' |
TypeError: argument 'decimals': 'int' object cannot be converted to 'PyString' |
None |
AttributeError |
TypeError |
3.0 |
AttributeError |
TypeError |
["3123"] |
AttributeError |
TypeError |
np.uint64(5) |
AttributeError |
TypeError |
np.array("3123") |
AttributeError |
TypeError |
bytes stays TypeError (different message). TypeError is the better error, so I'm not arguing for the old behavior — but the docstring says "returns a Python int exactly as it always has", which is a claim about the return type only, while the retained-alias rationale is downstream compatibility. Anything downstream doing except AttributeError around this parse boundary silently changes behavior.
Two message changes also ride along, in case zagg matches on text: np.str_ inputs used to render as decimal Morton id np.str_('0123') and now render '0123'; control characters used to be repr-escaped ('3123\\n') and now embed literally.
test_private_alias_still_works_and_returns_int (line 63) pins only the happy path and the return type. Suggest either pinning the non-str behavior you intend (pytest.raises(TypeError) for a couple of these) or noting the exception-type change in the docstring, so the deprecation cycle documents it rather than discovering it.
Generated by Claude Code
| if arr.size == 0: | ||
| # An empty list arrives as float64 (numpy's default for []), which the | ||
| # dtype guard below would reject; there is nothing to parse either way. | ||
| return np.empty(arr.shape, dtype=np.uint64) |
There was a problem hiding this comment.
🤖 from Claude (review)
The size == 0 early return makes the type guard data-dependent — it passes until the array has data.
The comment justifies the branch by the []→float64 case, but the branch is on size, not on dtype provenance, so it short-circuits the guard for any wrong dtype that happens to be empty:
>>> decimals_to_words(np.array([], dtype=np.int64))
array([], dtype=uint64) # accepted
>>> decimals_to_words(np.zeros((0, 3), dtype=complex))
array([], shape=(0, 3), dtype=uint64) # accepted, complex input
>>> decimals_to_words(np.array([1, 2]))
TypeError: decimals_to_words expects decimal Morton strings, got an array of dtype dtype('int64')The failure scenario is the ordinary one: a caller filters an integer array down to some key set and parses it. Empty batch → sails through and returns a uint64 array. First non-empty batch → TypeError. That is the bug class the guard exists to prevent, and it is hidden until there is data.
Narrowing the special case to what the comment actually describes keeps both properties:
arr = np.asarray(decimals)
if arr.dtype.kind not in ("U", "O"):
# numpy defaults [] to float64; an empty input has nothing to parse either way
if arr.size == 0:
return np.empty(arr.shape, dtype=np.uint64)
raise TypeError(...)test_empty_input (line 107) only covers [], so none of the above is caught.
Generated by Claude Code
| words = _rustie.rust_mi_from_decimal(arr.ravel().tolist()) | ||
| return words.reshape(arr.shape) |
There was a problem hiding this comment.
🤖 from Claude (review)
Two input classes reach here without the care the guard above shows.
(1) A bare string is accepted and returns a 0-d array. np.asarray("3123") is a 0-d <U4 array, kind "U", size 1 — so it passes the guard and round-trips through reshape(()):
>>> decimals_to_words("3123")
array(3566850904877432835, dtype=uint64) # 0-d, no errorThe singular/plural pair invites exactly this typo, and a 0-d array is not a useful thing to hand back from a vectorized API — it fails later and elsewhere (MortonIndexArray.from_decimal("3123") surfaces as ValueError: morton_index values must be 1-dimensional, which does not point at the real mistake). Worth either rejecting arr.ndim == 0 or documenting it. Untested either way.
(2) Object arrays leak the PyO3 message. The guard only screens the array's dtype, so a non-string element of an object array is caught by the binding:
>>> decimals_to_words(np.array(["3123", 5], dtype=object))
TypeError: argument 'decimals': 'int' object cannot be converted to 'PyString'
>>> decimals_to_words(np.array(["3123", None], dtype=object))
TypeError: argument 'decimals': 'NoneType' object cannot be converted to 'PyString'No mention of decimals_to_words, no element, no index — which reads oddly next to the hand-written message three lines up that goes out of its way to name the function and the dtype. test_object_array_of_strings (line 112) only covers the all-strings case. Object arrays with mixed content are the realistic way this dtype arises (a pandas column, a np.array(..., dtype=object) of parsed path parts), so it is worth a message on par with the neighbouring one.
Generated by Claude Code
| if dtype is int: | ||
| return word | ||
| if dtype is MortonIndexScalar: | ||
| return MortonIndexScalar(word) | ||
| try: | ||
| requested = np.dtype(dtype) | ||
| except TypeError: | ||
| requested = None | ||
| if requested == np.uint64: | ||
| return np.uint64(word) | ||
| raise TypeError( | ||
| f"decimal_to_word dtype must be np.uint64 (the default), int, or " | ||
| f"MortonIndexScalar; got {dtype!r}" | ||
| ) |
There was a problem hiding this comment.
🤖 from Claude (review)
The dtype guard is sound on the case I most expected to leak; two looseness cases remain.
First, the good news, since it's the obvious worry with a bare except TypeError: I tried ~60 values through np.dtype() looking for one that raises something else and escapes the guard as a spurious ValueError (which would be indistinguishable from a malformed-id error) — None, "", " ", "uint65", "bad,spec", "[('a','i8')]", [("a",)], 1, 1.5, {}, np.ndarray, "V1", "m8[s]", "T", "a10". Every failure was a TypeError. dtype=None correctly rejects (np.dtype(None) is float64), as do bool, np.uint32, "not-a-dtype". No leak found.
Two things do slip through:
(1) Instances are accepted where a type is meant.
>>> decimal_to_word("3123", dtype=np.uint64(0)) # a value, not a type
np.uint64(3566850904877432835) # accepted(2) MortonIndexScalar subclasses silently downgrade. The is check on line 104 misses subclasses, and the fallthrough then matches them because np.dtype(AnyUint64Subclass) is uint64:
>>> Sub = type("Sub", (MortonIndexScalar,), {})
>>> type(decimal_to_word("3123", dtype=Sub)).__name__
'uint64' # not Sub, and no errorSo a caller subclassing MortonIndexScalar to customise rendering gets a plain uint64 back with no diagnostic — the one failure mode where silence is worse than an error. isinstance(dtype, type) and issubclass(dtype, MortonIndexScalar) on line 104 would either honour it or make the rejection explicit.
Minor, same area: the == comparison makes accepted spellings platform-dependent — "<u8", "=u8", "|u8", "L", "Q", "uintp" are accepted here while ">u8" is rejected (byte order), and "uintp" would flip on a 32-bit build. Probably fine, but the docstring advertises only "uint64" / np.dtype("uint64"), so the real accepted set is wider and less stable than documented.
Generated by Claude Code
| @pytest.mark.parametrize("spelling", ["uint64", np.dtype("uint64")]) | ||
| def test_uint64_spellings_accepted(self, spelling): | ||
| assert decimal_to_word("3123", dtype=spelling) == decimal_to_word("3123") |
There was a problem hiding this comment.
🤖 from Claude (review)
This assert is type-blind, and the thing it is meant to pin is the type.
The test's purpose is that "uint64" / np.dtype("uint64") are accepted as spellings of the np.uint64 default — i.e. that they yield the same return shape. But == on the two results only compares values, and every candidate return shape satisfies it:
ref = decimal_to_word("3123")
int(ref) == ref # True
np.uint64(ref) == ref # True
MortonIndexScalar(ref) == ref # True
np.float64(ref) == ref # True <-- and this one is lossyThe float64 row is the sharp end: np.float64(ref) == ref is True while int(np.float64(ref)) != int(ref) — so this test would pass green against an implementation that silently returned a precision-destroyed word for these spellings. That is the exact defect a parse-surface test should catch.
test_default_is_numpy_uint64 (line 78) gets it right; mirroring it here would fix this:
got = decimal_to_word("3123", dtype=spelling)
assert isinstance(got, np.uint64)
assert int(got) == int(decimal_to_word("3123"))Generated by Claude Code
| @pytest.mark.parametrize("bad", [float, "float64", np.int64, object()]) | ||
| def test_unsupported_dtype_raises(self, bad): |
There was a problem hiding this comment.
🤖 from Claude (review)
The rejection set covers plausible-but-wrong dtypes; it misses the values a caller is most likely to actually pass.
dtype=None is the notable gap. It currently raises TypeError — correct, and I verified it is not an accident of np.dtype(None) being float64 rather than an error — but "None means take the default" is a widespread numpy-adjacent convention, so this is a real fork in the API's behavior and it is unpinned. Whichever way you want it, it should be a test.
Also worth adding, all confirmed against the current implementation:
boolandnp.uint32— near-miss integer dtypes, both correctly rejected, neither pinned.np.uint64(0)— an instance; currently succeeds (see my note on the dispatch). If that is intended, pin it; if not, this is the test that would have caught it.
Since TypeError here is the API's way of saying "bad dtype" and ValueError means "bad id", a test that the two never cross would also be cheap and worth having.
Generated by Claude Code
| # The decimal parse surface (issue #114) is the exception to the laziness | ||
| # above: it needs only numpy and the Rust extension -- no pandas -- so it is a | ||
| # plain eager export, keeping per-key parse paths free of a pandas import. |
There was a problem hiding this comment.
🤖 from Claude (review)
The export is right, but this comment describes something that isn't happening.
morton_index is already imported eagerly on line 133, three lines above (from . import arrow, morton_index). So this from .morton_index import ... is not "the exception to the laziness above" and does not add an eager import — the module object already exists; this only binds two names off it. The laziness in the block above applies solely to MortonIndexDtype / MortonIndexArray, which are built on demand through __getattr__ (line 152), and to the arrow names.
Measured, so the "keeping per-key parse paths free of a pandas import" clause doesn't read as stronger than it is:
>>> import sys, mortie
>>> "pandas" in sys.modules, "pyarrow" in sys.modules
(True, True)import mortie pulls in both when they are installed. What is actually true — and it is the property that matters — is that decimal_to_word / decimals_to_words work when pandas is absent, which I confirmed with pandas and pyarrow both blocked at meta_path. Suggest re-wording along the lines of: these two need only numpy and the Rust extension, so they are bound eagerly off the already-imported module and stay callable on a numpy-only install. Per §6's "ground every claim", the current phrasing invites a reader to believe import mortie avoids pandas.
Generated by Claude Code
| @classmethod | ||
| def from_decimal(cls, decimals): | ||
| """Parse decimal Morton strings into an array (issue #114). | ||
|
|
||
| The inverse of :meth:`to_decimal`, and sugar over the numpy-only | ||
| :func:`decimals_to_words` for pandas users -- ``to_decimal()`` | ||
| output round-trips straight back through it. An unmarked order-29 | ||
| id yields the AREA word; only a ``p``-marked one yields the POINT | ||
| word (spec section 4), so point-ness does not survive a round-trip | ||
| through an unmarked string. Raises ``ValueError`` naming the first | ||
| malformed id. | ||
| """ | ||
| return cls(decimals_to_words(decimals)) |
There was a problem hiding this comment.
🤖 from Claude (review)
from_decimal itself checks out — I verified cls(...) validates, so a 2-D input is correctly rejected (ValueError: morton_index values must be 1-dimensional), matching from_words, and the empty case constructs a valid length-0 array.
The gap is next door: the PR adds a vectorized parse but leaves the one in-tree consumer of that parse on the per-element deprecated alias. from_hive_path (line 671) still loops:
word = _decimal_to_word(dec)one call per path, through the alias this PR just marked deprecated. The new test_hive_path_leaves_round_trip (test file line 175) asserts these two are the same parse, so the vectorized path is directly applicable — collect the dec values in the existing loop, then a single decimals_to_words after it. Measured on this branch, 100k ids: 40.5 ms vectorized vs 535.5 ms looped, so it is the same ~13x the PR is claiming elsewhere.
Related and smaller: the from_hive_path docstring at line 645 cross-references :func:`_decimal_to_word` for the order-29 area rule. Now that a public function exists and that name is deprecated, the reference should point at decimal_to_word — otherwise the public docs send readers to a private name the same PR is deprecating.
Both are outside the literal diff, so entirely your call whether they belong in this PR or a follow-up.
Generated by Claude Code
|
🤖 from Claude Both adversarial self-reviews are in and folded, across two commits ( Acted onRound 1 (the Rust kernel)
Round 2 (the Python surface)
Not acted on — one item, raised for youThe
Extraction copies every id into Rust memory while holding the GIL, before
State
All four phases complete. Leaving Generated by Claude Code |
2 participants
Closes #114
Implements the shape you signed off on in this comment: public scalar
decimal_to_wordreturningnp.uint64with an optionaldtypeflag for all three return shapes, a vectorized Rustdecimals_to_words, aMortonIndexArray.from_decimalclassmethod for pandas users, the private_decimal_to_wordalias kept for a later deprecation cycle, and the order-29 point/area non-injectivity documented on the parse side.The gap this closes
On
mainthe emit direction is fully public —decimal_repr(),to_decimal(),hive_path()— while the parse direction was the private Python scalar_decimal_to_word(mortie/morton_index.py:35), not in any__all__. zagg's parse boundary reaches into that private name, so an internal rename breaks it silently. There was also no vectorized parse path anywhere:_decimal_to_wordbuilt a 1-element array per call and unwrapped[0], so per-shard key parsing was a Python loop.Phases — all four complete
decimal_morton::from_decimal_repr+ the vectorizedrust_mi_from_decimalbinding.decimal_to_word(s, dtype=...),decimals_to_words(arr), exported frommortie;_decimal_to_wordretained as a private alias.MortonIndexArray.from_decimal, the inverse of.to_decimal().Phase 1 — the Rust kernel
pub fn from_decimal_repr(s: &str) -> Result<u64, ParseError>insrc_rust/src/decimal_morton.rs, the exact inverse ofto_decimal_reprand now the single implementation of the grammar fromdocs/specification.md:93-98:A single left-to-right scan (the order is the digit count, so no lookahead), returning a typed
ParseErroron every malformed id rather than panicking — the base-digit range guarantees a base cell of0..=11, so thefrom_nested/from_nested_pointasserts are unreachable from here. Error wording is carried over verbatim from the Python original, so the user-visibleValueErrortext does not change.rust_mi_from_decimalmirrors the emit-siderust_mi_decimal_repr: list ofstrin,uint64numpy array out, underpy.allow_threads. Deliberately serial, like the emit side — the per-id work is a short string scan, and a serial fold makes the reported error deterministic (the first bad id in input order, not whichever thread failed first).Phase 2 — the public numpy-only surface
Both are plain eager exports from
mortie/__init__.py(they need only numpy and the Rust extension), so a per-key parse path never touches pandas._decimal_to_wordbecomes a three-line alias returning a Pythonintexactly as before, so nothing in-tree or in zagg changes behavior.Two things worth calling out, both found by writing the tests:
decimals_to_wordsrefuses non-string input rather than coercing.np.asarray([1, 2], dtype=str)would silently turn the integer1into the valid order-0 id"1". A parse surface must not invent input, so the dtype kind is checked (U/Oonly) and anything else raisesTypeError.sys.modulescheck.import mortiedoes import pandas when pandas is installed — deliberately, via the eager_build_classes()dtype-registration probe inmorton_index.py. So the test runs a fresh interpreter with ameta_pathblocker that makespandasunimportable, then importsmortieand parses. That actually pins the property zagg cares about.Phase 3 — the pandas classmethod
MortonIndexArray.from_decimal(decimals), sugar overdecimals_to_words, mirroring.to_decimal(). Tested for round-trip againstto_decimal(), agreement withfrom_hive_pathon the same leaf ids, and the malformed-id error.Phase 4 — the doc rider
A
#### Parse-side API (normative surface)block indocs/specification.md§4 (between<!-- parse:api:begin/end -->markers) tabling the three entry points and stating the caveat in parse-side terms, plus a "Parsing decimal ids back" section indocs/morton_index_datatype.mdwith runnable examples.The caveat, stated the way a parse caller needs it: an unmarked order-29 id parses to the area word, so a point word does not round-trip through an unmarked string. Emit renders point words
p-marked, soword → to_decimal → from_decimalis the identity end to end — but any channel that strips the marker (a path component above all, which never carries it) returns the area word for what may have been a point. Orders 0–28 are unambiguous.Per the acceptance criterion this is pinned by tests, not just prose:
TestPointAreaNonInjectivityasserts the bit-level tie-break (same prefix+body, area suffix< 48vs point suffix>= 48), andTestSpecPageParseSectionis a drift pin asserting the page names every public entry point, that those entry points exist and are callable, and that the prose rule and the actual behavior agree.How it was tested
cargo test: 205 passed, 0 failed, 1 ignored (6 new).cargo fmtclean.cargo clippy --all-targets: 46 warnings vs 45 onmain— the one new warning is the macro-generateduseless_conversionthat fires on every#[pyfunction](the known false positive tracked as Consolidated follow-ups from the June/July sweeps (#73, #88, #97) #108 Group A item 2, suppressed on PR Group A small-fix bundle from issue #108 #111). No other new lint.maturin develop --release+ fullpytest -v: 743 passed, 11 skipped (34 new, inmortie/tests/test_decimal_parse.py), up from 709 onmain.flake8 mortie --select=E9,F63,F7,F82: clean. The non-blocking--max-line-length=88pass reports nothing in any file this PR touches.(nested, depth)bridge so the two encode paths cannot drift.Questions for review
par_iterand accept a nondeterministic which-id-failed message.dtypeflag domain. Your ask was "any of the three, defaultnp.uint64". Implemented asnp.uint64(default),int, andMortonIndexScalar, with"uint64"/np.dtype("uint64")accepted as spellings of the default and anything else raisingTypeError. Reasonable, or do you want it strictly the three literals?_decimal_to_wordon non-strinput. The old pure-Python parser raisedAttributeErrorfor e.g.Noneor anint(it called.endswith); routed through the Rust binding it now raisesTypeError. Better error, but technically a changed exception type on a name zagg imports. Fine, or should the alias keep the old shape until the deprecation cycle ends?'id'where the Python original used Python'srepr(identical for ordinary ascii ids, differing only for an id containing a quote). Non-issue in practice; flagging since it is technically user-visible text.Closes #114outright, or stayRefsuntil you've confirmed zagg has moved off the private name?