-
Notifications
You must be signed in to change notification settings - Fork 1
Docs coverage of the public surface + bidirectional pin (issue #176) #214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
0bca837
044b03a
9d66659
f511cb6
e9af4c4
86d92ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,3 +76,4 @@ answer can err near a boundary. | |
| options: | ||
| members: | ||
| - Moc | ||
| - moc | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # mortie.rank_xy | ||
|
|
||
| Subtree-local rank <-> face-local `(x, y)` bit deinterleave for 2-D block | ||
| views (issue #149). A depth-`d` subtree holds `4**d` cells whose ascending | ||
| packed-word order is a Z-order (morton) curve over a `2**d x 2**d` block; | ||
| `rank_to_xy` / `xy_to_rank` convert between a cell's **rank** in that block | ||
| and the deinterleaved pair, matching the healpy / HEALPix C++ `pix2xyf` | ||
| 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); | ||
| the public functions ship the Rust kernel (`src_rust/src/rank_xy.rs`). The | ||
| names stay flat on the package (`mortie.rank_to_xy`, `mortie.xy_to_rank`). | ||
|
|
||
| ::: mortie.rank_xy | ||
| options: | ||
| members: | ||
| - rank_to_xy | ||
| - xy_to_rank | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,3 +55,4 @@ each answer can err near a span edge (the quanta are ~2–4 s). | |
| options: | ||
| members: | ||
| - Toc | ||
| - toc | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,31 @@ workers (e.g. an AWS Lambda layer without pyarrow). The raw Arrow C structs are | |
| built in Rust (via the `arrow` crate), so nothing on the critical path imports | ||
| pyarrow. | ||
|
|
||
| ## The pyarrow extension classes: `MortonIndexType` / `MortonIndexExtArray` | ||
|
|
||
| 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 | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 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. try:
import pyarrow as _pa # noqa: F401
_build_type()
except ImportError:
passand 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 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”.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 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 The |
||
| rendered [API page](api/arrow.md): mkdocstrings resolves modules statically, | ||
| and the classes do not exist until first touched. They are documented here | ||
| instead. Touching either name without pyarrow installed raises an | ||
| `ImportError` pointing at the missing extra. | ||
|
|
||
| - **`MortonIndexType`** is the `pyarrow.ExtensionType` subclass over | ||
| `uint64` storage with extension name `mortie.morton_index`. It carries no | ||
| parameters — its serialized form is empty; the extension name is the whole | ||
| identity — so the type survives parquet / IPC round-trips. | ||
| `morton_index_type()` builds, registers, and returns the singleton | ||
| instance; there is no reason to construct the class directly. | ||
| - **`MortonIndexExtArray`** is the matching `pyarrow.ExtensionArray` | ||
| 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 | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude (review) “materializes the (the The Fix direction: say
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude Fixed in f511cb6, and I took the docstring sync you flagged in the same commit (same finding). The bullet now says the
Gates on that commit: |
||
| converts); for the null → sentinel-`0` word mapping, go through | ||
| `to_morton_index` instead. | ||
|
|
||
| ## Producing a column (any Arrow lib) | ||
|
|
||
| `export_c_array` returns the `(schema_capsule, array_capsule)` pair of the | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,22 +8,64 @@ | |
| **stale-entry** direction: every ``members:`` name in ``docs/api/*.md`` must | ||
| resolve as an attribute of the module its page documents. | ||
|
|
||
| The reverse direction is deliberately not pinned: nothing here requires every | ||
| public name to appear on *some* page, so a move that deletes a member from its | ||
| old page and forgets to add it to the new one still vanishes silently — as do | ||
| the public names that have no ``docs/api`` entry today. Closing that would | ||
| mean pinning an explicit roster of undocumented names, a docs-policy decision | ||
| rather than a drift check. | ||
| The reverse direction is pinned too (issue #176): every non-submodule name in | ||
| ``mortie.__all__`` must appear on exactly one page — keyed on *(defining | ||
| module, name)*, so the arrow skins of core functions stay legal on their own | ||
| page — and every submodule in ``__all__`` must have a page whose ``:::`` block | ||
| is that module. A move that deletes a member from its old page and forgets | ||
| the new one now fails here instead of vanishing silently. The only names | ||
| allowed off the pages are the two lazily-built Arrow classes | ||
| (``LAZY_ARROW_UNDOCUMENTED``), which mkdocstrings cannot resolve statically. | ||
|
|
||
| And the docs cannot outgrow the frozen surface either: every ``members:`` | ||
| entry must be in ``__all__`` by name, or sit in ``MODULE_SCOPED_DOCUMENTED`` | ||
| — the documented names deliberately reached through their submodule | ||
| (``mortie.arrow.export_c_array``, ``mortie.morton_index.MortonIndexScalar``) | ||
| rather than flat. Growing either roster is a deliberate act reviewed here, | ||
| not a silent omission. | ||
| """ | ||
|
|
||
| import importlib | ||
| import inspect | ||
| import re | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
||
| API_DIR = Path(__file__).resolve().parents[2] / "docs" / "api" | ||
|
|
||
| # The two pyarrow extension classes are defined inside ``_build_type()`` and | ||
| # reached only through ``arrow.__getattr__`` (pyarrow is an optional extra), | ||
| # so griffe's static resolution cannot see them and no ``members:`` entry can | ||
| # render them. They are documented narratively in docs/arrow_interchange.md | ||
| # instead — the structural reason will not expire, so this roster should | ||
| # never grow (issue #176). Never ``getattr`` these here: resolving them | ||
| # raises ImportError when pyarrow is absent. | ||
| LAZY_ARROW_UNDOCUMENTED = frozenset({"MortonIndexType", "MortonIndexExtArray"}) | ||
|
|
||
| # Documented names deliberately *not* flat on the package: each is public as | ||
| # an attribute of a submodule that is itself in ``__all__``. The C Data | ||
| # Interface trio is namespaced interop plumbing (``mortie.arrow.export_c_array``, | ||
| # issue #93); ``MortonIndexScalar`` is the repr/scalar type handed back by the | ||
| # ExtensionArray, spelled ``mortie.morton_index.MortonIndexScalar`` (#104). | ||
| MODULE_SCOPED_DOCUMENTED = frozenset({ | ||
| ("mortie.arrow", "export_c_array"), | ||
| ("mortie.arrow", "export_c_schema"), | ||
| ("mortie.arrow", "import_c_array"), | ||
| ("mortie.morton_index", "MortonIndexScalar"), | ||
| }) | ||
|
|
||
| _MISSING = object() | ||
|
|
||
|
|
||
| def all_pages(): | ||
| """Map each documented module to its page name and ``members:`` roster.""" | ||
| return { | ||
| module: (page.name, members) | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude (review)
Verified by copying whereas the same Three consequences of the collision: a stray / non-public Fix direction: key on the page (
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude Fixed in e9af4c4 — took the
Re-ran your escape scenario against a scratch copy of Gates: |
||
| for page in sorted(API_DIR.glob("*.md")) | ||
| for module, members in [page_members(page)] | ||
| } | ||
|
|
||
|
|
||
| def page_members(path): | ||
| """The (module, members) a docs/api page declares, from its mkdocstrings block.""" | ||
|
|
@@ -55,3 +97,86 @@ def test_every_member_resolves_on_its_module(page): | |
| "(mkdocstrings drops these silently — the rendered page just loses them)" | ||
| ) | ||
| assert len(members) == len(set(members)), f"{page.name}: duplicate members" | ||
|
|
||
|
|
||
| def test_every_public_name_documented_on_exactly_one_page(): | ||
| # The reverse direction of the pin above (issue #176): a name in | ||
| # ``mortie.__all__`` that no page lists has no rendered API entry at all | ||
| # — deleting ``- morton_buffer`` from buffer.md must fail here, not | ||
| # vanish silently. Keyed on (defining module, name) by object identity, | ||
| # so a page documenting another module's *skin* of the same name (the | ||
| # arrow forms of from_wkb / polygons_to_morton_mocs) neither satisfies | ||
| # nor double-counts the flat name. | ||
| import mortie | ||
|
|
||
| pages = all_pages() | ||
| problems = [] | ||
| for name in mortie.__all__: | ||
| if name in LAZY_ARROW_UNDOCUMENTED: | ||
| continue # structurally unrenderable; see the roster's comment | ||
| obj = getattr(mortie, name) | ||
| if inspect.ismodule(obj): | ||
| if f"mortie.{name}" not in pages: | ||
| problems.append(f"submodule {name}: no docs/api page") | ||
| continue | ||
| homes = [ | ||
| fname | ||
| for module, (fname, members) in pages.items() | ||
| if name in members | ||
| and getattr(importlib.import_module(module), name, _MISSING) is obj | ||
| ] | ||
| if len(homes) != 1: | ||
| problems.append(f"{name}: on {homes or 'no page'}") | ||
| assert not problems, ( | ||
| "public names must render on exactly one docs/api page " | ||
| f"(the page of the module they are bound from): {problems}" | ||
| ) | ||
|
|
||
|
|
||
| def test_lazy_arrow_roster_is_not_stale(): | ||
| # If someone finds a way to render the lazy classes (stub declarations, | ||
| # a griffe extension), the allowlist must shrink in the same change. | ||
| import mortie | ||
|
|
||
| documented = {name for _, (_, members) in all_pages().items() for name in members} | ||
| assert not LAZY_ARROW_UNDOCUMENTED & documented, ( | ||
| "allowlisted-as-undocumentable names now appear on a page — prune " | ||
| f"LAZY_ARROW_UNDOCUMENTED: {sorted(LAZY_ARROW_UNDOCUMENTED & documented)}" | ||
| ) | ||
| missing = LAZY_ARROW_UNDOCUMENTED - set(mortie.__all__) | ||
| assert not missing, f"allowlisted names no longer public: {sorted(missing)}" | ||
|
|
||
|
|
||
| def test_every_documented_name_is_public(): | ||
| # The docs cannot outgrow the frozen surface: a ``members:`` entry must | ||
| # be reachable from ``mortie.__all__`` — flat by name, or through the | ||
| # justified module-scoped roster. Growing MODULE_SCOPED_DOCUMENTED is a | ||
| # deliberate, reviewed act (issue #176). | ||
| import mortie | ||
|
|
||
| pages = all_pages() | ||
| public = set(mortie.__all__) | ||
| stray = [ | ||
| f"{fname}: {name}" | ||
| for module, (fname, members) in pages.items() | ||
| for name in members | ||
| if name not in public and (module, name) not in MODULE_SCOPED_DOCUMENTED | ||
| ] | ||
| assert not stray, f"documented names missing from mortie.__all__: {stray}" | ||
|
|
||
| documented_pairs = { | ||
| (module, name) | ||
| for module, (_, members) in pages.items() | ||
| for name in members | ||
| } | ||
| stale = MODULE_SCOPED_DOCUMENTED - documented_pairs | ||
| assert not stale, f"MODULE_SCOPED_DOCUMENTED entries no longer on a page: {sorted(stale)}" | ||
| unreachable = { | ||
| (module, name) | ||
| for module, name in MODULE_SCOPED_DOCUMENTED | ||
| if module.removeprefix("mortie.") not in public | ||
| } | ||
| assert not unreachable, ( | ||
| "module-scoped names must hang off a submodule that is itself in " | ||
| f"__all__: {sorted(unreachable)}" | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 from Claude (review)
This anchor (and
../coverage_methods.md#ring-validityondocs/api/coverage.md) resolves correctly today — I built the site and confirmedspecification/index.htmlcarriesid="8-rank-space-x-y-deinterleave"andcoverage_methods/index.htmlcarriesid="ring-validity"— but the PR’s “mkdocs build --strict: green” does not cover it: mkdocs 1.6 defaultsvalidation.links.anchorstoinfo(.venv/.../mkdocs/config/defaults.py:194), andmkdocs.ymldeclares novalidation:block, so a wrong fragment builds clean.That gap is already live in the tree, including in a file this PR edits:
Five links point at an anchor the built page does not have, and
--stricthas never said so — the same silent-drift shape this PR exists to close, one layer up frommembers:.Fix direction: add
validation: {links: {anchors: warn}}tomkdocs.ymlso--strictactually pins the new cross-page anchors; that turns the five#latitude-conventionlinks 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 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.htmlcarriesid="8-rank-space-x-y-deinterleave"andcoverage_methods/index.htmlcarriesid="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 defaultsvalidation.links.anchorstoinfoandmkdocs.ymldeclares novalidation:block).(2) Not turning on
validation.links.anchors. Changingmkdocs.ymlalters what the Docs build enforces repo-wide and would turn the five pre-existing#latitude-conventionlinks 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:15now points at../specification.md#9-latitude-convention-authalic-on-wgs84— the real rendered id, read off the built spec page (grep -o id="[^"]*latitude[^"]*" → only9-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.