MortonWord: decimal-label constructor, strict accessors, flat export (issue #152) - #212
MortonWord: decimal-label constructor, strict accessors, flat export (issue #152)#212espg wants to merge 19 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #212 +/- ##
==========================================
+ Coverage 96.53% 96.58% +0.05%
==========================================
Files 20 20
Lines 2365 2402 +37
==========================================
+ Hits 2283 2320 +37
Misses 82 82
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:
|
| label (sign column + base digit ``1..6``, one ``1..4`` digit | ||
| per order, optional terminal ``p`` -- spec sections 2 and 4). | ||
| """ | ||
| if isinstance(value, str): |
There was a problem hiding this comment.
🤖 from Claude (review)
non-blocking — the headline silent-wrong-cell bug survives on the bytes path.
isinstance(value, str) is the whole disambiguation, but numpy.uint64 also accepts bytes / np.bytes_ and reads them as a base-10 packed word. So the exact failure this PR exists to close still reproduces verbatim, one type away — the PR body's own example, with a b prefix (verified on this branch, numpy 2.2.6):
>>> MortonIndexScalar("4331422412232") # fixed by this PR
4331422412232 # == decimal_to_word(...) == 5347397354838294540
>>> MortonIndexScalar(b"4331422412232") # still the old reinterpretation
<invalid 0x000003f07ce4edc8> # int() == 4331422412232, the wrong cell, no errorNot academic for the stated downstream: decimal labels come back as bytes from HDF5 attributes (h5py/h5coro hand back bytes for fixed-length ASCII attrs) and from some parquet/arrow metadata paths — which is precisely where zagg's parse boundary sits. MortonIndexScalar(attrs["morton_index"]) gets the label→packed-word reinterpretation the PR says is gone.
Two clean options, either is fine:
- Treat ASCII
bytes/np.bytes_as a label too —isinstance(value, (str, bytes, np.bytes_)),value.decode("ascii")inside the sametryso a decode failure lands in the same pointedValueError. This is what "disambiguated by type alone" reads like to a caller. - Reject them —
TypeErrornaming the ambiguity ("bytesis ambiguous between a label and a packed word; decode it, or passint(...)"). Loud, and keeps the "never silently constructs the wrong cell" invariant total.
What is not fine is the current state, where the class docstring says the inherited constructor "used to read a label string as a base-10 packed word, silently constructing the wrong cell" — past tense — while it still does for bytes.
Whichever way this lands, the Parameters entry at line 65 (value : int, numpy.uint64, or str) should name the real accepted set: bytes and float both construct today through the pass-through (MortonIndexScalar(3.7) → 3), so the documented type list is already narrower than the behavior.
There was a problem hiding this comment.
🤖 from Claude
Folded in cc73cc6 — bytes/bytearray (and therefore np.bytes_, a bytes subclass) now raise a pointed TypeError before the str branch, rather than falling through to numpy's base-10 word reinterpretation:
MortonIndexScalar(b'4331422412232'): bytes is ambiguous here -- numpy.uint64 would read it as a
base-10 packed word, which silently builds the wrong cell for a decimal Morton label. Pass
value.decode('ascii') for a label, or an int for a packed word.
No silent parse of bytes-as-label — that would widen the str-only label surface that was ruled on, and the two readings really are ambiguous, so the caller states which one they meant.
Docstrings updated with it: the class docstring now says bytes is refused (naming the HDF5-attr path that produces it), and __new__'s Parameters entry is rewritten to match actual behavior — value : int-like or str, "any form numpy.uint64 itself accepts ... passed through to it unchanged" instead of the narrower int, numpy.uint64, or str — with a TypeError entry under Raises covering both the bytes refusal and the pass-through of whatever numpy.uint64 raises for anything else.
Tests: test_bytes_is_refused_not_silently_reinterpreted parametrized over b"4331422412232", b"-31123", bytearray(b"3123"), np.bytes_(b"4331422412232"), plus test_bytes_refusal_names_both_ways_out, which pins that the message names both escapes and that .decode("ascii") actually round-trips to the right word.
| word = decimal_to_word(value, dtype=int) | ||
| except ValueError as exc: | ||
| raise ValueError( | ||
| f"MortonIndexScalar({value!r}): not a decimal Morton " |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — {value!r} interpolates the whole input, unbounded.
A decimal Morton label maxes out at 31 characters (sign + base digit + 29 order digits + p), but the message embeds whatever arrived. A caller that passes a large blob by mistake — a whole line of a file, a serialized payload, an accidentally-joined array of labels — gets an exception whose message is that blob, verbatim, into logs and tracebacks:
>>> MortonIndexScalar("3" * 100_000) # ValueError message is ~100 kB longSince the message is otherwise the model of the pointed error the issue asked for, a truncation keeps it that way:
shown = value if len(value) <= 40 else value[:40] + "..."and interpolate shown!r. Cheap, and it keeps the useful part (the leading characters, which are where the grammar violation almost always is) while bounding the worst case.
There was a problem hiding this comment.
🤖 from Claude
Folded in 35b9138. The caller-controlled fragments of the constructor errors are now bounded by a module-level _clip(text, limit=_ERR_REPR_LIMIT) helper (_ERR_REPR_LIMIT = 64), which truncates with a trailing .... Message wording is otherwise byte-identical — {value!r} simply became {_clip(repr(value))}.
One extension beyond the literal finding: the chained {exc} tail echoes the same caller-controlled string (the kernel's parse error quotes the id back), so clipping only the head still let a 100 kB argument produce a 100 kB message. That tail is clipped at 160 so the fold actually achieves the stated goal; the same treatment covers the bytes TypeError from the thread above. A "0123"-style message is unchanged at 208 chars; the 100 kB case now lands at 375.
Test added, test_error_message_is_bounded_for_a_huge_argument: "9" * 100_000 yields a message under 1,000 chars that still contains the truncation marker and the not a decimal Morton label wording. The existing label cases are short and unaffected, so no existing test needed adjusting.
| """ | ||
| return str(self) | ||
|
|
||
| @property |
There was a problem hiding this comment.
🤖 from Claude (review)
nit, aimed at "Questions for review" item 1 rather than this diff — if .base gets folded in here, note that it is not a free name: np.generic already defines base (the array-base attribute), so MortonIndexScalar("-31123").base resolves today and returns None.
>>> np.uint64(5).base is None, hasattr(np.uint64(5), "base")
(True, True)
>>> hasattr(np.uint64(5), "order"), hasattr(np.uint64(5), "decimal")
(False, False)So .decimal and .order are genuinely new names on the type (good — nothing shadowed), but .base would be an override of an inherited numpy attribute, which is a different and louder decision than the two accessors that landed here: anything walking np.generic's attribute surface (numpy's own repr/introspection helpers, dask/pandas duck-typing, inspect-based tooling) would start seeing a base-cell integer where it expects an array or None.
If the answer to Q1 is "fold .base in", base_cell is the collision-free spelling and matches the domain vocabulary the module docstring already uses ("Domain operations (coarsen/order/base_cell)", line 14). Worth pinning the decision before the 1.0 tag, since renaming a public accessor afterwards isn't free.
There was a problem hiding this comment.
🤖 from Claude
Leaving by design — no code change here. The collision you name is real: np.generic.base exists, so a .base accessor on MortonIndexScalar would shadow it, and that is exactly why this PR does not add one. The accessor is deferred as item (1) under "Questions for review" in the PR body, and your base_cell naming suggestion is the substance of that question rather than a separate fix.
So the finding stays standing for espg, to be decided together with Questions item (1) before the 1.0 tag; adding the accessor (under either name) now would decide that question by implementation instead of by ruling.
There was a problem hiding this comment.
🤖 from Claude
Ruled and landed: espg took option (B) on the standing question — the accessor ships now as base_cell (never .base, for exactly the np.generic.base shadowing flagged here), strict like the other accessors, in 85f647a (naming) + 9ede10f (strict posture). The type itself is renamed MortonWord and exported flat in the same fold; adoption at morton-return sites is deferred to 1.1 as issue #215.
| # grammar make it an invalid label, not a word. | ||
| with pytest.raises(ValueError, match="not a decimal Morton label"): | ||
| MortonIndexScalar(bad) | ||
| with pytest.raises(ValueError, match=repr(bad)): |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — match= is a regex, and repr(bad) is being fed to it unescaped.
pytest.raises(match=...) runs re.search on the string, so this assertion only means "the input is named in the message" for inputs whose repr happens to contain no regex metacharacters. Today's twelve cases are all safe by luck; the first case anyone adds with a +, *, (, [, ? or . either blows up as a regex error or, worse, quietly matches/mismatches something other than the literal. "+31123" is a natural addition to this exact grammar-refusal list (the sign column rejects +) and its repr '+31123' compiles as a valid regex that does not match the message — a test that passes for the wrong reason if the surrounding assertion ever loosens.
match=re.escape(repr(bad)) fixes it in one token (re is not currently imported in this file).
Same reasoning applies to match="not a decimal Morton label" two lines up, but that literal has no metacharacters and is fixed text, so it's fine as-is.
There was a problem hiding this comment.
🤖 from Claude
Folded in 4d896b7 — match=repr(bad) is now match=re.escape(repr(bad)), with import re added to the test module. The current bad cases happen not to contain regex metacharacters that change the match, but the escape means adding one later cannot quietly turn the assertion into a weaker (or accidentally passing) pattern.
| with pytest.raises(ValueError, match=repr(bad)): | ||
| MortonIndexScalar(bad) | ||
|
|
||
| def test_int_forms_are_byte_for_byte_uint64(self): |
There was a problem hiding this comment.
🤖 from Claude (review)
non-blocking — the test's name claims more than its body pins.
"byte_for_byte_uint64" reads as every non-str construction path is untouched, but the body exercises exactly two: Python int and np.uint64. The inputs where the new isinstance(value, str) branch actually changes the answer, or where the pass-through is worth pinning, are all absent:
bytes/np.bytes_— the one that matters (see the comment onmorton_index.pyline 83):MortonIndexScalar(b"31123")is still the packed word 31123 whileMortonIndexScalar("31123")is the label. Whatever the resolution there, it wants a pin here so the two paths can't drift again silently.float—MortonIndexScalar(3.7) == 3, still truncating likenp.uint64.bool—True→ 1.- The error-parity cases:
None→TypeError,-1→OverflowError,2**64→OverflowError, all still identical tonp.uint64(I verified all four on this branch). Those are the cases where a hand-written__new__most easily regresses to something else, and theRaisessection on__new__(line 76) currently documents onlyValueError, so nothing pins that the numpy-native exceptions still come through unchanged.
Cheapest form is a parity loop rather than more hardcoded expectations — for each input, assert MortonIndexScalar(v) and np.uint64(v) agree on value or raise the same exception type:
@pytest.mark.parametrize("v", [0, 1, True, 3.7, np.float64(3.9), b"31123", 2**64 - 1, None, -1, 2**64])
def test_non_str_construction_matches_uint64_exactly(self, v):
try:
expected = int(np.uint64(v))
except Exception as exc:
with pytest.raises(type(exc)):
MortonIndexScalar(v)
else:
assert int(MortonIndexScalar(v)) == expectedThat is the actual "byte for byte" claim, and it would have surfaced the bytes hole on its own.
There was a problem hiding this comment.
🤖 from Claude
Folded in 1fb9b36. test_int_forms_are_byte_for_byte_uint64 is now a real parity pin rather than a value spot-check: parametrized over 0, 1, 5347397355232559123, 2**64 - 1, True, False, 1.5, np.uint64(...), np.uint32(7), np.int64(9), each asserted int(MortonIndexScalar(word)) == int(np.uint64(word)) — against numpy itself, so a numpy coercion change surfaces here as a parity break rather than as a stale hand-written expectation — plus type(...) is MortonIndexScalar so the subclass is not silently demoted, and the default MortonIndexScalar() == np.uint64().
Error parity is a companion test, test_int_form_error_parity_with_uint64: None -> TypeError, -1 -> OverflowError, 2**64 -> OverflowError, each asserted on np.uint64(bad) and MortonIndexScalar(bad) in the same test, so the pin is the equality of behavior, not a snapshot of it.
bytes is excluded here by design: per the thread above it is now the one deliberate divergence from np.uint64, pinned by its own tests in cc73cc6. The parity test says so in a comment so the omission reads as a decision.
| s = MortonIndexScalar(0xF000000000000000) | ||
| assert str(s).startswith("<invalid") | ||
|
|
||
| def test_label_constructed_scalar_pickles_as_itself(self): |
There was a problem hiding this comment.
🤖 from Claude (review)
nit, and explicitly pre-existing — not introduced by this commit, so leave-by-design is a perfectly good answer; flagging it because this test is the natural place the question gets asked.
pickle round-trips because __reduce__ (line 174) is honored. copy does not: np.generic defines its own __copy__/__deepcopy__, which take precedence over __reduce__, so both copy paths silently demote to a bare np.uint64 and drop the decimal display:
>>> s = MortonIndexScalar("-31123")
>>> type(copy.copy(s)), str(copy.copy(s))
(<class 'numpy.uint64'>, '10403315139225845764')
>>> type(copy.deepcopy(s)), type(pickle.loads(pickle.dumps(s)))
(<class 'numpy.uint64'>, <class 'mortie.morton_index.MortonIndexScalar'>)I confirmed this is the behavior on main too (a stripped-down np.uint64 subclass with only __reduce__ behaves identically), so it is not a regression from the new __new__.
It's worth a decision now rather than later because the PR body claims "pickle round-trips through the new __new__ unchanged" and this test pins that half — while a copy.copy in any caller (a dataclass replace, a deepcopy of a config dict holding shard keys, pandas' own copy paths) silently loses the label the type exists to carry, which is the same class of surprise as the pickle bug __reduce__ was added to fix. Two lines close it:
def __deepcopy__(self, memo):
return type(self)(int(self))
__copy__ = ... # same shape, no memo argplus one assertion beside the pickle one. Alternatively, leave it and say so in the class docstring next to the __reduce__ note, so the asymmetry is documented rather than discovered.
There was a problem hiding this comment.
🤖 from Claude
Acknowledged, leaving by design — no code change. Confirmed the behavior: numpy's __copy__/__deepcopy__ win over __reduce__, so copy.copy/copy.deepcopy demote a MortonIndexScalar to a bare np.uint64. Checked against main as well — it is pre-existing there and unchanged by this PR, not a regression introduced by the constructor override (pickle, which does go through __reduce__, round-trips as the subclass and is pinned by test_label_constructed_scalar_pickles_as_itself).
Fixing it means adding __copy__/__deepcopy__ to the scalar, which is outside the re-scoped residual for this PR (constructor + .decimal/.order). It stays standing for espg as a possible follow-up — filing a follow-up issue is a side-effecting action, so that is his call rather than something I open off a review comment.
There was a problem hiding this comment.
🤖 from Claude
Status after the ruling fold: still leave-by-design. The 2026-08-24 rulings (rename to MortonWord, flat export, base_cell, strict accessors) did not touch copy semantics — copy.copy/copy.deepcopy still demote to bare np.uint64, pre-existing on main and unchanged by this PR. A follow-up remains espg's call.
Merging this PR will improve performance by 13.04%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | coverage_triangle[4] |
145.5 µs | 128.8 µs | +13.04% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/152-scalar-constructor (c960c65) with main (4900a7e)
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. ↩
| pass an ``int`` for a packed word. Non-``str``, non-int-like | ||
| values raise whatever ``numpy.uint64`` raises for them. | ||
| """ | ||
| if isinstance(value, (bytes, bytearray)): |
There was a problem hiding this comment.
🤖 from Claude (review)
non-blocking — the guard closes the bytes carrier, but a 0-d numpy string array walks past both branches and still silently constructs the wrong cell — the exact failure this PR exists to close, and the one docs/api/morton_index.md:14 now calls impossible ("never silently constructs"):
>>> MortonIndexScalar(np.array("31123")) # 0-d <U5
<invalid 0x0000000000007993> # == word 31123, not the label's cell
>>> MortonIndexScalar(np.array(b"31123")) # 0-d |S5 -- same
<invalid 0x0000000000007993>
>>> MortonIndexScalar("31123")
31123(verified on this branch, numpy 2.2.6, .venv-152.)
np.str_ / np.bytes_ are covered only because they subclass str / bytes; a 0-d array subclasses neither. The class docstring names "what an HDF5 attr reader hands back" as the motivating source, and np.asarray(attr) / a |S dataset scalar are exactly how those readers pass labels around — same caller, uncovered. Two lines ahead of the guards would close it:
if isinstance(value, np.ndarray) and value.ndim == 0 and value.dtype.kind in "SU":
value = value[()]so a 0-d U takes the str path and a 0-d S gets the pointed TypeError. If it is deliberately out of scope, then the "never silently constructs" claim on the docs page wants a qualifier, because it is falsifiable in one line today.
There was a problem hiding this comment.
🤖 from Claude
Folded in 7ecb8f8. __new__ now unwraps a 0-d ndarray whose dtype.kind is in "SU" via .item() before the two guards, so np.array("31123") takes the label path and np.array(b"31123") hits the bytes refusal. Numeric 0-d arrays are untouched (numpy parity preserved).
if (
isinstance(value, np.ndarray)
and value.ndim == 0
and value.dtype.kind in "SU"
):
value = value.item()Three tests added in TestScalarConstructor: test_zero_d_str_array_is_a_label_too, test_zero_d_bytes_array_is_refused_like_bytes, and test_zero_d_numeric_array_keeps_numpy_parity (the last pins np.array(word, dtype=np.uint64) and np.array(5) against np.uint64 itself).
| raise TypeError( | ||
| f"MortonIndexScalar({_clip(repr(value))}): bytes is " | ||
| f"ambiguous here -- numpy.uint64 would read it as a " | ||
| f"base-10 packed word, " |
There was a problem hiding this comment.
🤖 from Claude (review)
non-blocking — the message asserts a numpy behavior that only holds for two of the three types the guard catches. For bytearray, numpy reads the buffer, not a base-10 word:
>>> np.uint64(b"3123")
np.uint64(3123)
>>> np.uint64(bytearray(b"3123"))
array([51, 49, 50, 51], dtype=uint64) # ASCII code points -- and an ndarray, not a scalarSo for a bytearray argument, "numpy.uint64 would read it as a base-10 packed word, which silently builds the wrong cell" is simply not what would have happened — it would not have built a cell at all. Refusing it is still the right call; only the stated reason is wrong for that member. The same overclaim is repeated in the test comment at mortie/tests/test_decimal_parse.py:131-134 ("numpy.uint64 reads b"4331422412232" as a base-10 packed word", asserted over a parametrization that includes bytearray) and in the CHANGELOG's "instead of numpy's silent base-10 word reinterpretation".
Cheapest fix: widen the wording once ("numpy would take it as a packed word or a raw byte buffer — never as a label") rather than naming a single reading that only bytes/np.bytes_ exhibit.
There was a problem hiding this comment.
🤖 from Claude
Folded in edc8f21. Confirmed both readings locally: np.uint64(b"31123") -> np.uint64(31123) (base-10 word), np.uint64(bytearray(b"31123")) -> array([51, 49, 49, 50, 51], dtype=uint64) (raw buffer). The wording now refuses bytes-like input as ambiguous and names both readings, at every site: the TypeError message ("bytes-like input is ambiguous here -- numpy reads bytes as a base-10 packed word and bytearray as a raw buffer, and neither reading is the decimal Morton label..."), the class docstring, the Raises section, the test_bytes_is_refused_not_silently_reinterpreted comment, and the CHANGELOG sentence. Both escapes (value.decode("ascii") / int) are kept verbatim; the existing pytest.raises(match=...) strings moved to the new phrase.
| ---------- | ||
| value : int-like or str | ||
| Either a packed word -- any form ``numpy.uint64`` itself | ||
| accepts (``int``, ``numpy.uint64``, ``bool``, ``float``, ...), |
There was a problem hiding this comment.
🤖 from Claude (review)
non-blocking — 35b9138/1fb9b36 widened this from "any integer form numpy.uint64 accepts" to "any form ... (int, numpy.uint64, bool, float, ...)", and the widened version is inaccurate at both ends:
- It is not always a
MortonIndexScalar. Buffer-protocol arguments make__new__return something else entirely:
>>> type(MortonIndexScalar(memoryview(b"3123")))
<class 'numpy.ndarray'> # array([51, 49, 50, 51], dtype=uint64)
>>> type(MortonIndexScalar(np.array([3])))
<class 'numpy.ndarray'>That is inherited numpy behavior and predates the PR, but the Returns section right below now promises MortonIndexScalar for "any form numpy.uint64 itself accepts", which the trailing "..." explicitly extends to the rest of numpy's coercion table. Either scope the sentence to scalar int-likes, or say the buffer forms pass through as numpy makes them.
floatis now advertised, and pinned.MortonIndexScalar(1.5)→ word1(<invalid 0x...0001>), and the new parity parametrization attest_decimal_parse.py:161locks that in. In a PR whose thesis is "an ambiguous argument is refused, never silently reinterpreted", a float silently truncating into a packed word is the same shape of hazard as the bytes case — and it is now documented as supported rather than tolerated. Fine either way, but worth one clause saying which it is (numpy parity by choice vs. an oversight), sincebytesgot a pointed refusal for strictly less ambiguity.
There was a problem hiding this comment.
🤖 from Claude
Folded in 012acc5. The Parameters entry now leads with the label form (str, plus the 0-d "U" array unwrapped by 7ecb8f8) and says everything else is handed to numpy.uint64 and takes its semantics whole -- "bool and float included, so 1.9 truncates to 1, kept as numpy parity by choice rather than tightened here". Returns carries the buffer caveat explicitly: an input that numpy turns into an array rather than a scalar (a memoryview, or a multi-element array) "comes back as numpy returns it, a plain ndarray, not this type".
Two tests pin the claims so the docstring cannot drift: test_buffer_input_follows_numpy_and_is_not_this_type (asserts type(out) is np.ndarray and equality with np.uint64(memoryview(b"123"))) and test_float_truncates_exactly_as_numpy_does. numpydoc lint mortie/morton_index.py is clean.
|
|
||
| ## [Unreleased] | ||
|
|
||
| - **`MortonIndexScalar` constructs from the decimal label, and grows `.decimal` |
There was a problem hiding this comment.
🤖 from Claude (review)
non-blocking — this entry has no BREAKING marker, and by this file's own bar it is one. Both changes alter the result of code that ran clean on main:
MortonIndexScalar("123")returned packed word123; it now returns a different cell (or raises).MortonIndexScalar(b"123")returned packed word123; it now raisesTypeError.
The file marks smaller changes than that — line 79 is **BREAKING (small): ``norm2mort`` and ``mort2norm`` keep a length-1 array ...**, and there are 6 more BREAKING entries in the 1.0 window (lines 24, 79, 99, 142, 201, 265, 332). A reader scanning for breaks before 1.0 will skim past this one. Suggest leading with **BREAKING (small):** and one migration clause for anyone who was (accidentally) relying on the old str/bytes coercion.
There was a problem hiding this comment.
🤖 from Claude
Folded in cae4214. The entry now opens:
- **BREAKING (small): `MortonIndexScalar` constructs from the decimal label,
and grows `.decimal` / `.order` accessors** (issue #152).matching the existing BREAKING (small): norm2mort and mort2norm keep a length-1 array an array entry's style. The two behavior changes that earn it -- MortonIndexScalar("123") changing meaning, and bytes now raising -- are already spelled out in the body text.
| An invalid label raises `ValueError` at the boundary, naming the input and | ||
| the grammar, and `bytes` input is refused with a pointed `TypeError` (decode | ||
| to `str` for a label, pass an `int` for a word) instead of numpy's silent | ||
| base-10 word reinterpretation. `int` / `numpy.uint64` construction, arithmetic, and the |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — this line is 89 columns; every other line in the entry (and in the file) wraps at ~79. Re-wrap so the paragraph doesn't have one long line in the middle.
There was a problem hiding this comment.
🤖 from Claude
Folded in d6294a0, its own commit. The whole entry is rewrapped; every line in it is now <= 76 columns (the 100- and 121-column lines are gone, the latter having grown from the finding-2 edit). The em dash was normalized to the file's — while rewrapping, since the finding-2 text had introduced a --.
| included) parsed eagerly through `decimal_to_word` — an invalid label raises | ||
| `ValueError` at the boundary, never silently constructs, and `bytes` is refused | ||
| with a pointed `TypeError` rather than reinterpreted. The `.decimal` / | ||
| `.order` accessors read the label string and the HEALPix order back off the |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — two small drifts against the docstrings mkdocstrings renders directly below this paragraph:
- "
.decimal... read[s] the label string" — it doesn't always..decimalisstr(self), so it yields"<NA>"for the sentinel and"<invalid 0x...>"for a bad word (the property docstring says this explicitly). The page attaches the never-raise caveat only to "display", which a reader will take asrepr/str/format, not as the accessor named one clause earlier. - Line 11 enumerates the word forms as "an
int/numpy.uint64", while the__new__docstring rendered below now says "(int,numpy.uint64,bool,float, ...)". Same screen, two different answers — pick one enumeration.
There was a problem hiding this comment.
🤖 from Claude
Folded in 1440d48. The constructor sentence on docs/api/morton_index.md now mirrors the docstring's actual acceptance: a str (or a 0-d "U" array of one) is the label, "anything else is the packed word, handed to numpy.uint64 and taking its semantics whole (int, numpy.uint64, and by numpy parity bool and a truncating float)", and bytes-like input is called out as the one deliberate divergence. The .decimal prose no longer floats the sentinels as a separate display note -- it says .decimal is exactly the str rendering, "so the display's lazy/never-raise sentinels pass straight through it: "<NA>" for the empty word, "<invalid 0x...>" for one with an invalid prefix", which is what test_decimal_matches_str_rendering asserts.
| """ | ||
| if len(text) <= limit: | ||
| return text | ||
| return text[: limit - 3] + "..." |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — two rough edges in _clip, both currently unreachable at the two call sites (limits 64 and 160), flagged only so a third caller doesn't inherit them:
- Clipping the rendered
reprcuts inside the quotes, so a long argument prints as an unterminated literal: the 100k-9case intest_error_message_is_bounded_for_a_huge_argumentyieldsMortonIndexScalar('9999...with an opening quote and no closing one. Clipping the value beforerepr(or re-appending the closing quote) reads better in a traceback. limit < 4slices negatively (_clip("abcdef", 2)→"abc...", longer than the limit it promises). The docstring's "truncated ... if it would otherwise exceedlimitcharacters" is then false; a one-line floor or a note on the parameter would cover it.
There was a problem hiding this comment.
🤖 from Claude
Folded in 34e3f57. _clip now clamps the slice -- text[: max(limit - 3, 0)] + "..." -- so a limit under 4 can no longer slice negatively and return more than it was asked for. The docstring says both things: a limit under 4 "leaves no room for anything but the ellipsis, which is then returned on its own", and truncation is by characters, "so a clipped repr is a readable fragment rather than a valid Python literal -- its closing quote may be gone".
test_clip_handles_a_limit_with_no_room_for_text walks limit 0..4 and asserts the result ends in "..." and is no longer than max(limit, 3). Kept to the helper; the constructor's own limits (80 / 160) never reach this path.
|
🤖 from Claude Status: both phases complete, both adversarial-review passes folded (13 threads: 11 folded, 2 left by design and standing), CI green on
|
espg
changed the title
| ValueError | ||
| If this word decodes to no legal cell (empty sentinel included). | ||
| """ | ||
| self._decode_label() |
There was a problem hiding this comment.
🤖 from Claude (review)
non-blocking — the _decode_label() gate here is provably redundant: rust_mi_base_cell_of already carries the exact same predicate as its 255 sentinel, which the docstring two lines up even names ("rather than mapping to the kernel's 255 sentinel").
Measured over 400k uniformly random 64-bit words: rust_mi_base_cell_of(w) != 255 and "rust_mi_decimal_repr succeeds" agree on every word, zero disagreements (word 0 maps to 255 too, so the empty sentinel is covered by the same test).
So the gate buys nothing but a full label decode plus a str allocation that is thrown away on the next line:
w.base_cell (gate + base_cell_of) 1.35 us
raw base_cell_of only 0.56 us
~2.4x for a property documented as a cheap data query. The strict behavior is right; only the mechanism is over-built. One shared cheap gate — decode the base cell once, raise if it comes back 255 — gives byte-identical semantics and messages for one kernel call:
base = int(_rustie.rust_mi_base_cell_of(np.asarray([int(self)], dtype=np.uint64))[0])
if base == 255:
raise ... # same two messages, empty-sentinel branch still keyed on word == 0
return baseThe same predicate would serve .order's gate at line 248 (0.56 us vs 0.69 us there), leaving _decode_label for .decimal, the one accessor that actually wants the string.
There was a problem hiding this comment.
🤖 from Claude
Folded in 0c93c76. _decode_label is gone; _require_cell(self) is the shared gate:
word = int(self)
if word == 0:
raise ValueError(
"MortonWord 0x0000000000000000 is the empty sentinel -- it "
"decodes to no legal cell (display renders it '<NA>')"
)
cell = int(_rustie.rust_mi_base_cell_of(np.asarray([word], dtype=np.uint64))[0])
if cell == 255:
raise ValueError(
f"MortonWord {word:#018x} decodes to no legal cell (invalid packed word)"
)
return cellBoth message shapes are byte-identical to before except the dropped kernel tail (finding 2). Accessor wiring: .base_cell is now return self._require_cell() — one kernel call total, no second rust_mi_base_cell_of; .order gates then defers to orders_of; .decimal gates then renders through rust_mi_decimal_repr.
I re-verified your equivalence result independently before folding rather than taking it on trust: 400,000 seeded-random uint64 words plus both sentinels, comparing rust_mi_base_cell_of == 255 against "rust_mi_decimal_repr raises" elementwise — exact agreement, 99,752 illegal, and word 0 → 255 confirmed (which is why the word == 0 check stays first, to keep the distinct empty-sentinel message).
Docstrings updated to match: _require_cell records why the base-cell sentinel is the gate (equivalence + ~2.4x), .order now says "gated on naming a legal cell" instead of "decoded first", .base_cell notes the 255 sentinel is the gate. numpydoc clean. Existing anchors still pass — test_accessors_raise_on_the_empty_sentinel and test_accessors_raise_on_an_invalid_word are green across all three accessors.
| except ValueError as exc: | ||
| raise ValueError( | ||
| f"MortonWord {word:#018x} decodes to no legal cell " | ||
| f"(invalid packed word): {exc}" |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — two small things about this interpolated kernel message.
-
It is unclipped, unlike its sibling in the constructor at line 167, which wraps the same kind of value in
_clip(str(exc), 160)._clip's own docstring names this exact case as its purpose: "The rendered fragment, e.g.repr(value)orstr(exc)." Today's_rustiemessages are short fixed strings so nothing overflows, but the module has one convention for embeddingstr(exc)and this is the one site that skips it. -
The rendered message leaks array vocabulary into a scalar accessor. Actual output for
MortonWord(0xd000000000000123).base_cell:
ValueError: MortonWord 0xd000000000000123 decodes to no legal cell
(invalid packed word): morton_index array contains an empty or invalid word
The reader asked one word for its base cell and is told an array contains an invalid word. The prefix this commit adds is the good part; the kernel tail is what confuses. Either drop the : {exc} tail (the prefix already says everything the caller can act on) or keep it clipped — up to you, but the two points travel together.
There was a problem hiding this comment.
🤖 from Claude
Subsumed by finding 1's commit 0c93c76 — resolved there rather than clipped.
The refactor drops the chained tail entirely: the new gate is rust_mi_base_cell_of's 255 sentinel, which returns a value instead of raising, so there is no exc to interpolate. The invalid-word message is now exactly
MortonWord 0xf000000000000000 decodes to no legal cell (invalid packed word)
with no : {exc} suffix — both the unbounded-length concern and the array-vocabulary leak go away at the source. _require_cell's docstring names that as a reason for the choice, not just a side effect.
I checked the other accessor paths for any remaining chained kernel text: .decimal's rust_mi_decimal_repr call sits past the gate and is no longer wrapped in try/except, and .order delegates to orders_of past the gate. So no accessor path chains kernel text any more, and no _clip call was needed. The constructor's _clip(str(exc), 160) on the label-parse path is untouched — that one quotes the grammar parser at a caller who passed a label, which is the right vocabulary.
| # raise -- the strict posture is confined to the accessors. | ||
| for word in (0, 0xF000000000000000): | ||
| s = MortonWord(word) | ||
| assert str(s) in ("<NA>",) or str(s).startswith("<invalid") |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — this assertion is weaker than the test's name promises. str(s) in ("<NA>",) is just == "<NA>", and because the disjunction is evaluated per-word with no tie to which word is in hand, either branch can satisfy either word. Swap the two renderings — empty sentinel printing <invalid 0x0000000000000000>, an invalid-prefix word printing <NA> — and this test still passes.
The never-raise half is genuinely exercised (the calls happen), so this is only about the value pin. Pinning per word costs nothing and closes it:
assert str(MortonWord(0)) == "<NA>"
assert str(MortonWord(0xF000000000000000)) == "<invalid 0xf000000000000000>"with the repr(s) == str(s) == f"{s}" identity kept as-is over both.
There was a problem hiding this comment.
🤖 from Claude
Folded in 726b81c. The loop-with-disjunction is gone; each word is pinned by value, with its strict-raise assertion alongside:
empty = MortonWord(0)
assert str(empty) == "<NA>"
assert repr(empty) == str(empty) == f"{empty}"
with pytest.raises(ValueError, match="empty sentinel"):
empty.decimal
invalid = MortonWord(0xF000000000000000)
assert str(invalid).startswith("<invalid")
assert repr(invalid) == str(invalid) == f"{invalid}"
with pytest.raises(
ValueError, match="0xf000000000000000 decodes to no legal cell"
):
invalid.decimalSwapping the two renderings now fails the test, which was your point — the old str(s) in ("<NA>",) or str(s).startswith("<invalid") accepted either word rendering as either form. The repr == str == f"" identity is kept per word, and the comment says explicitly that the pinning is per word and not as a set, so the weaker form does not creep back.
| mortie.decimal_to_word("-31123") # np.uint64, the packed word | ||
| mortie.decimal_to_word("-31123", dtype=int) # a Python int | ||
| mortie.decimal_to_word("-31123", dtype=mortie.morton_index.MortonIndexScalar) | ||
| mortie.decimal_to_word("-31123", dtype=mortie.morton_index.MortonWord) |
There was a problem hiding this comment.
🤖 from Claude (review)
nit — this line still routes through the submodule path, which contradicts two claims the same commit makes elsewhere.
The CHANGELOG entry says the type is now mortie.MortonWord "(decimal_to_word's dtype= escape spells it the same way)", and docs/api/morton_index.md says "The type is exported flat as mortie.MortonWord." But the one runnable example of the dtype= escape in the docs spells it mortie.morton_index.MortonWord — the pre-flat-export path. Both resolve, so nothing breaks; it is the copy-paste line a reader takes away, and it is the longer spelling the flat export exists to retire.
mortie.decimal_to_word("-31123", dtype=mortie.MortonWord)Worth a glance at line 113 in the same file too, which introduces the type as "a MortonWord (a numpy.uint64 subclass)" without ever naming where it is imported from — this example is the only place the page spells the path.
There was a problem hiding this comment.
🤖 from Claude
Folded in 5aed7aa. docs/morton_index_datatype.md line 145 now reads
mortie.decimal_to_word("-31123", dtype=mortie.MortonWord)matching the flat export the rest of the page (and the CHANGELOG entry) uses. I swept the whole docs/ tree plus the root markdown for the old spelling — grep -rn "morton_index.MortonWord" docs/ *.md now returns nothing, so this was the last one. mkdocs build --strict is clean.
|
|
||
| def __reduce__(self): | ||
| """Pickle as a ``MortonIndexScalar`` rather than a bare ``uint64``. | ||
| """Pickle as a ``MortonWord`` rather than a bare ``uint64``. |
There was a problem hiding this comment.
🤖 from Claude (review)
nit / note, no action requested — recording a consequence of the rename that nothing in the diff currently states.
__reduce__ returns (type(self), (int(self),)), so a pickle written by any released mortie carries the literal name mortie.morton_index.MortonIndexScalar in its stream. With no alias (espg ruled that, and I am not asking for one), loading such a pickle under this branch fails at import resolution:
AttributeError: Can't get attribute 'MortonIndexScalar' on <module 'mortie.morton_index'>
That reaches further than a source-level rename: any persisted pickle, and any multiprocessing / dask exchange spanning a version boundary. Pre-1.0 that seems entirely acceptable — but the CHANGELOG's BREAKING entry currently enumerates the source consequences only. One clause naming the pickle break would let a reader on 0.9.x know before they hit it, e.g. "pickles written by earlier releases no longer load, since the scalar pickles by name."
There was a problem hiding this comment.
🤖 from Claude
Folded in c960c65. The [Unreleased] MortonWord entry now carries the clause, placed right after the "construction, arithmetic, and display are unchanged" sentence so the break is not buried:
Because the type pickles by name (
__reduce__rebuilds the wrapper), pickles ofMortonIndexScalarwritten by prior releases no longer unpickle — the class that name refers to is gone. That is a deliberate pre-1.0 break: no alias or shim is provided, so re-emit any persisted scalars (or store the packedint/uint64, which is version-independent).
It names the mechanism (__reduce__ returns (type(self), (int(self),)), so the pickle carries the class name, not the buffer), states the no-shim decision as deliberate rather than an oversight, and gives the two migrations — re-emit, or persist the raw word, which never had this exposure in the first place.
1 participant
Closes #152 — the residual delta re-scoped in this comment after the 2026-08-24 sweep on issue #48: most of the issue shipped via #104 (
MortonWord) and PR #195 phase 6 (scalar-return unification onnp.uint64); what remained was the decimal-label constructor and the.decimal/.orderaccessors.What this does
The headline bug (silent wrong-cell construction):
Constructor semantics, disambiguated by type (
mortie/morton_index.py):strargument parses as a decimal Morton label, routed through the publicdecimal_to_wordpath (dtype=int), point-kindpsuffix grammar included.MortonWord("4331422412232")now equalsdecimal_to_word("4331422412232")and displays as its own label.ValueErroreagerly at the boundary, naming the input and the grammar, chained on the kernel'sParseErrorrendering:MortonWord('7123'): not a decimal Morton label (['-'] + base digit 1..6 + one 1..4 digit per order + optional terminal 'p' -- spec sections 2 and 4): decimal Morton id '7123': base digit 7 outside 1..6int/np.uint64/ word-like inputs keep today's packed-word behavior byte-for-byte, including the lazy display posture for invalid words (<invalid 0x...>renders, never raises) — the eager/lazy split ruled on the 2026-08-19 comment.Accessors, pure delegation (the Moc-object delegation discipline from #197, no new math):
.decimal— the canonical label string, exactly thestr/reprrendering (so"<NA>"/"<invalid 0x...>"for the sentinel/invalid words, matching the repr posture)..order— the HEALPix order 0–29 viaorders_of(unvalidated suffix decode, exactly the kernel's documented posture; lazily imported somortie.morton_indexstays a leaf import).Folded from adversarial review (phase 1 pass):
bytes/bytearrayinput now raises a pointedTypeErrornaming both escapes (.decode('ascii')for a label,intfor a word) instead of numpy's silent base-10 word reinterpretation — the same silent-wrong-cell failure mode, arriving via HDF5-attr byte strings; constructor error messages are bounded (~64-char clipped argument); int-form construction is pinned byte-for-byte againstnp.uint64itself, error parity included.Contract preserved: still an
np.uint64subclass; arithmetic demotes to barenp.uint64(pinned), comparisons/hash stay word-valued (pinned), display dunders untouched, pickle round-trips through the new__new__unchanged.Phases
.decimal/.orderaccessors + tests (mortie/tests/test_decimal_parse.py: regression pin on the sweep's exact example, grammar-refusal matrix incl. a packed-word-as-string, byte-for-byte int-form pins, arithmetic-demotion pin)docs/api/morton_index.md, MortonWord prose only) + CHANGELOG under the 1.0 windowHow tested
maturin develop --releasebuild in a fresh worktree venv;pytest -v(full suite),flake8 mortie --select=E9,F63,F7,F82,ruff check --select E,F,W,Ion touched files,numpydoc lint mortie/morton_index.py.Coordination
MortonWordprose ondocs/api/morton_index.md(nomembers:roster change — the class was already listed), to keep the overlap surface minimal.Coordinated fold (espg rulings, in-session 2026-08-24) — commits
85f647a+9ede10f:MortonWord(pre-1.0, no alias — every internal spelling updated,decimal_to_word(dtype=MortonWord)included), exported flat (mortie.MortonWord, in__all__), and gainsbase_cell(not.base, whichnumpy.genericalready owns). Adoption at mortie's own morton-scalar return sites is deferred to 1.1 as issue Adopt MortonWord at scalar morton-return sites (deferred half of the option-(B) ruling) #215 — no return sites change here..decimal/.order/.base_cellare data queries and raise a pointedValueErrornaming the word when it decodes to no legal cell (empty sentinel included) — "we don't want to propagate invalid data". The never-raise posture stays confined to the display dunders (pinned side by side on the same invalid words).Questions for review
All three original questions are resolved by espg ruling (2026-08-24):
Deferred pieces of the 2026-08-19 ruling— Option (B) ruled: rename toMortonWord+ flat export +base_celllanded in this PR (commit85f647a); adoption at morton-return sites deferred to 1.1 (issue Adopt MortonWord at scalar morton-return sites (deferred half of the option-(B) ruling) #215). The.base-shadowing review thread resolved the naming:base_cell.Should— Strict ruled: it raises (commit.decimalraise on invalid words?9ede10f).Should— Strict ruled: it raises, as does.orderraise?base_cell(commit9ede10f).Still standing (leave-by-design from review):
copy.copy/copy.deepcopydemote to barenp.uint64— pre-existing onmain, unchanged here; a follow-up is espg's call.Note on docs coordination: PR #214's pin roster lists the scalar under its old name/scope; if #214 merges first, a merge-forward pin fix follows on this branch (this PR retitles only the
docs/api/morton_index.mdentries it owns).