add morton_index datatype user docs (issue #64)#129
Conversation
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh adversarial review of this docs-only change, validated by reading mortie/morton_index.py, mortie/arrow.py, mortie/__init__.py, and pyproject.toml (not by running maturin).
Accurate — spot-checked against source and confirmed:
- Every documented method/accessor exists with the claimed behavior:
from_nested/from_latlon/from_words,orders()/order()(raises on mixed),base_cells()/base_cell(),is_fixed_order(),coarsen(),to_nested(),decimal_repr()/to_decimal()("<U32"),hive_path(),isna(),from_morton_index/to_morton_index/morton_index_type,from_arrow, and theMortonIndexScalardisplay-vs-int()split. - The two quoted
ImportErrormessages match the__getattr__import guards verbatim (pandas and pyarrow). - Pinned numeric outputs I could recompute are exact: the packed words
[1152921504606846977, 1441151880758558721, 1729382256910270465, 2017612633061982209],argsort → [1 3 2 0],b == arr → [False False True False], andcoarsen(2)mapping12321/12322/12323/12324 → 123. - Extras story matches
pyproject.toml: numpy is the only hard runtime dep;pandas/pyarroware optional extras; each example is labelled*Requires mortie[...]*and the "every example below requires the relevant extra" gate is stated up front. - Extension name
mortie.morton_index, theextension<mortie.morton_index<MortonIndexType>>repr, the null↔empty-sentinel round-trip, and the__from_arrow__/to_pandas_dtypeparquet landing all line up witharrow.py. - The in-page anchor
#what-is-and-isnt-supported-in-1xresolves, and all threeSee alsocross-links (arrow_interchange.md,specification.md,coverage_methods.md) exist underdocs/. - The NEP-42 forward pointer is appropriately hedged (deferred / forward-looking, no version promised, "nothing above will change its meaning") — no over-promise; and it correctly notes the backing set ops already exist as
moc_and/moc_or/moc_minus. - No
@-mentions; no misuse of#Nin the doc body (issue references stay in the PR body where they belong).
Findings: two non-blocking nits, inline. Nothing blocking — issue #64's acceptance points (extras-gated runnable examples, how the dtype wraps the packed-u64 kernel, repr + order accessors, optional-dep story, deferred-grammar pointer without over-promising) are all met.
Generated by Claude Code
| and the empty sentinel renders as `<NA>`: | ||
|
|
||
| ```python | ||
| na = MortonIndexArray._from_sequence( |
There was a problem hiding this comment.
🤖 from Claude (review)
nit (non-blocking): this is the one NA example, and it reaches for the underscore-private _from_sequence, which reads as internal API in a user-facing doc. The public path builds the same array and is what a reader would actually write:
na = pd.array([int(words[0]), pd.NA, int(words[2])], dtype="morton_index")(pd.array dispatches to _from_sequence internally.) Verified against mortie/morton_index.py: _coerce_words maps pd.NA/None/NaN → the all-zero sentinel, so isna() still round-trips the same way.
Generated by Claude Code
| ``` | ||
|
|
||
| Note the reverse convenience does **not** exist: `pa.Table.from_pandas(df)` on a | ||
| `morton_index` column raises `ArrowTypeError` (the pandas array does not |
There was a problem hiding this comment.
🤖 from Claude (review)
nit (non-blocking): pinning the exact class ArrowTypeError here is version-sensitive. The behavior was verified on pandas 3.0.3 / pyarrow 25, but the extras pin allows pandas>=2.0 / pyarrow>=14, and older stacks can surface a different Arrow error (ArrowInvalid / ArrowNotImplementedError) for the same missing-__arrow_array__ case. Consider softening to "raises an Arrow error" — the real takeaway is the workaround (from_morton_index / pa.array(series.array)). Confirmed against mortie/morton_index.py that MortonIndexArray deliberately defines __arrow_c_array__ but no __arrow_array__, so the pandas→arrow from_pandas path does indeed fail.
Generated by Claude Code
Merging this PR will not alter performance
Comparing Footnotes
|
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial review of the morton_index datatype docs (issue #64). I built the Rust extension and executed every runnable snippet on the PR branch against a real mortie[pandas,pyarrow] env.
Verified accurate (every claim held):
- All pandas outputs reproduce exactly as commented:
from_nested/from_latlonreprs, the packed-word round-trip (from_words↔np.asarray(..., uint64)),pd.Series(words, dtype="morton_index"),MortonIndexScalardecimal-vs-int()split,decimal_repr/to_decimal, the order accessors, the mixed-orderValueErrortext,coarsen,argsort/==Z-order, and the<NA>sentinel/isnabehavior. - All pyarrow outputs reproduce exactly: the
extension<mortie.morton_index<MortonIndexType>>type repr,from_/to_morton_index, null↔sentinel round-trip, the parquet +table.to_pandas()landing asmorton_index,pa.array(arr)via the C-Data path, and the documentedpa.Table.from_pandas→ArrowTypeError. - Both
ImportErrormessage blocks are verbatim frommorton_index.py/arrow.py; the extras (mortie[pandas],mortie[pyarrow]) matchpyproject.toml; numpy stays the only runtime dep; the lazy-__getattr__optional-dependency story is correctly described. - The NEP-42 forward pointer is honest — arithmetic is genuinely undefined, the operator grammar is framed as deferred/non-1.x without over-promising, and the accessor methods are correctly named as the stable 1.x contract (with the existing
moc_*functions cited). - Cross-doc links (
arrow_interchange.md,specification.md,coverage_methods.md) and the internal anchor#what-is-and-isnt-supported-in-1xall resolve.
Findings: none blocking. Two optional polish notes inline (an undemonstrated .hive_path() in the Supported list; a minor np.asarray dtype-idiom inconsistency). Extras-gating is clearly stated per section. The page is terse and accurate.
Generated by Claude Code
| parquet), NA/sentinel semantics, the decimal repr, ordering/equality | ||
| comparisons and `argsort` (the Z-order), and the domain accessors | ||
| (`.orders()`/`.order()`, `.base_cells()`/`.base_cell()`, `.is_fixed_order()`, | ||
| `.coarsen(k)`, `.to_nested()`, `.hive_path()`). |
There was a problem hiding this comment.
🤖 from Claude (review)
.hive_path() is listed here as a supported 1.x accessor but is the only one on this list never demonstrated or explained anywhere on the page (.orders()/.order(), .base_cells()/.base_cell(), .is_fixed_order(), .coarsen(k), .to_nested() all get an example or a mention). It's a real method (MortonIndexArray.hive_path(root="", suffix=".zarr") in mortie/morton_index.py), so the claim is accurate — but a reader hits a dead reference. Optional/non-blocking: either add a one-line example, or drop it from the enumeration to keep the page total. Not a defect in what's written.
Generated by Claude Code
| print(arr.is_fixed_order()) # True | ||
| print(arr.base_cells()) # [0 0 0 0] | ||
|
|
||
| mixed = MortonIndexArray(np.concatenate([np.asarray(arr), np.asarray(pts)])) |
There was a problem hiding this comment.
🤖 from Claude (review)
Very minor consistency nit (non-blocking): earlier the page deliberately teaches np.asarray(arr, dtype=np.uint64) as the idiom for pulling the packed uint64 storage back out (Construction section, and again on line 210 w = np.asarray(arr)). Here the mixed example uses the un-dtyped np.asarray(arr)/np.asarray(pts). Both work — I verified the whole snippet runs and reproduces order=mixed exactly — because element iteration yields MortonIndexScalar (a np.uint64 subclass), so numpy infers uint64. But relying on inferred dtype here reads as slightly inconsistent with the storage-extraction idiom the page just established. Consider np.asarray(arr, dtype=np.uint64) for uniformity.
Generated by Claude Code
|
I'm trying to understand this (under section):
can you explain? Why is repr returning healpix base cells instead of morton base cells in this example? |
|
🤖 from Claude Folded the four non-blocking self-review nits into
Docs-only; no Rust/Python source touched. Marking Generated by Claude Code |
|
🤖 from Claude Good question — and the earlier run should have addressed it instead of only folding the self-review nits. There's no bug, but the example was genuinely confusing. Explanation, then the doc fix (
The confusion is the mismatch with the decimal-Morton repr, which renders the base cell in its signed form (spec §2): base pixels Verified against the build, spanning the range so both forms are visible: b = MortonIndexArray.from_nested([0, 4, 40, 44], depth=1) # base pixels 0, 1, 10, 11
print(b.base_cells()) # [ 0 1 10 11] native HEALPix base pixel (0–11)
print(b.decimal_repr()) # ['11', '21', '-51', '-61'] signed decimal-Morton formSo the all-base-0 example paired One thing worth your call, not changed here: Generated by Claude Code |
Closes #64. Refs #48, #51.
What
A new user-facing page,
docs/morton_index_datatype.md, documenting themorton_indexdatatype skin: the pandasExtensionArray(MortonIndexArray/MortonIndexDtype, registered as"morton_index") and the matching pyarrowExtensionType.It covers:
mortie[pandas],mortie[pyarrow]). The exactImportErrormessages raised by the lazy__getattr__import guards are quoted so a numpy-only user knows what they'll see.ExtensionArray— construction (from_nested,from_latlon,from_words,pd.Series(..., dtype="morton_index")), the packed-uint64storage and how the raw constructor expects packed words (not decimal ids), the decimal-Morton repr andMortonIndexScalar(displays11,int()s to the packed word), order-aware accessors (.orders()/.order(),.base_cells()/.base_cell(),.is_fixed_order(),.coarsen(k)), Z-order comparisons/argsort, and the NA / empty-sentinel semantics.ExtensionType—from_morton_index/to_morton_index, null↔sentinel round-trip, parquet +table.to_pandas()landing back asmorton_index, andpa.array(arr)via the C-Data interface. Points atdocs/arrow_interchange.mdfor the pyarrow-free C Data surface.mortie/morton_index.pydocstrings: "no arithmetic operators are defined").Also added a one-paragraph cross-link from the "Integration with DataFrames" section of
USAGE.md.No code changes — docs only.
Approach
Grounded every snippet in the real API by building the extension (
maturin develop --release) with thetestextra (pulls pandas + pyarrow) and executing each example, then pasting the actual output as inline comments. Two early draft snippets that mis-used decimal ids as packed words (rendering<invalid ...>) were corrected to use real packed words pulled fromnp.asarray(arr, dtype=np.uint64). Thepa.Table.from_pandas(df)path was found to raiseArrowTypeErroron amorton_indexcolumn (no__arrow_array__hook) — documented as a known non-path with the explicit-conversion workaround rather than shown as working.Phases
docs/morton_index_datatype.mdwith runnable, extras-gated examples; wire a cross-link intoUSAGE.md.How it was tested
maturin develop --release+pip install -e ".[test]"(pandas 3.0.3, pyarrow 25.0.0).flake8 mortie --select=E9,F63,F7,F82— clean.pytest -q— 683 passed, 11 skipped, 1 failed. The single failure is the pre-existingtest_spec_page.py::TestDecimalParseTieBreak::test_order29_string_parses_to_area_word(issue main is red: test_spec_page tie-break test predates #120's p-suffix rendering #123, tracked in a separate PR); a docs-only change does not touch it.Questions for review
docs/morton_index_datatype.mdalongsidearrow_interchange.md/coverage_methods.md/specification.md, cross-linked fromUSAGE.md. There is no sphinx/mkdocs toctree in the repo (docs are plain markdown linked fromREADME.md/USAGE.md), so no index wiring beyond the cross-link — confirm that's the intended home.🤖 Generated with Claude Code
https://claude.ai/code/session_015Ziny2mbAruJjuNQTTaHip
Generated by Claude Code