Decimal-string display & casting layer for morton_index#105
Conversation
Merging this PR will not alter performance
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | fast_norm2mort_orders[6] |
269.4 ns | 327.8 ns | -17.8% |
| ❌ | fast_norm2mort_orders[10] |
315.3 ns | 373.6 ns | -15.61% |
| ⚡ | coverage_triangle[4] |
130.1 µs | 113.4 µs | +14.81% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/104-decimal-display-casting (41b3bac) with main (4fb4cd2)2
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. ↩
-
No successful run was found on
main(5b92649) during the generation of this report, so 4fb4cd2 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #105 +/- ##
==========================================
+ Coverage 93.73% 94.14% +0.40%
==========================================
Files 9 9
Lines 1230 1299 +69
==========================================
+ Hits 1153 1223 +70
+ Misses 77 76 -1
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
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of phase 1 (commit 2b495d9: element display = decimal string, MortonIndexScalar wrapper). Phases 2 and 3 landed on the branch while this review was in flight and are not covered here — they need their own pass.
Issue #104 scope items 1 and 3: met. _word_repr/_formatter/__repr__ render the decode-through-kernel decimal string, the len=N, order=K header stays, and element access and iteration return MortonIndexScalar with str/repr = decimal string and int() = packed word (all verified by running the branch).
Verified safe (claims I attacked and could not break):
- The
except ValueErrorcatch in__str__is total:rust_mi_decimal_reprmaps every empty/invalid word toPyValueErrorviaOption(src_rust/src/lib.rsL1111-1127) — no panic path, so "a repr must never raise" holds._word_repr(0)still returns<NA>(pre-existing test passes). - Arithmetic on the scalar degrades to plain
np.uint64(s + 1,-s,s * 2all verified), so derived words never carry a misleading decimal repr. - pandas paths: Series repr renders decimal strings with
<NA>for missing;to_csvemits the decimal string (empty for NA rows);astype(str)gives decimal strings — consistent with the issue #48 always-strings interchange convention. - Per-element FFI in
_word_repris bounded: at most 6 calls in__repr__; measured ~1.6 us/element (about 5x the vectorizedto_decimal()path), fine for display. - Conventions: flake8 blocking selectors and the 88-col style pass are clean, module was 516 lines at phase 1, no new dependencies, docstrings match the surrounding freeform style, tests ship in the same commit, and all 21 relevant tests pass locally.
Findings, ranked (inline comments):
- [low] pickle drops the subclass — a shard key round-tripped through multiprocessing/dask reverts to raw-word display;
__reduce__fixes it. - [low]
__format__has no direct test (neither the working>10spec nor the documentedValueErroron numeric specs), and"%d" % ssilently emits the raw word — pin whichever numeric-spec answer is chosen. - [note, pre-existing] the boxed sentinel scalar prints
<NA>butpd.isna()on it isFalse; flagged for a recorded decision, not a change.
Nothing blocking the next phase.
Generated by Claude Code
| def __str__(self): | ||
| word = int(self) | ||
| if word == 0: | ||
| return "<NA>" | ||
| try: | ||
| return _rustie.rust_mi_decimal_repr( | ||
| np.asarray([word], dtype=np.uint64) | ||
| )[0] | ||
| except ValueError: | ||
| return f"<invalid {word:#018x}>" | ||
|
|
||
| __repr__ = __str__ |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] Pickle drops the wrapper: a MortonIndexScalar comes back as a plain np.uint64 and prints the raw packed word.
Verified on this branch:
>>> s = a[0] # MortonIndexScalar, str(s) == "-31123"
>>> p = pickle.loads(pickle.dumps(s))
>>> type(p).__name__, str(p)
('uint64', '10403315139225845764')numpy scalars reduce to multiarray.scalar(dtype, bytes), which rebuilds the base type (copy.copy/deepcopy do preserve the subclass — it is only pickle). So a shard key crossing a process boundary (multiprocessing/dask, i.e. exactly the zagg-style consumers this display layer targets per issue #104) silently reverts to raw-word display. A two-line __reduce__ returning (MortonIndexScalar, (int(self),)) restores it; worth a pinning test either way.
Generated by Claude Code
| def __format__(self, spec): | ||
| # numpy's numeric __format__ would print the packed word; the display | ||
| # form of a morton_index is its decimal string, so f"{shard_key}" (and | ||
| # any string spec, e.g. ">10") formats that instead. int(self) remains | ||
| # the escape hatch to format the raw word numerically. | ||
| return format(str(self), spec) |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] __format__ is a behavioral change with no direct test.
Nothing in the suite pins a nonempty format spec. Verified behavior on this branch:
f"{s:>10}"→' -31123'(works as the docstring claims)f"{s:d}"→ValueError: Unknown format code 'd' for object of type 'str'(the behavior the PR body flags as an open question)
Since the numeric-spec question is posed under "Questions for review", a test pinning whichever answer is chosen belongs in this PR (CLAUDE.md §4: every behavioral change needs tests in the same PR) — right now only the empty spec is exercised via f"{s}".
Also verified but undocumented: old-style %-formatting bypasses __format__ entirely — "%d" % s emits the raw word 10403315139225845764 while f"{s}" gives -31123. Worth one docstring line so that asymmetry reads as a documented choice rather than an oversight.
Generated by Claude Code
| result = self._data[item] | ||
| if np.isscalar(result) or isinstance(result, np.integer): | ||
| return np.uint64(result) | ||
| return MortonIndexScalar(result) |
There was a problem hiding this comment.
🤖 from Claude (review)
[note, pre-existing] The boxed missing element now prints as NA without being NA.
__getitem__ on a sentinel row returns MortonIndexScalar(0) rather than dtype.na_value (pd.NA). Verified:
>>> s2 # Series with one missing entry
0 -31123
1 <NA>
dtype: morton_index
>>> pd.isna(s2.iloc[1])
FalseThe mismatch predates this diff (it returned np.uint64(0) before), but the new <NA> repr makes the scalar look like a missing marker that pd.isna / NA-propagation will not honor. The strict ExtensionArray contract is that __getitem__ returns self.dtype.na_value for missing entries. If sentinel-boxing is deliberate (likely, given the all-zero-word design), fine — flagging so it is a recorded decision rather than an accident; out of scope to change in this phase.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Recorded as deliberate, not changed in this PR: the all-zero sentinel is the array's storage-level missing value, and boxing it as MortonIndexScalar(0) keeps __getitem__ total over the raw words (the strict dtype.na_value return is a pre-existing deviation, as noted). The <NA> rendering matches the array formatter's treatment of the same word, so display is at least consistent; making __getitem__ return pd.NA is an ExtensionArray-contract change that belongs to its own decision, standing for review here.
Generated by Claude Code
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial review of phases 2–4 (commits c0c90fd..d0ec61a): to_decimal, to_legacy_i64, hive_path/from_hive_path/_decimal_to_word, and the string-layer golden pins. All claims below were verified by running the code at this head.
Findings (3 inline):
- Medium —
from_hive_path's directory cross-check raises a falseValueErrorfor a bare leaf under a root whenever the root has ≥ order+1 components ("s3://bucket/-3.zarr"fails; order-0 ids fail under any root), contradicting the docstring's "one under an arbitrary root skips the check". The loose direction (partial contradicting digit dirs accepted) is flagged in the PR body, but this false-reject direction is not. - Low — a single
pathlib.Pathargument tofrom_hive_pathraisesTypeError: 'PosixPath' object is not iterable. - Low / non-blocking —
from_hive_pathmakes one single-element Rust call per path (~0.54 s / 100k vs ~0.08 s emit); recorded for the file.
Probed and confirmed correct: _decimal_to_word rejects Arabic-Indic/fullwidth/superscript digits, U+2212 minus, +-signed, and whitespace-padded ids (the isdigit() and isascii() pair is sufficient); to_legacy_i64's order-18 cap is exact and never data-dependent (southern all-4 order-18 -6444444444444444444 fits int64 and inverts bit-exactly through from_legacy, while the smallest order-19 id 1.11e19 already overflows); the <U31 width claim holds (northern order-29 renders 30 chars, southern 31, kernel-bounded so numpy's silent U31 truncation is unreachable); sentinel words raise through decimal_repr before any emit path returns; the order-29 point→area collapse through hive round-trip is documented and pinned by test_golden_repr_not_injective_point_vs_area; pandas Series.array round-trips all three new methods.
Conventions: mortie/morton_index.py is 644 lines (under the ~1000 cap), no new dependencies, both flake8 gates clean, all 31 tests in the touched suites pass locally, commit style matches §3.
Generated by Claude Code
| head = 2 if dec.startswith("-") else 1 | ||
| levels = [dec[:head], *dec[head:]] | ||
| if len(parts) - 1 >= len(levels): | ||
| got = parts[-1 - len(levels):-1] | ||
| if got != levels: | ||
| raise ValueError( | ||
| f"hive path {p!r} directories {'/'.join(got)!r} " | ||
| f"do not match leaf id {dec!r}" | ||
| ) |
There was a problem hiding this comment.
🤖 from Claude (review)
Medium — the directory cross-check false-positives on exactly the case the docstring promises to accept. The docstring says a bare {full_id}.zarr "or one under an arbitrary root" skips the check, but the gate is component-count-based (len(parts) - 1 >= len(levels)), so any bare leaf whose root has at least order + 1 components is misread as a full digit chain and raises. Verified against this commit:
MIA.from_hive_path("-31123.zarr") # ok (documented)
MIA.from_hive_path("s3://bucket/-3.zarr") # ValueError: directories 'bucket' do not match leaf id '-3'
MIA.from_hive_path("x/-3.zarr") # ValueError: directories 'x' do not match leaf id '-3'
MIA.from_hive_path("s3://bucket/data/-31123.zarr") # ok (root shorter than chain, check skipped)Order-0 ids are the worst case: any root at all trips it. The flip side of the same heuristic is that partially-contradicting digit dirs pass silently (MIA.from_hive_path("9/9/-31123.zarr") parses fine), which the PR body flags — but the false-reject direction isn't flagged anywhere and contradicts the docstring as written.
Root components are inherently indistinguishable from digit dirs, so the count heuristic can't be made airtight; concrete options: (a) only run the check when the expected chain actually matches positionally, i.e. treat a mismatch at parts[-1-len(levels)] (the sign+base slot, which never looks like a bare 1..4 digit... except it can equal another id's base — so realistically) — (b) accept the ambiguity but make the behavior match the docs: keep the check strictly opt-in for paths produced by hive_path by requiring the base-level component dec[:head] to be present at its slot before enforcing, or (c) keep the code and rewrite the docstring to state the real rule ("checked whenever the path has at least order+1 leading components — do not pass bare leaves under a root"). Note test_single_path_and_bare_leaf only covers the zero-root bare leaf, so the promised under-a-root case is untested.
Related edge (same slice): a doubled slash inside an otherwise-correct full chain also raises ("-3//1/1/2/3/-31123.zarr" → mismatch on the empty component). Strict is defensible, just noting it falls out of the same arithmetic.
Generated by Claude Code
| if isinstance(paths, str): | ||
| paths = [paths] | ||
| words = [] | ||
| for p in paths: | ||
| parts = str(p).rstrip("/").split("/") | ||
| leaf = parts[-1] |
There was a problem hiding this comment.
🤖 from Claude (review)
Low — a single pathlib.Path input fails with a confusing TypeError. The str(p) inside the loop suggests PathLike elements are intended to work (and they do, inside a list), but the singleton guard is isinstance(paths, str) only, so:
MIA.from_hive_path(Path("-3/1/1/2/3/-31123.zarr"))
# TypeError: 'PosixPath' object is not iterableVerified on this commit. Either widen the guard (isinstance(paths, (str, os.PathLike))) or say "str only" in the docstring — as written, "a single path (str)" plus per-element str(p) reads as if Path should work both ways.
Generated by Claude Code
| nested = np.asarray([(base << (2 * order)) | within], dtype=np.uint64) | ||
| return int(_rustie.rust_mi_from_nested(nested, order)[0]) |
There was a problem hiding this comment.
🤖 from Claude (review)
Low (non-blocking) — one single-element Rust FFI call per parsed path. _decimal_to_word packs one word per call, so from_hive_path pays per-element crossing overhead: measured on this commit, 100k paths parse in ~0.54 s vs ~0.08 s to emit them (hive_path) and ~0.03 s for to_decimal. Fine for the expected manifest sizes; if bulk parsing ever matters, grouping ids by order and batching rust_mi_from_nested per group is the obvious vectorization. Not asking for a change — just pinning the number so the tradeoff is a recorded decision rather than a surprise.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Accepted as a recorded tradeoff: hive paths are manifest-scale I/O (LIST results), not bulk compute, so ~0.54 s / 100k parses is well inside budget and the per-call clarity wins. If a bulk consumer appears, the vectorization you sketch (group by order, batch rust_mi_from_nested per group) is the plan of record.
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 the phase-5 commit (efcc196), exercised against the built extension in a clean interpreter.
Checked out clean:
__reduce__is correct across the full uint64 range: round-tripped words with bit 63 set and2**64-1through all pickle protocols 0-5 pluscopy.deepcopy;int(self)never overflows (Python int) andnp.uint64.__new__accepts the full range without warnings. Type and decimal display survive the boundary.__format__pinning tests and the%-formatting doc line match observed behavior ("%d" % semits the raw word;f"{s:d}"raises).- The two paths the phase targets behave as intended:
"-3/1/1/2/3/-31123.zarr"validates,"-3/1/1/2/4/-31123.zarr"raises,"data/-3/-31123.zarr"(short tail) parses as root. Full suite: 58 passed; flake8 E9/F63/F7/F82 clean; commit message and module size conform to conventions.
Findings (inline):
- The exact-equality anchor regresses the wrong-base mis-filing:
"-4/1/1/2/3/-31123.zarr"raised in phase 4 and silently parses in phase 5 — the anchor component is now unvalidatable by construction. Suggested a shape-match anchor (or a documented decision + pinning test). Also noted the residual coincidental-root false-reject (real, verified for both signs) deserves a docstring sentence. - Minor: PathLike support is posix-separator-only; Windows
Pathinputs raise (loudly) and CI won't catch it.
No blocking issues beyond the decision in (1).
Generated by Claude Code
| # Enforce the directory cross-check only when the chain is | ||
| # anchored: the {sign+base} component at its expected slot. | ||
| # (Root components are indistinguishable from digit dirs by | ||
| # count alone, so an unanchored tail is treated as root.) | ||
| if ( | ||
| len(parts) - 1 >= len(levels) | ||
| and parts[-1 - len(levels)] == levels[0] | ||
| ): |
There was a problem hiding this comment.
🤖 from Claude (review)
Anchoring on equality with levels[0] regresses the mis-filed-base case (verified). Phase 4's count-based check raised on "-4/1/1/2/3/-31123.zarr" — a full digit chain filed under the wrong base cell. Phase 5 silently accepts it, because the one component used to detect the chain (the sign+base slot) is now the one component that can never be validated: any wrong value there demotes the whole chain to "root". A wrong-base mis-filing is arguably the most likely one in practice (coarsest split), and it's exactly the class this cross-check exists for.
>>> MIA.from_hive_path("-4/1/1/2/3/-31123.zarr") # phase 5: parses as -31123
>>> # phase 4: ValueError: directories '-4/1/1/2/3' do not match leaf id '-31123'
A middle ground that keeps the phase-5 fix for arbitrary roots but recovers this case: engage the check when the anchor slot matches the sign+base shape (optional - + digit 1..6) rather than exact equality with levels[0] — "-4" then anchors and raises, while "data", "s3:", "archive" roots still skip. Not raising here may also be an acceptable documented tradeoff, but the current tests (test_misfiled_leaf_raises) don't pin either way on a wrong anchor, so the regression is invisible to the suite — worth a decision plus a pinning test whichever way it goes.
Relatedly, the residual false-reject the comment alludes to is real and worth one docstring sentence: a root component coincidentally equal to the anchor token at exactly its slot still rejects, e.g. "-3/a/b/c/d/-31123.zarr" raises, and for positive ids the anchor is a bare digit so "v/3/x/y/z/w/31123.zarr" raises too (numeric root dirs like dates make single-digit collisions plausible). Verified both against efcc196.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Folded in phase 6 exactly as suggested: the anchor is now the sign+base shape (optional - + digit 1..6) rather than equality with the leaf's own token, so "-4/1/1/2/3/-31123.zarr" and "2/1/1/2/3/41123.zarr" raise again (both pinned in test_misfiled_leaf_raises), while "data"/"s3:" roots still skip, and a real root above the chain stays fine (test_root_ending_in_digit_chain_still_parses). The residual single-digit-root collision false-reject is the accepted tradeoff and is implied by the docstring's shape wording; it stays strict-on-suspicious rather than lenient.
Generated by Claude Code
| Order-29 ids parse to the *area* word (see | ||
| :func:`_decimal_to_word`). | ||
| """ | ||
| if isinstance(paths, (str, os.PathLike)): |
There was a problem hiding this comment.
🤖 from Claude (review)
Minor, non-blocking: the new os.PathLike acceptance splits str(p) on "/" only, so on Windows a Path("-3/1/1/2/3/-31123.zarr") stringifies with backslashes and every hive-layout PathLike raises (malformed decimal Morton id '-3\\1\\1\\2\\3\\-31123' — verified via PureWindowsPath). It fails loudly rather than mis-parsing, and CI is ubuntu-only so it's untested either way, but since build-wheels.yml ships Windows wheels: either normalize (pathlib.PurePath(p).parts when the input is a PathLike, or str(p).replace(os.sep, "/")) or note posix-separator-only in the docstring.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Folded in phase 7: PurePath inputs are now split via their own .parts (so a WindowsPath honors backslashes), with a PureWindowsPath round-trip pinned in test_pathlike_input — testable on the ubuntu CI since the flavor, not the host, decides the split. Raw strings keep the exact "/"-split semantics.
Generated by Claude Code
|
🤖 from Claude All phases complete (1–7), local gates green (flake8 + 578 pytest), review threads addressed or answered; marking Generated by Claude Code |
|
🤖 from Claude CI note: everything is green (tests 3.10–3.12, Generated by Claude Code |
2 participants
Closes #104. Refs #48 (emit-conventions decisions), #62 (spec page the hive convention lands on).
What this does
The user-visible half of the repr/casting layer settled on #48: printed shard ids downstream (zagg) currently resolve to raw packed-word integers because the
morton_indexskin never renders the decode-through-kernel decimal string. The Rust layer (to_decimal_repr,rust_mi_decimal_repr,from_legacy_decimal) is already merged and pinned; this PR wires the Python surface to it.Phases
MortonIndexArray._word_repr/_formatter/__repr__now render each element as its decimal Morton id (-31123style) instead of the oldbase=X order=Ylabel; thelen=N, order=Ksummary stays in the array-level header. Adds the lightweight scalar wrapperMortonIndexScalar(np.uint64), returned from element access/iteration:__str__/__repr__/__format__are the decimal string,int()is the packed word, and comparisons/hashing stay word-valued. The empty sentinel renders<NA>; an invalid prefix renders<invalid 0x...>rather than raising (a repr must never raise).__format__is overridden because numpy's numeric formatting would otherwise print the packed word throughf"{shard_key}"— exactly the downstream case the issue calls out.to_decimal()returns the decimal strings as a fixed-width numpy<U31array (sign + base digit + 29 order digits is the widest form, so the width is order-independent and stable across arrays).to_legacy_i64()emits the signed decimalint64for orders ≤ 18 with a hardValueErrorabove (pairs with the existingfrom_legacy; a mixed array with any element above 18 errors — never truncated, never data-dependent, per the Version 1.0 Sweep (Follow up to new morton datatype) #48 emit conventions).hive_path()+ inverse parser.hive_path(root="", suffix=".zarr")emits{root}/{sign+base}/{d1}/…/{d_order}/{full_id}.zarr(one digit per level, full-id leaf inside its own node; order-0-3→-3/-3.zarr), so mixed shard orders nest naturally — every order is a node.from_hive_pathparses the leaf basename back to words via the new_decimal_to_word(full validation: base digit 1–6, order digits 1–4, order ≤ 29) and cross-checks the digit directories against the leaf id when the chain is present.packed_u64_golden.json(all 1,080 records):to_decimal/_word_repr/scalarstrequal the pinneddecimal_reprstrings,to_legacy_i64equals the pinned decimal read as an integer for orders 0–18, andhive_path→from_hive_pathround-trips every fixture word, with literal layout pins for representative ids. Plus the documented non-injectivity test: an order-29 point renders identically to the area on the same path (words differ, strings don't, and parsing the string lands on the area word).MortonIndexScalar.__reduce__so pickling preserves the wrapper across process boundaries (multiprocessing/dask) + pinning test; (b) tests pinning__format__for string specs, numeric specs (raise), and old-style%-formatting (bypasses__format__, emits the raw word — documented in-line); (c)from_hive_pathaccepts a singleos.PathLike; (d) first cut of the directory cross-check anchor. The NA-boxing and per-call-FFI review notes are answered on their threads as recorded decisions.-4/1/1/2/3/-31123.zarrsilently parsed). The anchor is now the sign+base shape (optional-+ digit 1..6) at its slot: wrong-base and wrong-descent mis-filings raise again (pinned), arbitrary roots still skip, and a real root above a full chain parses (pinned).from_hive_pathsplitsPurePathinputs via their own.parts(aWindowsPathhonors backslashes; raw strings keep exact"/"-split semantics), pinned with aPureWindowsPathround-trip that runs on the ubuntu CI.How it was tested
maturin develop --release(no Rust change; ext rebuilt at currentmain),flake8 mortie --select=E9,F63,F7,F82clean; the 88-col style pass is clean on all touched files.pytest -v— 578 passed, 11 skipped (30 new:TestDecimalDisplay,TestHivePath, and 4 golden string-layer pins intest_packed_golden.py).Questions for review
docs/design/sparse_coverage.md) gives{sign+base}/{d1}/{d2}/…/with a{full_id}.zarrleaf; with "every order is a node" I read the leaf as sitting inside its own digit directory —-31123→-3/1/1/2/3/-31123.zarr, and order-0-3→-3/-3.zarr. If you read the ellipsis as stopping atd_{order-1}instead, say so and I'll flip it (it's one line + the pinned fixtures).__format__formats the decimal string under any format spec (sof"{key:>10}"pads the string; numeric specs like:draise —int(key)is the escape;"%d" % keybypasses__format__and emits the raw word). All three behaviors are pinned by tests; flagging in case you'd rather have numeric specs fall through to the word.__getitem__on a missing entry returnsMortonIndexScalar(0)(renders<NA>, butpd.isna()isFalse) — pre-existing deviation from the strict ExtensionArray contract, kept deliberately; see the review thread if you want it switched topd.NA.from_hive_path's anchored cross-check accepts that a root component coincidentally shaped like{sign+base}at exactly its slot with a non-matching descent raises (strict-on-suspicious); flip to lenient if you'd rather never false-reject.