Docs coverage of the public surface + bidirectional pin (issue #176) - #214
Docs coverage of the public surface + bidirectional pin (issue #176)#214espg wants to merge 6 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #214 +/- ##
==========================================
- Coverage 96.53% 96.44% -0.09%
==========================================
Files 20 20
Lines 2365 2365
==========================================
- Hits 2283 2281 -2
- Misses 82 84 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
| The pyarrow skin's two classes are public as `mortie.MortonIndexType` and | ||
| `mortie.MortonIndexExtArray` (and on `mortie.arrow`), but they are **built | ||
| lazily on first attribute access** behind a module `__getattr__`, so that | ||
| importing mortie never imports pyarrow. That is also why they have no |
There was a problem hiding this comment.
🤖 from Claude (review)
“so that importing mortie never imports pyarrow” is false whenever pyarrow is installed, so the sentence states the opposite of the contract the code actually keeps. mortie/arrow.py ends with an eager registration block:
try:
import pyarrow as _pa # noqa: F401
_build_type()
except ImportError:
passand mortie/__init__.py does from . import arrow eagerly — so import mortie pulls pyarrow in and builds/registers the extension type at import time:
$ .venv/bin/python -c "import sys, mortie; print(\"pyarrow\" in sys.modules, mortie.arrow._EXT_TYPE is not None)"
True True
The next clause inherits the same problem: “the classes do not exist until first touched” — with pyarrow present the type object exists from import. What is true (and is the real griffe reason) is that the two names are never module-level attributes: they are locals of _build_type(), reachable only through arrow.__getattr__, so static resolution has nothing to render either way.
Fix direction: state the guarantee the code makes — importing mortie never requires pyarrow (a numpy-only install imports fine, and touching either name raises the curated ImportError) — and pin the no-render reason on “never bound as module attributes” rather than on “not built yet”.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 9d66659 — the sentence stated the opposite of the contract, exactly as you show.
The subsection now pins the no-render reason on the binding, not on build timing: the classes are defined inside _build_type() and handed out by a module __getattr__, so they are never bound as module attributes and static resolution finds nothing to render whether or not pyarrow is installed. The optionality claim is re-scoped to what the code actually keeps — importing mortie never requires pyarrow (numpy-only install imports fine, touching either name there raises the curated ImportError) — and the eager block you quoted is now stated positively: when pyarrow is present, mortie.arrow builds and registers the extension type at import so a parquet read resolves the extension name without the user touching the type first.
The LAZY_ARROW_UNDOCUMENTED comment in mortie/tests/test_docs_api_pages.py already carried the accurate version of this reason ("defined inside _build_type() and reached only through arrow.__getattr__"), so the prose and the roster now say the same thing.
| subclass: what `from_morton_index` returns, and what pyarrow hands back | ||
| when the registered type resolves on read. Its one addition over the | ||
| stock class is `to_numpy(**kwargs)`, which materializes the `uint64` | ||
| storage (defaulting `zero_copy_only=False` so a null-bearing array |
There was a problem hiding this comment.
🤖 from Claude (review)
“materializes the uint64 storage (defaulting zero_copy_only=False so a null-bearing array converts)” overpromises for exactly the case the parenthetical calls out: a null-bearing array does not convert to packed words, it converts to float64/NaN.
$ .venv/bin/python
>>> a = marrow.from_morton_index(MortonIndexArray(np.array([1, 0, 3], dtype=np.uint64)))
>>> a.null_count, a.to_numpy(), a.to_numpy().dtype
(1, array([ 1., nan, 3.]), dtype("float64"))
(the 0 word is MortonIndexArray._SENTINEL, mortie/pandas.py:140, so it lands as a null in the Arrow storage). zero_copy_only=False only stops it raising; pyarrow still widens an integer array with nulls to float64, which is lossy for 64-bit words. A reader following this bullet gets NaNs where the doc promised uint64.
The to_morton_index steer is correct — it does storage.fill_null(int(MortonIndexArray._SENTINEL)) (mortie/arrow.py:275) — but as written the sentence reads as “both paths give you words, one just maps nulls”.
Fix direction: say to_numpy() returns the uint64 words for a null-free array and float64/NaN once nulls are present, which is why to_morton_index is the path whenever nulls are possible. Worth syncing MortonIndexExtArray.to_numpy’s own docstring (mortie/arrow.py, “Returns … The uint64 packed words”), which has the same gap.
There was a problem hiding this comment.
🤖 from Claude
Fixed in f511cb6, and I took the docstring sync you flagged in the same commit (same finding).
The bullet now says the zero_copy_only=False default only stops the call raising: null-free storage comes back as the uint64 words, while any null present makes pyarrow widen the result to float64 with NaN — lossy for 64-bit words — which is why to_morton_index is the path whenever nulls are possible (it fills nulls with the sentinel 0 word and stays uint64).
MortonIndexExtArray.to_numpy’s own Returns section in mortie/arrow.py carried the same gap and now states the null-free / null-bearing split and points at to_morton_index.
Gates on that commit: pytest mortie/tests/test_arrow.py -q 25 passed; numpydoc lint mortie/arrow.py clean; flake8 mortie --select=E9,F63,F7,F82 clean.
| def all_pages(): | ||
| """Map each documented module to its page name and ``members:`` roster.""" | ||
| return { | ||
| module: (page.name, members) |
There was a problem hiding this comment.
🤖 from Claude (review)
all_pages() keys the dict on the module, so if two pages ever document the same module the later-sorted page silently replaces the earlier one’s roster — the very mis-association page_members() refuses to tolerate within a page (“Fail loud here rather than mis-associate”) is silently allowed across pages, and it disarms the reverse direction.
Verified by copying docs/api to a scratch dir and repointing API_DIR at it (working tree untouched): added zz_coverage_split.md carrying a second ::: mortie.coverage block with coverage.md’s four members, and appended a non-public - np to coverage.md. All three new tests pass:
M3-two-pages-one-module: test_every_public_name_documented_on_exactly_one_page PASS
M3-two-pages-one-module: test_lazy_arrow_roster_is_not_stale PASS
M3-two-pages-one-module: test_every_documented_name_is_public PASS
whereas the same - np on coverage.md alone fails as designed:
M2-bogus-np: test_every_documented_name_is_public FAIL -> documented names missing from mortie.__all__: ["coverage.md: np"]
Three consequences of the collision: a stray / non-public members: entry on the losing page escapes the docs→__all__ direction entirely; a name genuinely rendered twice escapes the “exactly one page” rule; and when the surviving page does not repeat a member, the forward test reports "<name>: on no page" for a name that is in fact on a page — a misleading failure to debug.
Fix direction: key on the page ({page.name: (module, members)}) and derive the module lookup from the values, or add a loud assert that no module is claimed by two pages, mirroring the one-block-per-page guard just below.
There was a problem hiding this comment.
🤖 from Claude
Fixed in e9af4c4 — took the assert option, mirroring the one-block-per-page guard in page_members() rather than re-keying the dict (consumers stay as they are, and the failure names both pages).
all_pages() is now a loop that asserts a module is claimed by at most one page: zz_coverage_split.md: mortie.coverage is already documented by coverage.md.
Re-ran your escape scenario against a scratch copy of docs/api (working tree untouched, no stray files left): with a second ::: mortie.coverage page present, all three tests now fail on that assert instead of passing —
test_every_public_name_documented_on_exactly_one_page: FAIL -> zz_coverage_split.md: mortie.coverage is already documented by coverage.md
test_lazy_arrow_roster_is_not_stale: FAIL -> (same)
test_every_documented_name_is_public: FAIL -> (same)
Gates: pytest mortie/tests/test_docs_api_pages.py -q -p no:randomly 20 passed; ruff check --select E,F,W,I --ignore E501 and flake8 --select=E9,F63,F7,F82 on the file clean.
| convention (origin at the subtree's south corner). The input is rank-space, | ||
| **not** packed morton words — strip the shard prefix down to the base-4 | ||
| digit-tail rank first. Normative statement: | ||
| [specification.md §8](../specification.md#8-rank-space-x-y-deinterleave); |
There was a problem hiding this comment.
🤖 from Claude (review)
This anchor (and ../coverage_methods.md#ring-validity on docs/api/coverage.md) resolves correctly today — I built the site and confirmed specification/index.html carries id="8-rank-space-x-y-deinterleave" and coverage_methods/index.html carries id="ring-validity" — but the PR’s “mkdocs build --strict: green” does not cover it: mkdocs 1.6 defaults validation.links.anchors to info (.venv/.../mkdocs/config/defaults.py:194), and mkdocs.yml declares no validation: block, so a wrong fragment builds clean.
That gap is already live in the tree, including in a file this PR edits:
$ grep -rn "#latitude-convention" docs/
docs/healpix_interchange.md:83 / docs/index.md:55 / docs/api/convert.md:15 / docs/specification.md:315,318
$ grep -o "id=\"[^\"]*latitude[^\"]*\"" site/specification/index.html
id="9-latitude-convention-authalic-on-wgs84"
Five links point at an anchor the built page does not have, and --strict has never said so — the same silent-drift shape this PR exists to close, one layer up from members:.
Fix direction: add validation: {links: {anchors: warn}} to mkdocs.yml so --strict actually pins the new cross-page anchors; that turns the five #latitude-convention links red, so either fix them in the same change or leave both for a follow-up issue — your call on scope, since the dangling ones are pre-existing.
There was a problem hiding this comment.
🤖 from Claude
Partial fix in 86d92ba, with the config change and the four other dangling links deliberately left standing.
(1) The anchors this PR adds are verified. Built the site and grepped the HTML: specification/index.html carries id="8-rank-space-x-y-deinterleave" and coverage_methods/index.html carries id="ring-validity", so both new cross-page links resolve today — confirming your check independently rather than resting on --strict, which as you show says nothing about fragments (mkdocs 1.6 defaults validation.links.anchors to info and mkdocs.yml declares no validation: block).
(2) Not turning on validation.links.anchors. Changing mkdocs.yml alters what the Docs build enforces repo-wide and would turn the five pre-existing #latitude-convention links red — a behavior change past this PR’s scope (documenting the public surface). That is a scope call for espg; worth its own issue alongside question (3) on the PR body, since both are "make the docs build assert what we verified by hand".
(3) Fixed the one link in a file this PR already edits. docs/api/convert.md:15 now points at ../specification.md#9-latitude-convention-authalic-on-wgs84 — the real rendered id, read off the built spec page (grep -o id="[^"]*latitude[^"]*" → only 9-latitude-convention-authalic-on-wgs84), not guessed. Rebuilt after: mkdocs build --strictgreen and the rendered href is../../specification/#9-latitude-convention-authalic-on-wgs84`.
The other four (docs/healpix_interchange.md:83, docs/index.md:55, docs/specification.md:315,318) are outside this PR’s file set and stay standing for espg — they are pre-existing and fixing them here would be an unrelated docs sweep.
Merging this PR will not alter performance
Comparing Footnotes
|
|
🤖 from Claude Status: all phases complete, all four adversarial-review findings folded (fix shas and rationale on each inline thread), and CI on 86d92ba is green on every substantive check — tests 3.10/3.11/3.12, ruff, numpydoc validation, arro3-no-pyarrow, msrv, crate, all wheel builds, codecov patch+project, Python benchmarks, Docs strict build. The one non-concluded check at flip time is CodSpeed's "Rust benchmarks" (in_progress) — the known-noise job on docs-heavy diffs (same job read cancelled/unstable on PR #195); noted, not chased. Marked ready for review and applied |
Closes #176.
Docs coverage of the public surface, per the approved plan on the issue (approach (A), all five leans), re-derived against post-#195
mainas the blocked-status note required:mortie.__all__is now 88 names, and 13 of them rendered on nodocs/apipage — the pre-#195 eleven plusmocandtoc, the issue #196/#198 callable constructor namespaces, which nomembers:roster ever listed.Disposition table (re-derived on
main@ 4900a7e, by object identity)geo2uniq,norm2uniq,uniq2geo,unique2parentdocs/api/convert.md; the stale "Not yet documented here" admonition deleted (issue #136 closed and settled the signatures;heal_normis gone from the tree)ring_validity,ring_is_simple,RingValiditydocs/api/coverage.md, with an intro pointer to the Ring validity narrativerank_to_xy,xy_to_rankdocs/api/rank_xy.md+ nav entry afterorders(spec §8 linked as the normative statement)moc,tocdocs/api/moc_object.md/docs/api/toc_object.mdmembers:— each renders as the module attribute with its "constructor, bound where the submodule used to be" docstringMortonIndexType,MortonIndexExtArrayLAZY_ARROW_UNDOCUMENTED) with the griffe/static-resolution reason inline;docs/arrow_interchange.mdgains a real "The pyarrow extension classes" subsection, sodocs/api/arrow.md's redirect finally points at somethingReverse direction (docs → surface): 4 documented names are not flat in
__all__and sit in the justifiedMODULE_SCOPED_DOCUMENTEDroster keyed on (module, name) —mortie.arrow.export_c_array/export_c_schema/import_c_array(namespaced C Data Interface plumbing, issue #93) andmortie.morton_index.MortonIndexScalar(#104). Each hangs off a submodule that is itself in__all__, and the test asserts exactly that reachability.Phases
arrow_interchange.mdsubsection, stale admonition deleted) — 0bca837mortie/tests/test_docs_api_pages.pybidirectional — 044b03aThe bidirectional pin's shape
Three new tests beside the existing stale-entry direction (which is unchanged):
test_every_public_name_documented_on_exactly_one_page— every non-submodule__all__name must appear on exactly one page, keyed on (defining module, name) by object identity (getattr(page_module, name) is getattr(mortie, name)), so the arrow skins offrom_wkb/polygons_to_morton_mocsonarrow.mdstay legal without double-counting the flat name; every submodule in__all__(geometry,morton_index,arrow) must have a page whose:::block is that module. The issue's acceptance mutation now fails: deleting- morton_bufferfrombuffer.md(the post-API consolidation: one polymorphic function per operation (issue #187) #195 equivalent of thechildren_ofdeletion in the PR #172 review finding) fails this test — verified by mutation locally.test_every_documented_name_is_public— everymembers:entry must be in__all__by name or inMODULE_SCOPED_DOCUMENTED; the roster itself is guarded against staleness (entries must still be on their page, and their submodule must be in__all__). Adding a bogusmembers:entry fails — verified by mutation.test_lazy_arrow_roster_is_not_stale— if the two lazy Arrow classes ever become renderable and get listed, the allowlist must shrink in the same change; and they must stay in__all__. The lazy pair is nevergetattr-ed (resolving them without pyarrow raises ImportError, per the risk note on the issue).The symmetric pages→
__all__flat-promotion direction is handled by the roster rather than by forcing the 4 module-scoped names into__all__— see question (2) below.Render-anchor verification (the
RingValidityrisk item)The espg ruling asked whether griffe actually renders the namedtuple (and the two
_MocNamespace()/_TocNamespace()instance attributes) —hasattralone can't see a render-time drop. Checked manually (scripted grep over themkdocs build --strictoutput, not automated in the suite): every one of the 11 newly documented names has itsid="mortie.<module>.<name>"anchor in the built site, includingRingValidity,moc, andtoc. Automating it would put the docs toolchain into the test env; left manual, flagged in question (3).Also in this PR
Phase 2's identity-keyed check exposed a pre-existing test leak:
test_arrow.py::test_clear_error_without_pyarrowevictsmortie.arrowfromsys.modulesand re-imports a transient pyarrow-blocked copy — which rebinds the parent package'sarrowattribute, andmonkeypatch.undo()restores thesys.modulesentry but not the attribute, leavingmortie.arrow is not sys.modules["mortie.arrow"]for the rest of the session (order-dependent failure underpytest-randomly). Fixed at the source with onemonkeypatch.setattr(mortie, "arrow", ...)registration soundo()restores both.Testing
pytest -v: 1872 passed, 16 skipped (full suite, including the new pins; also re-run with the docs-pin file ordered aftertest_arrow.pyto confirm the leak fix).- morton_bufferdeletion and bogus-member addition both fail the new tests (see above).mkdocs build --strict: green; anchors verified as above.flake8 mortie --select=E9,F63,F7,F82: clean.ruff check --select E,F,W,I --ignore E501on the two touched Python files: clean. numpydoc lint: no non-test.pytouched (tests are excluded from the gate).Questions for review
moc/tocplacement. They render as attribute entries under their object pages' existing:::block (docstring: "TheMocconstructor, bound where the submodule used to be"). Alternative was allowlisting them as narratively-documented (the pages' prose already opens withmortie.moc(...)). The member entry seemed strictly better — say if you'd rather not have the attribute stubs rendered.export_c_array,export_c_schema,import_c_array,MortonIndexScalar): kept module-scoped and rostered, per the standing lean that the symmetric direction is not forced. If any should instead be promoted flat into__all__before the 1.0 freeze, that's a surface change and your call — the roster shrinks by the same entry either way.site/for every documented member's anchor. Worth an issue, or leave manual?