Convert morton_index, geometry and arrow docstrings to numpydoc - #141
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #141 +/- ##
==========================================
+ Coverage 94.41% 94.90% +0.49%
==========================================
Files 9 10 +1
Lines 1378 1434 +56
==========================================
+ Hits 1301 1361 +60
+ Misses 77 73 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 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)
Fresh-context adversarial review of e68098e (the head as of this review — the branch gained 02b230c and e68098e mid-review, both included below).
Verdict: the conversion is faithful. I went looking specifically for dropped rationale and did not find any in the three modules. Four comments posted, one of which is a real content loss (from_wkb / from_wkt), one a stale contradiction between the PR body and the head commit, and two minor.
What I verified independently, not by reading the summary
- Docstring-only claim — re-derived, holds. Parsed both revisions with
ast, stripped every module/class/function docstring, comparedast.dump. All three modules IDENTICAL. Separately skimmed the raw diff for changed logic, defaults, or runtime string literals: none. But the AST check is blind to comments — see themorton_index.pycomment on the six (not two) removed comment blocks. All six survive in the docstrings that replaced them; only the body's count is wrong. - Lost rationale — the priority item. Extracted every docstring from both revisions keyed by qualified name and diffed them pairwise, then ran a token-level check for words present in the old text and absent from the new. Every hit but one resolved to a faithful paraphrase or a move into a structured section. Spot-confirmed the load-bearing cross-references all survive:
#35/#58(both module docstrings),#48(from_legacy,decimal_repr,to_decimal,to_legacy_i64),#93(from_arrow),#104(MortonIndexScalar,__getitem__,hive_path,_word_repr),#114(the four parse functions),#120(thepkind-suffix rule in five places),#62(hive_path),#71(from_geometry,to_geometry). The_require_shapelyspherely open question and the_stitch_segmentspole-reachability contract are both intact, as the body claims. The one genuine loss isfrom_wkb/from_wkt. - Examples that do not run — vacuous. No
Examplessection and no>>>line anywhere in the diff, so there was nothing to execute. - Docstrings that lie — none found. Wrote an
astwalker comparing everyParametersentry against the real signature across all three files: zero missing, zero extra, zero renamed, defaults all correct. (The only "no Parameters section" hits areMortonIndexArray.__init__, whose parameters correctly live on the class docstring per numpydoc, and the six one-line comparison dunders.) Then exercised ~30Returns/Raisesclaims against the built extension rather than reading them —order()/base_cell()returningNoneon empty, the mixed-orderValueError, the 2-D ctorValueError,points=Truewithorder != 29, lat/lon shape mismatch,to_legacy_i64above order 18, all threefrom_hive_pathrejections, the point-idhive_pathrejection,decimal_to_word's dtypeTypeError,decimals_to_wordson a barestr,decomposeon a Point and onPOLYGON EMPTY,_spherical_signed_areaon a 2-vertex ring returning0.0,_tangent_azimuthon parallel vectors returning0.0,__getitem__scalar-vs-slice return types. All matched. Including the subtle one —"%d" % keyreally does bypass__format__and emit10403315139225845764whilef"{key}"gives-31123, exactly as the promoted__format__docstring claims. - Behaviour change — none. Test suite is byte-identical to baseline.
Gates, re-run at e68098e
ruff check mortie/{morton_index,geometry,arrow}.py All checks passed!
flake8 mortie --select=E9,F63,F7,F82 clean
flake8 <the three> --max-line-length=88 clean
pytest -q 757 passed, 12 skipped
I also reproduced the rendered-docs check independently, which is now easier than when the body was written: mkdocs.yml and docs/ have since landed on main (c901d63..1fa5bfa, issue #133), so no cross-branch assembly is needed. Building main's docs tree against this branch's mortie with mkdocs build --strict (which mkdocs.yml also sets internally) → built clean, 14 rendered doc sections on api/geometry and 13 on api/arrow, matching the body's numbers. api/morton_index renders 6, consistent with Questions item (1).
On the open questions
- (1)
_build_classes()nesting. Confirmed —griffecannot reach the nested classes, so the ~45-object runtime surface publishes as 6. Converting them anyway was the right call: they are whathelp()and an IDE show. Agreed this is a design call for a follow-up, not something to fold in here. - (3)
hive_path/from_hive_pathRaisesgaps. Verified all three additions against the running code (transcripts in thefrom_wkbcomment's style). The new entries are accurate and the previous docstrings were genuinely incomplete. Documenting observed behaviour is the right resolution — no behaviour changed. - (4) Reworded summaries. Reviewed each. The
base_cell/order/is_fixed_orderrewrites lose nothing (the "or raise" half has a realRaisessection to land in)._decimal_to_wordis the exception — see that comment. Two other summaries shed a word without a home:to_decimaldropped "Vectorized" (which was what distinguished it fromdecimal_repr) and_word_reprwent from "issue #104" to "#104", inconsistent with the file's usual spelling. Both trivial, mentioned only for completeness.
Scope respected: tools.py, coverage.py, prefix_trie.py, linestring.py untouched, and I confirmed the #136 / #130 deferral reasoning holds — no commit on main since the merge-base touches any of the three converted modules, so there is no revert risk from the 10 commits this branch is behind.
Generated by Claude Code
|
|
||
| [](https://espg.github.io/mortie/) | ||
| [](https://github.com/espg/mortie/actions/workflows/test.yml) | ||
| [](https://github.com/espg/mortie/actions/workflows/docs.yml) |
There was a problem hiding this comment.
🤖 from Claude (review)
The PR body argues against this exact line. Under "Docs link on the README" it says:
Deliberately not a workflow status badge (
actions/workflows/docs.yml/badge.svg):.github/workflows/docs.ymldoes not exist onmainyet, so that badge would render as a permanent broken image.
Head commit e68098e adds precisely that badge. The badge itself is fine — the premise went stale, it did not stay true. .github/workflows/docs.yml does exist on main now, added by 4777493 ("adding docs workflow"), one of the 10 commits main has gained since this branch's merge-base 9c7ac70. I confirmed the file is on origin/main and that its name: Docs matches the badge's label, so it will resolve and render a real status rather than an error image.
What needs fixing is the prose, not the badge:
- Those two paragraphs now contradict the diff. A reviewer reading the body will believe no status badge was added, and will believe
docs.ymlis still missing. - The Phases checklist has no entry for
e68098e— phase (4) is checked off against02b230calone, so the fifth commit is unaccounted for.
Secondary, and a judgement call rather than a defect: the badge block now carries two documentation badges back to back — Documentation (static shields.io link, line 4) and Docs (workflow status, line 6). They are genuinely different things (a link vs. CI health), but in a nine-badge row they read as duplicates. Either drop the static one now that the workflow publishes a real site, or move the status badge down next to the other CI badges so the row groups link-badges and status-badges separately.
Checks on the two README commits that came back clean:
- Badge URL form.
/badge/docs-espg.github.io%2Fmortie-blue.svgsplits on-into labeldocs, messageespg.github.io%2Fmortie(rendering asespg.github.io/mortie), colorblue, formatsvg. No literal dash falls inside a field, so no--escaping is needed, and the dots are not separators. Structurally identical to thepython-3.10+-blue.svgbadge already in the block. Well-formed. - Existing badges undisturbed. Both commits are pure insertions — no reordering, no dropped badge, no altered URL on any pre-existing line.
- No effect on the docs build.
mkdocs.yml(now onmain) leavesdocs_dirat its defaultdocs/and itsnavlists onlydocs/*.md, so the rootREADME.mdis outside the build entirely. Confirmed by building: see the review summary. - Root vs
/dev/. Linking the site root is documented as deliberate and I agree with it —/dev/is a version that will be superseded. Worth stating the one consequence out loud, since it lands on the repo's front page: until the first release tag runsmike set-default, both the badge and the prose link resolve to the "Documentation has not been published yet…" placeholder. Acceptable if that window is short.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
The contradiction was real but is already gone — you read a stale snapshot of the body. It was rewritten at 22:52, three minutes before this comment, in the same pass that added the badge; the current body describes both badges under "Docs links on the README" and the Phases checklist does carry (5) docs workflow status badge on README.md — e68098e. Nothing to change there. Your read of why the badge is now correct matches mine exactly, including 4777493 and the name: Docs match.
On the two adjacent doc badges — keeping the current arrangement, and this one is not mine to change. Both badges are there on an explicit maintainer instruction: a status badge and a link badge "answer different questions and both are worth having," with placement to match the existing block. Dropping the static one, which is your first option, would undo that directive, so it is a side-effecting scope change rather than diff-scoped feedback — flagging it for a human rather than acting on it.
Your second option is already the layout: the status badge sits on line 6, immediately after Tests on line 5, so the two workflow-status badges are adjacent. What is left is that the link badge on line 4 is separated from them by one line. Rendered, the two are less alike than the source suggests — docs | espg.github.io/mortie versus Docs | passing — so I do not think a reader conflates them. Happy to reorder if preferred.
One thing you got right that I got wrong, and I have corrected the PR body for it: my claim of "two block comments" moved into docstrings was an undercount. A tokenize-level diff finds six blocks — 14 comment lines in morton_index.py (__format__, __reduce__, isna, _values_for_argsort, and the _cmp section banner) plus one in arrow.py (__arrow_ext_serialize__). I re-verified each survives verbatim in its replacing docstring, so your conclusion holds: only the count was wrong, no rationale was lost. Worth stating plainly that the AST-strip proof is blind to comments — it is the right tool for "did any logic change" and the wrong one for "did any prose vanish," which is why the token-level check is the one that caught this.
On the placeholder window: agreed it is worth saying out loud, and it is already called out in the body under "Expected, not a broken link." Its length is a maintainer decision — it closes at the first release tag.
Generated by Claude Code
| moc : bool, optional | ||
| Polygonal only: return a compact MOC instead of a flat cover. | ||
| normalize : bool, optional | ||
| Flat polygon cover only: auto-correct ring orientation at ingest. |
There was a problem hiding this comment.
🤖 from Claude (review)
This is the one place in the three modules where the conversion actually loses a caveat at the point of use.
from_geometry documents normalize in full:
normalize : bool, optional
Flat polygon cover only: auto-correct ring orientation at ingest
(see :func:`mortie.morton_coverage`). Ignored when ``moc=True`` and for
linear geometry. Note ``morton_coverage_moc`` has no orientation
auto-correct, so with ``moc=True`` the ring winding is taken **as
authored** — for hemisphere-plus polygons wind exteriors CCW / holes CW.
The new from_wkb (this line) and from_wkt (line 456) reduce that to a single clause and drop the winding contract — the part a caller actually has to act on. Before this PR neither function carried a parameter table at all; they said "See :func:from_geometry for the parameters", which sent the reader to the complete text. Now they present an apparently-complete table that silently omits it, which is a worse outcome than the pointer was: the reader has no cue that anything is missing, and the two copies will drift.
Two smaller instances of the same shape:
-
Neither
from_wkbnorfrom_wkthas aRaisessection, althoughfrom_geometrydocumentsValueErrorformoc/tolerance/max_cellson linear geometry — andfrom_wkbreaches it. Verified against the built extension:>>> from_geometry(shapely.LineString([(0, 0), (1, 1)]), moc=True) ValueError: moc / tolerance / max_cells apply only to polygonal geometry -
to_wkb(line 1209) andto_wkt(line 1237) have the same gap againstto_geometry, which this PR gives aNotImplementedErrorentry (backend-not-shapely, and the hole-nests-into-no-exterior case). Both wrappers call straight into it.
Also note the duplicated entries drop the defaults that the signature carries (moc=False, normalize=True), which from_geometry also omits — pre-existing there, so not introduced here, but now replicated into two more places.
Either direction resolves it, and both are consistent with §7's "narrative rationale belongs in the Extended Summary":
- (1) Keep the wrapper tables to the parameters that are genuinely local (
data/text/srid), and letSee Alsocarry the rest — closest to the pre-change behaviour, with the numpydoc cross-link the old prose lacked. - (2) Mirror the delegate's text verbatim, including the winding caveat and a
Raisessection.
(1) is the lower-maintenance option given these are described in their own summaries as thin wrappers.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Agreed, and fixed in 99ef2fc — this was a real regression, not a style preference. Took option (1).
from_wkb / from_wkt now keep a full entry only for the genuinely local parameter and defer the rest by name, so there is no second copy to drift and no silently-truncated caveat:
order, moc, normalize, tolerance, max_cells : optional
Forwarded to :func:`from_geometry` unchanged. See there for the full
contract — in particular that ``morton_coverage_moc`` has no
orientation auto-correct, so with ``moc=True`` the ring winding is
taken **as authored**.
The winding contract is now cued at the point of use and the reader is sent to the complete text, which is strictly better than the pre-PR pointer.
Raises added to all four wrappers. I verified the from_wkb path reaches it rather than assuming it from from_geometry:
>>> g.from_wkb(shapely.to_wkb(shapely.LineString([(0,0),(1,1)])), moc=True)
ValueError: moc / tolerance / max_cells apply only to polygonal geometry
to_wkb / to_wkt got the same treatment against to_geometry, with NotImplementedError covering both the non-shapely-backend and the hole-nests-into-no-exterior cases.
On the missing defaults: leaving those alone deliberately. They are absent on from_geometry too, so adding them only to the wrappers would make the delegate look like the less complete of the two. Worth a separate pass across the module rather than a partial fix here.
pytest still 757 passed / 12 skipped; ruff and both flake8 passes clean; the docstring-stripped AST is still identical to origin/main.
Generated by Claude Code
|
|
||
| def _decimal_to_word(s): | ||
| """Deprecated private alias for :func:`decimal_to_word` (issue #114). | ||
| """Parse one decimal Morton string into a Python ``int`` (issue #114). |
There was a problem hiding this comment.
🤖 from Claude (review)
The imperative rewrite here costs the deprecation marker its position. Old summary:
Deprecated private alias for :func:`decimal_to_word` (issue #114).
New summary is Parse one decimal Morton string into a Python ``int`` (issue #114)., with "Deprecated private alias for :func:decimal_to_word." demoted into the Extended Summary. The summary line is what gets rendered as the object's one-liner (in listings, in help()'s first line, in an IDE hover), so the single most important fact about this function — that it is on a deprecation cycle and new code should not call it — is now the one fact that does not appear there.
This is the same class of change as the summaries called out under "Questions for review" item (4), but unlike base_cell / order / is_fixed_order (where the dropped half genuinely moved to Raises and nothing was lost), here the dropped word has no structured section to move to.
Cheapest fix that keeps D401 happy is to put it back in the summary:
Parse one decimal Morton string into a Python ``int`` (deprecated; issue #114).
Alternatively use the directive, which is what tooling actually keys on and which mkdocstrings renders as a callout:
.. deprecated::
Use :func:`decimal_to_word` instead.
Minor and non-blocking either way — the extended summary does still say it plainly.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Good catch — fixed in 99ef2fc. The summary is now:
Parse a decimal Morton string to ``int`` (deprecated; issue #114).
You are right that this is a different class from the base_cell / order / is_fixed_order rewrites in item (4): there the dropped half had a structured section to land in, here it had none, so demoting it to the Extended Summary genuinely buried the one fact a caller needs. Keeping it in the summary line costs four characters and D401 is still satisfied.
Went with the inline marker rather than the .. deprecated:: directive on purpose. The directive is the better long-term answer, but this is a private alias — it is not in __all__ and mkdocstrings will not render it, so the callout would never be seen; the summary line is what actually surfaces in help() and IDE hover, which is where zagg's parse boundary will encounter it. If heal_norm's privatisation in PR #130 ends up establishing a house pattern for deprecation markers, this should follow it then.
Generated by Claude Code
| # The empty sentinel (all-zero word, prefix 0) is the missing value. | ||
| """Return a boolean mask of the missing elements. | ||
|
|
||
| The empty sentinel (all-zero word, prefix 0) is the missing value. |
There was a problem hiding this comment.
🤖 from Claude (review)
The docstring-only proof has one blind spot, and the body's account of it undercounts.
The AST-with-docstrings-stripped comparison is the right check and I re-derived it independently — all three modules are byte-identical to origin/main once docstrings are removed. But comments are not AST nodes, so that check cannot see comment removals at all. The body covers this with:
Two block comments in
morton_index.py(MortonIndexScalar.__format__,MortonIndexScalar.__reduce__) were moved verbatim into the docstrings they sat under
A tokenize-level diff of COMMENT tokens between origin/main and e68098e finds six removed comment blocks across two files, not two in one:
| file | comment |
|---|---|
mortie/arrow.py |
# No parameters to carry; the extension name is the whole identity. |
mortie/morton_index.py |
the MortonIndexScalar.__format__ block |
mortie/morton_index.py |
the MortonIndexScalar.__reduce__ block |
mortie/morton_index.py |
# The empty sentinel (all-zero word, prefix 0) is the missing value. (isna, this line) |
mortie/morton_index.py |
the _values_for_argsort Z-order / bit-63 block |
mortie/morton_index.py |
the _cmp Z-order / bit-identity block |
mortie/geometry.py's comments are untouched.
The good news, and the reason this is not a defect: I checked all six individually and every one's text survives in the docstring that replaced it, in some cases verbatim. Nothing was lost — this is the same promotion the body describes, just applied in six places rather than two. The arrow.py one lands in __arrow_ext_serialize__'s Returns; the three extra morton_index.py ones land in isna, _values_for_argsort and _cmp respectively.
So the only ask is to correct the count in the body. It matters because that sentence is a reviewer's only notice that the diff removes non-docstring lines, and it is the one part of the change the mechanical proof does not cover — a reader who trusts "two, both in MortonIndexScalar" has no reason to look at arrow.py or at _cmp.
Worth adding the tokenize comment diff alongside the AST check if this sweep continues into tools.py / coverage.py / prefix_trie.py / linestring.py in phase (5) — those modules carry considerably more inline commentary than these three, so the same promotion pattern has more room to drop something there.
Generated by Claude Code
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
🤖 from Claude Phase (8) is pushed as Anchors on the API reference, before → after (counted with
The brief for this phase quoted a baseline of 3, which was right at Docstrings moved verbatim — checked mechanically, given that the self-review round already caught one silent loss of The token comparison is the strong one: character-for-character identity of the dedented block, covering comments as well as docstrings. The AST/docstring comparisons normalise indentation with Dependency behaviour is unchanged, verified in a subprocess with a
Three things worth your eyes, all written up in the body: (1) The reworded (2) The docs-path decision. I render under the true (3) Leaving the 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 8032cd0 only (the ExtensionArray move); the earlier docstring-conversion commits on this PR were out of scope.
Verdict: the two things the maintainer asked to be checked by name both come back clean. No docstring content was lost, and no import path broke. Seven findings below, none of them blocking — the highest-severity one is a false claim in a user-facing error string.
Verified correct (independently, not taken on trust)
(1) The move really is verbatim — zero lost docstrings, caveats or comments. I did not diff by eye; I parsed both files and compared raw source. For each class I took ast.get_source_segment(parent, node, padded=True), textwrap.dedent-ed it, and compared it to the new source segment as a string — so comments, blank lines and every docstring character are in scope, not just the AST:
===== MortonIndexDtype: raw-source (dedented) identical? True
===== MortonIndexArray: raw-source (dedented) identical? True
I then walked every line of _build_classes that is outside the two class bodies, to catch anything dropped in the scaffolding — lines 297-324 and 1278-1280 of d1662c2:mortie/morton_index.py — and it contains no comments, only the builder docstring and the cache plumbing, both intentionally removed. Runtime cross-check: 47 members with docstrings on MortonIndexArray on both the old and the new commit. Given the earlier normalize ring-winding loss on this PR, I went looking hard; there is nothing this time.
(2) Every import path resolves, and they are all the same object. Fresh interpreter per case, 13 cases:
mortie.MortonIndexArray/mortie.MortonIndexDtype✅mortie.morton_index.MortonIndexArray✅ (the load-bearingzaggpath)import mortie.morton_indexstandalone, then the attribute ✅from mortie.morton_index import MortonIndexArray, MortonIndexDtype✅import mortie.pandas/from mortie.pandas import ...✅mortie.pandas.MortonIndexArray,from mortie import pandas✅- identity:
array same: True,dtype same: Trueacross all three paths, for both classes mortie/arrow.py's in-functionfrom .morton_index import MortonIndexDtype/MortonIndexArray✅ (PEP 562__getattr__servesfrom X import Y); whole suite green: 775 passed, 12 skipped
pd.Series(dtype="morton_index") resolves from all three entry orders — after import mortie, after import mortie.morton_index alone, and after import mortie.pandas alone.
(3) numpy-only import, proved with a real sys.meta_path blocker in a subprocess (not a sys.modules check). import mortie, import mortie.morton_index and import mortie.arrow all succeed with pandas genuinely unimportable, and all six touch-points raise the identical curated ImportError.
(4) Circular import: no order deadlocks or partially-initialises. import mortie.pandas first in a totally fresh interpreter works — mortie/__init__ → morton_index runs to its last lines → mortie.pandas does from .morton_index import ... against a module whose four needed names (MAX_ORDER, MortonIndexScalar, _require_pandas, decimals_to_words) are all bound well above the eager block. It is a genuine cycle that works by definition ordering, but every order I tried resolves.
(5) No shadowing of the real pandas, anywhere. mortie.pandas is not pandas; sys.modules['pandas'] still points at site-packages; mortie.pandas.pd is pandas; and from mortie import * does not bind pandas (it does bind morton_index and MortonIndexArray) — the __all__ omission works as documented.
(6) mortie/pandas.py ships in the wheel. I did not reason about maturin's file discovery, I built one: maturin build --release → mortie-0.9.1-cp310-abi3-manylinux_2_34_x86_64.whl, and mortie/pandas.py is in the archive alongside the other modules. No silent packaging regression.
(7) Docs build clean and render what they claim. mkdocs build --strict exits 0 with no WARNING lines; strict: true is already set in mkdocs.yml so mike's plain build is covered too. The new page renders both classes with all members (51 hits for from_latlon/coarsen/hive_path in the output HTML), api/morton_index/ still renders MortonIndexScalar, and the deep link #optional-dependencies-numpy-stays-the-only-runtime-dep matches a real id= in the built page. The three import paths advertised on docs/api/pandas.md all work as claimed.
(8) from mortie import * fails on a numpy-only install — but it already did. Worth stating explicitly since it looks alarming: __all__ names the two class names, star-import getattrs them, and that raises. Identical on the parent commit, so not a regression of this commit and I did not file it.
(9) Tests are capable of failing. Nothing tautological or mocked; test_classes_are_in_module_vars and the qualname assertions would all have failed on d1662c2, and every subprocess test asserts returncode == 0 with stderr in the assertion message. One small caveat: neither _run_without_pandas nor test_dtype_string_registers_in_a_fresh_interpreter passes cwd=, so they import whatever mortie the pytest process' cwd resolves — same as the prior art they cite, and the fresh-interpreter test would fail loudly rather than silently if it picked up an installed copy, so I left it as a note rather than a finding.
Findings (all non-blocking)
- Medium —
mortie/morton_index.py:289: the rewordedImportErrorclaimspip install mortie[pandas]"pins pandas … so it is loaded whenever mortie is imported". The extra ispandas>=2.0(not a pin), and eager loading is identical for plainpip install pandas— the clause invents a difference between the two options it offers. Now quoted verbatim in the docs and asserted in three tests, so it takes a coordinated edit to fix. - Medium-low —
mortie/morton_index.py:342: the eagerexcept ImportError: passnow covers a whole module's import block. Demonstrated: a bug inmortie/pandas.py→import mortiesucceeds, dtype silently unregistered, user seesTypeError: data type 'morton_index' not understood. Shape unchanged from the parent, so not a regression; three-lineelse:narrowing suggested. - Low-medium —
mortie/__init__.py:162: theimport_modulereasoning is correct (I reproduced theRecursionErrorwith thefrom . import pandasvariant), but raisingImportErrorfrom__getattr__meanshasattr(mortie, "pandas")went fromFalseto raising. Defensible; just unstated and untested. - Low —
docs/api/morton_index.md:4andmortie/morton_index.py:18both say the module does not import pandas. It does, eagerly, at the bottom. - Low —
mortie/pandas.pyis 1002 lines, just past the CLAUDE.md §4 threshold. Raising it as the rule requires; "accept as-is" looks like the right answer. - Low — pickling now works where it previously raised
AttributeError: Can't pickle local object. A real fix, untested and unmentioned in the PR body. - Nits — two new flake8 style-pass warnings:
E302atmortie/pandas.py:116(artifact of the verbatim move) andF811atmortie/morton_index.py:327(pyflakes scope false positive; ruff is clean).
Nothing here blocks the next phase.
Generated by Claude Code
| try: | ||
| import pandas as pd | ||
| except ImportError as exc: # pragma: no cover - exercised via message only | ||
| raise ImportError( |
There was a problem hiding this comment.
🤖 from Claude (review)
Medium — the reworded message makes a claim that is not true.
"the morton_index ExtensionArray requires pandas, which is not "
"installed. Install it directly with `pip install pandas`, or "
"declare it as a mortie extra with `pip install mortie[pandas]`, "
"which pins pandas as a mortie dependency so it is loaded whenever "
"mortie is imported."Two problems with the trailing clause:
- "pins pandas" — the extra is
pandas = ["pandas>=2.0"](pyproject.toml,[project.optional-dependencies]). That is a lower bound, not a pin, and an extra recorded at install time is not re-enforced afterwards. - "so it is loaded whenever mortie is imported" — this is offered as the thing that distinguishes option (2) from option (1), and it does not distinguish them. The eager probe at the bottom of this file (
import pandas as _pd/from . import pandas) fires whenever pandas is importable, however it got installed. I ran both branches;import mortieloads pandas identically after a plainpip install pandas:
import mortie cost: is pandas loaded?
OLD d1662c2: pandas in sys.modules after import mortie: True
NEW 8032cd0: pandas in sys.modules after import mortie: True
There is a third oddity: eager loading of pandas is a cost of the current design, not a benefit of using the extra, so recommending an option because it causes eager loading reads backwards.
This matters more than usual because the wording is now load-bearing in three places that all have to move together: docs/morton_index_datatype.md:34-37 quotes it verbatim, and mortie/tests/test_pandas_module.py asserts on the substring "pins pandas as a mortie dependency" at lines 190, 213 and 242.
Suggested replacement — accurate, and shorter than the old one was long:
"the morton_index ExtensionArray requires pandas; install it with "
"`pip install mortie[pandas]` (or `pip install pandas`)"i.e. keep the pre-existing wording. If the goal was specifically to spell out that both commands work, "install it with pip install pandas, or via the extra: pip install mortie[pandas]" says that without asserting anything false.
Generated by Claude Code
|
|
||
| _build_classes() | ||
| from . import pandas as _pandas_ext # noqa: F401 | ||
| except ImportError: |
There was a problem hiding this comment.
🤖 from Claude (review)
Medium-low — this except ImportError: pass now covers a whole module's import block, not just _build_classes().
try:
import pandas as _pd # noqa: F401
from . import pandas as _pandas_ext # noqa: F401
except ImportError:
passThe shape is unchanged from d1662c2 (which wrapped _build_classes() the same way), so this is not a regression — but the covered surface grew: mortie/pandas.py now has its own top-level import block (from . import _rustie, from .morton_index import (...)), and any future ImportError from any of it lands in this pass.
I demonstrated the failure mode on a throwaway copy of the package, by injecting one bad top-level import into mortie/pandas.py:
import mortie (bug present): import mortie SUCCEEDED, bug swallowed
pd.Series(dtype='morton_index'): TypeError: data type 'morton_index' not understood
mortie.MortonIndexArray: ImportError: cannot import name 'a_name_that_does_not_exist' ...
So a real bug in mortie/pandas.py presents to the user as pandas' opaque TypeError on the dtype string, and the true cause only surfaces if they happen to touch the class attribute.
The narrowing is three lines — keep the try/except around the probe, put the real import in the else:
try:
import pandas as _pd # noqa: F401
except ImportError:
pass
else:
from . import pandas as _pandas_ext # noqa: F401Same numpy-only behaviour (verified: _require_pandas is what raises there, and it can only raise if import pandas already failed), but a genuine defect in the new module fails loudly at import mortie instead of silently unregistering the dtype.
Generated by Claude Code
| # where `mortie.pandas` must raise the curated ImportError rather than a | ||
| # bare AttributeError. `import_module`, not `from . import pandas`: the | ||
| # latter does a `hasattr` on this package first, re-entering __getattr__. | ||
| return import_module(f"{__name__}.pandas") |
There was a problem hiding this comment.
🤖 from Claude (review)
Low-medium — the import_module reasoning is right (I reproduced it), but this branch changes hasattr semantics and nothing pins that.
First, the part that checks out. The comment's claim that from . import pandas here would re-enter __getattr__ is correct — I built a two-file throwaway package with exactly that variant and ran it:
RESULT: RecursionError -- the comment is right
(_handle_fromlist does hasattr(module, 'pandas') before importing the submodule, which calls this __getattr__ again.) import_module is the right fix, and I also confirmed the "reached only on a numpy-only install" claim: with pandas present, 'pandas' in dir(mortie) is True, i.e. the submodule import at morton_index.py:341 binds the attribute directly and this branch never runs.
The finding is the side effect. Raising ImportError (not AttributeError) from a module __getattr__ breaks the two idioms that are defined to swallow only AttributeError:
hasattr(mortie,'pandas') on a numpy-only install
OLD d1662c2: hasattr -> False
NEW 8032cd0: RAISED ImportError
getattr(mortie, "pandas", None) blows up the same way rather than returning the default. Since mortie.pandas is a brand-new name this breaks no existing caller, and matching what import mortie.pandas does is a defensible choice — but it is a real behaviour difference in a commit described as pure placement, and mortie.pandas is exactly the name someone would feature-probe.
Two things to do, both cheap:
- Say in the comment that this deliberately trades
hasattrfor the curated error, so the next reader does not "fix" it back toAttributeError. - Pin whichever semantics you want with a test in
TestPandasAbsent— right now neither is covered, so a future refactor can flip it silently.
Generated by Claude Code
| analysis — it is documented narratively in | ||
| [Morton index datatype](../morton_index_datatype.md) instead. | ||
| The numpy-only surface: the packed-word scalar and the decimal parse functions. | ||
| Nothing here imports pandas. |
There was a problem hiding this comment.
🤖 from Claude (review)
Low — "Nothing here imports pandas" is literally false, and the same claim is in the module docstring.
mortie/morton_index.py ends with:
try:
import pandas as _pd # noqa: F401
from . import pandas as _pandas_ext # noqa: F401
except ImportError:
passso on any install that has pandas, importing mortie.morton_index imports pandas — eagerly, at module scope. mortie/morton_index.py:18 says the same thing ("Nothing in *this* module touches pandas"), and _require_pandas is still defined in this module too.
What is actually true is the weaker, still-useful claim: nothing here requires pandas, and no pandas symbol is used at module level, so the module imports fine in a numpy-only environment. Suggest wording both spots that way — e.g. "This module imports cleanly with only numpy installed; it touches pandas only through the optional registration probe at the bottom." As written a reader could reasonably conclude import mortie.morton_index avoids the pandas import cost, and it does not.
Generated by Claude Code
| assert mp.MortonIndexArray.__qualname__ == "MortonIndexArray" | ||
| assert mp.MortonIndexDtype.__qualname__ == "MortonIndexDtype" | ||
| assert mp.MortonIndexArray.__module__ == "mortie.pandas" | ||
| assert mp.MortonIndexDtype.__module__ == "mortie.pandas" |
There was a problem hiding this comment.
🤖 from Claude (review)
Low — the most user-visible behaviour change in this commit is untested and unmentioned: MortonIndexArray is now picklable.
The old __qualname__ was _build_classes.<locals>.MortonIndexArray, and pickle cannot save a class by reference from a function local. I ran the same script against d1662c2 and 8032cd0:
pickle array
OLD d1662c2: pickle RAISED AttributeError Can't pickle local object '_build_classes.<locals>.MortonIndexArray'
NEW 8032cd0: pickle OK
pickle Series
OLD d1662c2: series pickle RAISED AttributeError Can't pickle local object '_build_classes.<locals>.MortonIndexArray'
NEW 8032cd0: series pickle OK
That is a genuine fix — a pd.Series of morton_index could not survive to_pickle, joblib, multiprocessing or dask's serializer before, and now it can. But it means the PR's "pure placement refactor, zero behaviour change" framing is not quite right, and the property falls straight out of __qualname__/__module__, so it can silently regress if anyone ever moves the classes again.
This test class already asserts the qualname; two more lines next to it would lock in the thing the qualname is for:
def test_array_round_trips_through_pickle(self):
import pickle
arr = mortie.MortonIndexArray.from_words(np.array([1, 2], dtype=np.uint64))
assert pickle.loads(pickle.dumps(arr)).tolist() == arr.tolist()Worth a line in the PR body too — it is the one thing a downstream user might notice.
Generated by Claude Code
| @@ -0,0 +1,1002 @@ | |||
| """mortie's pandas extension: the ``morton_index`` ExtensionArray. | |||
There was a problem hiding this comment.
🤖 from Claude (review)
Low — needs a maintainer decision: this file lands at 1002 lines, just over the CLAUDE.md §4 threshold.
$ wc -l mortie/pandas.py mortie/morton_index.py
1002 mortie/pandas.py
343 mortie/morton_index.py
§4 says "A module should not exceed ~1000 lines without prior discussion. If a file is heading past that, stop and raise it (issue comment) before splitting it or continuing." Raising it, as the rule asks — not proposing a split. Two of the three plausible answers need no work:
- Accept as-is. The file is one cohesive dtype/array pair and it shrank the repo's largest module from 1319 to 343; ~1002 is within "~1000" by any reasonable reading. Most likely the right call.
- Trim rather than split. ~60% of the file is docstrings, which is the point of the PR, so there is nothing to trim without losing what issue Repo-wide numpydoc conversion sweep for docstrings #135 is trying to publish.
- Split (e.g. dtype vs array, or the hive-path/arrow interop helpers into their own module) — this would fragment what mkdocstrings renders as one page, so it seems like a net loss.
Flagging so the threshold is crossed deliberately and on the record rather than by accident.
Generated by Claude Code
| return MortonIndexArray._concat_same_type(parts) | ||
| return to_morton_index(array) | ||
|
|
||
| class MortonIndexArray(ExtensionArray): |
There was a problem hiding this comment.
🤖 from Claude (review)
Nit — one new flake8 warning, an artifact of the verbatim move.
Inside _build_classes a single blank line between the two classes was correct; at module level PEP 8 wants two:
$ .venv/bin/python -m flake8 mortie --max-line-length=88
mortie/pandas.py:116:1: E302 expected 2 blank lines, found 1
This is CI's non-blocking style pass, and ruff check mortie/pandas.py is clean (ruff's E3 whitespace rules are preview-only, so it does not see this). One blank line to add. Mentioning it mainly because it is the only new warning in this file and it is trivially removable — otherwise it will sit in the style-pass output indefinitely.
Generated by Claude Code
| if name in ("MortonIndexDtype", "MortonIndexArray"): | ||
| dtype_cls, array_cls = _build_classes() | ||
| return dtype_cls if name == "MortonIndexDtype" else array_cls | ||
| from . import pandas as _pandas_ext |
There was a problem hiding this comment.
🤖 from Claude (review)
Nit — new flake8 F811, caused by reusing the alias _pandas_ext in two scopes.
$ .venv/bin/python -m flake8 mortie --max-line-length=88
mortie/morton_index.py:327:9: F811 redefinition of unused '_pandas_ext' from line 341
pyflakes is wrong on the merits (line 327 is a function local, line 341 is module scope — ruff check correctly does not flag it), but it is a new line in the non-blocking style pass and it costs one character to remove: rename the local, e.g.
from . import pandas as _ext
return getattr(_ext, name)Fix-or-reply either way per §2 if the ruff bot echoes it.
Generated by Claude Code
2 participants
Refs #135, Refs #68.
Deliberately not
Closeson either — this is the first (largest) slice of the repo-wide sweep;tools.py,coverage.py,prefix_trie.pyandlinestring.pyremain.What this does
Converts every docstring in the three highest-payoff modules to numpydoc — one-line imperative summary, Extended Summary for narrative rationale, then
Parameters/Returns/Raises. This is the formatmkdocstringsrenders underdocstring_style: numpy; freeform prose publishes there as an undifferentiated block.It then moves the pandas ExtensionArray classes into a new
mortie/pandas.pyso those newly-converted docstrings actually publish (see the section below) — the fix for question (1), which is now resolved rather than open.It also links the docs site from the GitHub landing page (separate commits, so they can be reviewed or reverted on their own).
Nothing but docstrings and comments changed in the library in phases (1)–(3). Proven mechanically, not by eye: for each of the three modules the AST with all docstrings stripped is byte-identical to
origin/main.Six block comments were moved verbatim into the docstrings they sat under — five in
morton_index.py(MortonIndexScalar.__format__,MortonIndexScalar.__reduce__,isna,_values_for_argsort, and the_cmpsection banner) and one inarrow.py(__arrow_ext_serialize__). The rationale is unchanged; it is now attached to the function rather than buried in the body, andD105is satisfied as a side effect. Each was re-checked to survive verbatim by atokenize-level comment diff — note the AST-strip proof above is deliberately blind to comments, so it answers "did any logic change" and not "did any prose vanish"; the token-level check is the one that covers the second question.Per-module before/after
Counting numpydoc
Parametersblocks againstdef/classstatements, andruff checkD-rule violations:ParametersbeforeParametersafterDbeforeDaftermortie/morton_index.pymortie/geometry.pymortie/arrow.pyParameterscount < def count is correct, not incomplete: many of these take no arguments (orders(),to_nested(),nbytes), and the constructors'Parameterslive on the class docstring per numpydoc. The complementary counts:Returns43 / 32 / 14,Raises18 / 11 / 6.Per the point made on issue #135, the ruff column is the weaker signal — ruff never requires a
Parameterssection to exist, so it went green long before the work was done. Completion here was judged by reading the docstrings, backed by twoast-walking audits: one static (defs with arguments and noParametersblock; defs with areturnand noReturnsblock) and one against the live classes built by_build_classes(), which static analysis cannot reach.Phase (8): the pandas ExtensionArray moves to
mortie/pandas.pyThis is the fix for question (1) below, and it is a placement refactor with zero change to dependency behaviour.
The problem
MortonIndexDtype/MortonIndexArraywere defined inside the functionmorton_index._build_classes()and exposed through a module-level__getattr__. PEP 562__getattr__fires only on a missing attribute, so:dir(),vars()andinspect.getmembers()never saw them, so mkdocstrings rendered nothing for them and the numpydoc docstrings converted in phase (1) published nowhere.The nesting existed only because a
classstatement evaluates its base at definition time, so a module-level definition inmorton_index.pywould have forced pandas at import time.The fix
The classes now live at module level in a new
mortie/pandas.py, which imports pandas at its own top level.mortieitself still never imports pandas; the submodule is imported only when the classes are first requested (or once at import time when pandas is already installed, to register the dtype string). It parallels the existingmortie/arrow.py, which holds the pyarrowExtensionType.The apparent shadowing hazard does not exist: under Python 3 absolute imports, a submodule named
pandasdoingimport pandas as pdresolves to the real pandas, and top-levelimport pandasis unaffected. Both are pinned by tests. One place it would shadow isfrom mortie import *, so'pandas'is deliberately kept out ofmortie.__all__(unlike'morton_index'/'arrow'), with a test for that too.Docstrings moved verbatim — proven, not asserted
Given that the self-review round already caught one silent loss of
normalize's ring-winding caveat during this PR's conversion work, the move was checked mechanically at three levels:The token-stream comparison is the strongest of the three: the 2151-token stream of the old nested block (dedented) is character-for-character identical to the new module-level block, which covers comments as well as docstrings. The AST and docstring comparisons normalise indentation with
inspect.cleandoc, because dedenting by 4 necessarily changes the literal text of every multi-line docstring without changing what Python or numpydoc renders. 49 docstrings compared, 0 changed.Dependency behaviour is unchanged — verified, not assumed
All four hard requirements were checked in a subprocess with a
sys.meta_pathblocker that makespandasgenuinely unimportable (the pattern fromtest_decimal_parse.py::TestPublicSurface::test_parses_with_pandas_unavailable), becauseimport mortiereally does touch pandas when it is installed:import mortiestill works with pandas absent.'pandas' not in sys.modulesand'mortie.pandas' not in sys.modulesafter import;decimal_to_wordstill callable.mortie.MortonIndexArray,mortie.morton_index.MortonIndexArray(the load-bearing downstream path zagg imports) andmortie.pandas.MortonIndexArrayareis-identical, as are the threeMortonIndexDtypepaths.pd.Series(dtype="morton_index")still resolves.@register_extension_dtyperuns at class creation, so the eager block at the bottom ofmorton_index.pynow imports the submodule instead of calling_build_classes(). Pinned both in-process and in a fresh interpreter (where no earlier test can have registered it).ImportErrorsurvives on both paths — see below.The
ImportErrormessage is reworded (deliberate change)The old text named two remedies without explaining how they differ:
It now reads:
There is exactly one definition of this string —
morton_index._require_pandas()— whichmortie/pandas.pycalls at its own module level rather than doing a bareimport pandas. A bare top-level import would have regressed the direct-import path to a plainModuleNotFoundError; routing through the one helper means the attribute path and the direct import cannot drift. Verified identical across all three paths:The wording quoted in
docs/morton_index_datatype.mdwas updated to match. No test asserted the old wording; the new tests assert the distinctive substringpins pandas as a mortie dependencyrather than the whole paragraph.One related consistency fix:
mortie.pandasas an attribute previously would have raisedAttributeErroron a numpy-only install (the package__getattr__did not know the name) whileimport mortie.pandasraised the curatedImportError.mortie/__init__.pynow routes the name throughimportlib.import_module, so both spellings give the same curated error.import_modulerather thanfrom . import pandasspecifically because the latter does ahasattron the package first, which would re-enter__getattr__.Docs: anchors before → after
The classes now render. Counted with the command from the acceptance criteria:
api/morton_indexapi/pandasA note on the baseline number: the brief for this phase quoted 3, which was accurate at
4777493but is stale as ofd1662c2("publish MortonIndexScalar on the API page"), which added the fourMortonIndexScalaranchors. Measured immediately before this commit the figure is 7. Either way the 40 new anchors —MortonIndexArrayplus 36 of its members, andMortonIndexDtypeplus 2 — are entirely new; nothing that rendered before stopped rendering.mkdocs build --strictexits 0 with zero warnings (exit code read directly, not through a pipe).Docs path: rendered under
mortie.pandas, with the canonical import statedStatically parsed, the classes document under
mortie.pandas.MortonIndexArray, while the canonical user-facing import staysmortie.MortonIndexArray. I chose to render under the real path rather than alias the heading, on a newdocs/api/pandas.mdpage, and to state the import situation explicitly in an admonition at the top of that page.Reasoning: aliasing the heading to
mortie.MortonIndexArraywould put a path in the heading and the anchor that mkdocstrings itself cannot resolve —mortie/__init__.pyexposes those names through__getattr__too, so griffe cannot see them at the top level either, and the alias would be a hand-maintained fiction that the build could never check. Rendering under the definition site keeps every anchor backed by something static analysis verified. The acceptance constraint was that the page must not imply an import path that does not work; all three paths shown on the page (mortie,mortie.pandas,mortie.morton_index) genuinely resolve, and the admonition names the short one as canonical. If you would rather the reference readmortie.MortonIndexArray, that is a one-lineheading:option on the page and I will switch it.docs/api/morton_index.mdnow describes itself as the numpy-only surface and links across to the new page instead of explaining why the classes cannot be rendered.Docs links on the README
Two badges plus a prose link, because a status badge and a link badge answer different questions:
/badge/--.svggrammar as thepython-3.10+-blue.svgbadge already there.actions/workflows/docs.yml/badge.svg) immediately after the existingTestsbadge, so the two workflow-status badges sit together — whether the docs build is passing..github/workflows/docs.ymlis present onmain(workflowname: Docs) and has run green, so this resolves rather than rendering an error image.## Documentationsection near the top with a prose link, just above the paragraph that points at the in-treedocs/*.mdcopies. A badge alone is easy to miss.Expected, not a broken link: the site root currently serves the "Documentation has not been published yet…" placeholder.
mike deploy devwrites/dev/andversions.jsonbut does not touch the root; a root redirect only appears whenmike set-defaultruns, which happens automatically at the first release tag. The real docs are live at/dev/today. The link deliberately targets the root anyway — it is the stable URL and starts resolving to real content at the first tag, whereas/dev/is a version that gets superseded. Please don't file the placeholder as a broken link.Scope: the other four modules are deliberately deferred
tools.py,coverage.py,prefix_trie.pyandlinestring.pyare untouched.tools.pyis being edited concurrently under #136 and by the unmerged PR #130 (which privatisesheal_norm); converting it in this PR guarantees a conflict for no benefit. They are also the already mostly-converted modules (18/23 and 11/16Parametersblocks), so the payoff is much smaller. They should land as a follow-up once #136 and #130 settle.Phases
mortie/morton_index.py—d637563mortie/geometry.py—832c280mortie/arrow.py—187f4fc## Documentationsection onREADME.md—02b230cREADME.md—e68098e99ef2fcMortonIndexScalaron the API page —d1662c2mortie/pandas.pyso its docstrings publish —8032cd0tools.py/coverage.py/prefix_trie.py/linestring.py— deferred, see aboveSelf-review round (
99ef2fc)The fresh-context review found one real regression, which is fixed:
The thin wrappers had become less informative than the prose they replaced.
from_wkb/from_wktpreviously said "Seefrom_geometryfor the parameters", which sent the reader to the complete text. My first pass replaced that with an abbreviated table that silently droppednormalize's winding contract — an apparently-complete table that omits the one thing a caller must act on is worse than the pointer was. They now document only the genuinely local parameter and defer the rest by name, cueing the caveat at the point of use and linking the full contract, with no second copy to drift:Raiseswas also missing on all four wrappers; added, and verified reachable rather than inferred from the delegate:Also:
_decimal_to_word's summary had lost the word "deprecated" to the imperative rewrite, putting the one fact that matters most in the only place that does not render. Restored to the summary line.How it was tested
pytest -v→ 775 passed, 12 skipped. The pre-phase-(8) baseline was 757 passed, 12 skipped; the 18 new tests are all inmortie/tests/test_pandas_module.pyand nothing that passed before was removed, skipped or weakened. Phases (1)–(3) had held the count byte-for-byte at 757, as a docs-only change must.flake8 mortie --select=E9,F63,F7,F82→ clean.ruff check mortie/pandas.py mortie/morton_index.py mortie/tests/test_pandas_module.py→ All checks passed. The twoD200/D400findingsruff check mortiereports onmortie/__init__.pyare on line 1 (the pre-existing module docstring) — confirmed identical before and after this commit by re-running against the stashed tree.griffeinParser.numpymode withwarn_unknown_paramson → no warnings.mkdocs build --strict→ exit 0, zero warnings, with the 47 anchors above.Questions for review
(1) RESOLVED in phase (8).
TheThe classes are now module-level inMortonIndexArray/MortonIndexDtypemethods are converted but do not publish today.mortie/pandas.pyand render as 40 anchors onapi/pandas. Recorded here rather than deleted because it is what motivated the move. Note the resolution took neither of the two options I floated (mkdocstrings dynamic loading, or aTYPE_CHECKINGshim) — moving the definition to a module that is allowed to import pandas keeps the docs statically verifiable and needs no docs-tool escape hatch.(2) The second clause of the new
ImportErrormessage describes both remedies, not just one — flagging rather than editing it. The message sayspip install mortie[pandas]"pins pandas as a mortie dependency so it is loaded whenever mortie is imported." The first half is the real distinction and is accurate. The second half ("so it is loaded whenever mortie is imported") is equally true after a plainpip install pandas, because the eager registration probe keys off whether pandas is importable, not off how it was installed — so as written it may read as implying the barepip install pandasroute leaves the ExtensionArray unavailable, which it does not. I applied the wording exactly as specified rather than quietly rewriting it; if you want the sharper distinction, something like "records pandas in mortie's own dependency metadata, so environments that reinstall mortie get it automatically" is the difference that actually holds. One-line change either way.(3)
mortie/pandas.pyis 1002 lines, marginally over the ~1000-line guideline. Raising it per the convention rather than acting. It is a pure move — 953 of those lines are the two class bodies verbatim, and the net across the pair is an improvement (morton_index.py1319 → 343, plus 1002 new). Splitting the dtype and the array into separate modules is possible but would break the "moved verbatim" property that makes this diff cheap to review, so I did not. Happy to split as a follow-up if you want it under the line.(4) Two adjacent documentation badges — your call. The block now carries
Documentation(static link, line 4) andDocs(workflow status, line 6), separated only byTests. They answer different questions and both were asked for, and rendered they look less alike than the source does (docs | espg.github.io/mortieversusDocs | passing). The self-review suggested dropping the static one now that the workflow publishes a real site. I did not, because that would undo an explicit instruction rather than fix a defect — but it is a one-line change if you prefer it.(5) Sphinx roles render literally on the new page. The moved docstrings use
:meth:/:func:roles, which mkdocstrings underdocstring_style: numpyemits as literal text (:meth: from_nested). This is pre-existing and repo-wide —api/morton_indexalready renders 10 such roles today — and the move made it visible on 40 more anchors rather than introducing it. Not fixed here because it is a cross-cutting choice (either strip the roles or add a cross-reference extension), and doing it inside a "moved verbatim" commit would destroy that guarantee. Worth its own issue.(6) Rationale I found hard to place. Two spots, both resolved by keeping the prose rather than tabulating it:
_require_shapelycarries a standing open question ("Whether to invest in a spherely introspection shim is an open question for the issue thread (see the PR's 'Questions for review')"). It refers to another PR's review section, which is no longer reachable from the code. Kept verbatim in the Extended Summary, but it is a dangling reference and probably wants either a real issue number or deletion._stitch_segments' explanation of when thepoleargument is reachable ("it is only ever reached when the segments are genuinely unbalanced, so a non-pole cover never touches it") is a contract statement about the argument, so it moved into thepoleentry underParameters; the GeoJSON-convention paragraph stayed prose.(7) One place the existing prose looks wrong, not merely unstructured — surfacing rather than silently rewriting.
MortonIndexArray.hive_pathdocumented onlyValueError"on any empty / invalid word", but the body also raisesValueErrorfor a point id:The docstring never mentioned it.
from_hive_pathhas the same gap twice over — it documented the mis-filed-leafValueErrorbut neither the point-suffix rejection nor the "leaf does not end withsuffix" rejection. I documented all three inRaises, since the behaviour is unambiguous in the code and matches spec section 2 / issue #120 — but the docstrings were previously incomplete, so this is a documentation-accuracy fix riding along and worth a second pair of eyes.(8) Missing defaults on the geometry parameter tables.
from_geometrydocumentsmoc/normalizewithout stating their defaults (False/True). Pre-existing, not introduced here, and I left it rather than fixing it only on the wrappers — that would make the delegate look like the less complete of the two. Worth a separate consistency pass across the module.(9) Minor.
base_cell,order,is_fixed_order,_dissolved_rings_pyand a few others had noun-phrase summaries ("The single shared order, or raise if…"), whichD401rejects. Rewriting them to imperative mood ("Return the single shared order of a fixed-order array.") changes the summary wording; the "or raise" half moved toRaises. No meaning lost, but the summaries do read differently from before.