Skip to content

Split tools.py into convert/orders/buffer (issue #159) - #169

Merged
espg merged 14 commits into
mainfrom
claude/159-domain-split
Aug 8, 2026
Merged

Split tools.py into convert/orders/buffer (issue #159)#169
espg merged 14 commits into
mainfrom
claude/159-domain-split

Conversation

@espg

@espg espg commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Closes #159.

⚠️ MERGE THIS WITH A MERGE COMMIT — NOT SQUASH, NOT REBASE

benchmarks/verify_pure_move.py pins phase 1's head, 011816ca, in SPLIT_BASES,
and that sha is an ancestor of this PR's head. An ordinary merge commit keeps it
reachable from main, so the verifier goes on working after the split lands — which
matters because #170 (batch.py) is the
next pure-move refactor and will use this tool. Squash or rebase orphans the sha.
Not silently — the script already reports not reachable in this clone and exits
non-zero — but the check stops being usable, and the fallback is then to retire the
SPLIT_BASES entry and the mortie/geometry.py arm of SPLITS at merge. (Question
(6), ruled by @espg; the same note is in the code, above SPLIT_BASES.)

Carves the two Python grab-bag modules into domain modules mirroring the Rust tree's
existing decomposition, per the approved proposal on
#48. This is
pure moves plus import rewiring — every hunk that is not a move is listed below.

All eight review questions are now ruled and recorded (see "Questions for review"),
and the zagg-side merge gate is clear — englacial/zagg#411
merged, zagg main at e9698f32 has zero mortie.tools, zagg#406 closed. Nothing on
this PR is waiting on anyone.

Phases

  • Phase 1 — split mortie/tools.py (1,493 lines) into convert.py / orders.py / buffer.py, delete tools.py, ship the pure-move verifier.
  • Phase 2 — extract mortie/dissolve.py (the spherical outline machinery) out of mortie/geometry.py (1,665 lines at phase 1's head).
  • Phase 3 — rename test_tools.py to match where its subjects now live, and update the wheel-smoke workflow that named it. (Question (2), approved by @espg.)
  • Phase 4 — peel mortie/codec.py (the backend gate + the codec quartet) out of geometry.py. (Question (5), ruled (b) by @espg.)

Phase 1: recomputed membership and line counts

tools.py grew past the figures in the proposal (children_of landed in it from
#156 phase 3), so membership and counts were recomputed from the current file
rather than taken from the issue. 32 top-level definitions in, 32 out.

module lines public private module-level
convert.py 860 unique2parent, norm2mort, geo2uniq, geo2mort, mort2norm, norm2uniq, uniq2geo, mort2geo, mort2bbox, mort2polygon, mort2healpix _encoder_orders, _normalize_antimeridian_polygon _rust_geo2mort
orders.py 573 order2res, res2display, orders_of_uniq, orders_of, is_point, infer_order_from_morton, validate_morton, clip2order, generate_morton_children, children_of ResolutionLevel, EARTH_RADIUS_KM, MAX_ORDER, _rust_mort2nested, _rust_nested2mort
buffer.py 121 morton_buffer, morton_buffer_meters _EARTH_RADIUS_M

1,554 lines total against tools.py's 1,493 — the +61 is three module docstrings and
three import blocks. Every module is under the ~1,000-line aim (CLAUDE.md §4), with
headroom for the next wave the proposal names (mort2polygons/mort2bboxes
convert.py, further order ops → orders.py).

Two deltas from the issue's table, both consequences of recomputing rather than choices:

  • children_oforders.py. It did not exist when the proposal was written; the
    proposal's own validation test places it "beside generate_morton_children", which is
    orders.py. It is 133 lines, and it is why orders.py is 573 rather than the
    projected 395.
  • convert.py is 860, not 750. Same cause — the projection predates
    #136's mixed-resolution UNIQ work and
    #116's group-by-order dispatch, which
    grew geo2uniq/uniq2geo/mort2bbox/mort2polygon.

Where each private helper went, and why

helper lands in why
_encoder_orders convert.py only consumers are geo2uniq and norm2uniq, both in convert.py
_normalize_antimeridian_polygon convert.py only consumer is mort2polygon
_EARTH_RADIUS_M buffer.py only consumer is morton_buffer_meters
_rust_geo2mort convert.py only consumer is geo2mort
_rust_mort2nested, _rust_nested2mort orders.py genuinely shared — see below

Nothing is duplicated. The two kernel-bridge aliases are the only helpers with
consumers on both sides of the split (_rust_mort2nested: infer_order_from_morton,
validate_morton, generate_morton_children in orders.py and mort2norm in
convert.py; _rust_nested2mort: generate_morton_children and norm2mort). They
live once, in orders.py, and convert.py imports them.

orders.py rather than convert.py because the dependency DAG only admits one
direction. convert.py already needs orders_of / orders_of_uniq /MAX_ORDER at
module scope (mort2geo, mort2bbox, mort2polygon, unique2parent, uniq2geo,
geo2uniq), so orders.py has to be the root: orders → convert and orders → buffer, no cycles. Putting the aliases in convert.py would need
from .convert import _rust_mort2nested in orders.py and a circular import at
package-init time. It is worth naming that this is the one place where the
dependency graph, not the domain, picked the home — a morton ↔ NESTED bridge reads
like a conversion.

Diff hunks that are not moves

Every one, exhaustively:

  1. Three module docstrings (convert.py, orders.py, buffer.py) and three
    import blocks. New text by necessity — tools.py had one docstring
    ("""Functions for morton indexing.""") and one import block.
  2. mortie/__init__.py — the single from .tools import (...) block becomes three,
    in isort position, with the # Inverse functions inline comment travelling with
    infer_order_from_morton. Two trailing comments that named tools.py now name
    convert.py. __all__ is untouched.
  3. Five one-line import rewires in the package: rank_xy.py (MAX_ORDER),
    moc.py (norm2mort), prefix_trie.py (geo2mort), and three function-local
    sites in geometry.py. geometry.py:848 needed both mort2polygon and
    _rust_mort2nested, which now live in different modules, so that one line becomes
    two. moc.py's import block was re-sorted to keep isort happy (.convert sorts
    before .coverage).
  4. docs/api/tools.mdconvert.md + orders.md + buffer.md, with the
    members: lists partitioned to follow the moves (nothing added, nothing dropped —
    the "Not yet documented here" note about the UNIQ helpers travels verbatim to
    convert.md, since all four live there). mkdocs.yml's nav gains the two new pages.
  5. docs/specification.md — two references to mortie.tools.order2res /
    mortie.tools.EARTH_RADIUS_KM now say mortie.orders.*. src_rust/src/lib.rs:230
    is the same edit and is the one place the Rust tree is touched — see
    "One Rust doc-comment, deliberately" below.
  6. Test/benchmark rewiring — mechanical tools.Xconvert.X / orders.X
    across 11 test modules and benchmarks/generate_morton_reference.py. No test body
    logic changed. Three consequences worth flagging:
    • test_tools.py imports orders as orders_mod. Six test bodies bind a local
      variable named orders (e.g. orders = rng.integers(0, MAX_ORDER + 1, size=30)),
      which shadows the module and made flake8 --select=F82 fail with F823. Aliasing
      the module was the option that changed no test-body identifier; the other files
      use the plain orders / convert names.
    • Six continuation lines re-indented. tools.convert. is two characters
      wider, so three visual-indent continuations became E128 and three lines crossed
      88 columns. Re-wrapped; net flake8 --max-line-length=88 count across the suite
      is unchanged from origin/main (see gates).
    • Two isort re-sorts (test_rank_xy.py, test_main_api.py) for the same reason as
      (3).
  7. test_tools.py's module docstring first line now names the two modules it
    tests rather than the deleted mortie.tools.
  8. benchmarks/verify_pure_move.py — new, see below.

Phase 2: recomputed membership and line counts

geometry.py was 1,665 lines at phase 1's head (011816c), not the 1,664 the issue
quotes — phase 1 added one import line to it. Recomputed from the current source: the
spherical-outline block is geometry.py:869-1554 (the section-marker comment through
_dissolved_polygons), 16 functions, exactly the set the issue names and no more.
_per_cell_polygons at :827 is the nearest neighbour and deliberately stays — it
is the dissolve=False emit path, one backend Polygon per cell, with no spherical
reasoning in it at all.

module lines contents
geometry.py 971 unchanged public API — decompose, from_geometry/from_wkb/from_wkbs/from_wkt, to_geometry/to_wkb/to_wkt — plus the privatized codec quartet (_geometry_from_wkb/_from_wkt/_to_wkb/_to_wkt), the backend gate (_require_backend, _require_shapely), #157's Rust WKB plumbing (_wkb_bytes, _rings_from_wkb, _cover_parts), and _per_cell_polygons
dissolve.py 718 _xyz_to_latlon, _spherical_signed_area, _boundary_rings_xyz, _tangent_azimuth, _chain_rings, _antimeridian_winding, _cut_at_antimeridian, _stitch_segments, _next_segment, _point_in_ring, _planar_signed_area, _ring_signed_area_lonlat, _reject_hemisphere_cover, _dissolved_rings_py, _nest_and_build, _dissolved_polygons, and the _DISSOLVE_SNAP constant

44 top-level definitions in, 44 out27 stay, 17 move. (An earlier revision of
this body said "28 stay, 16 move", counting _DISSOLVE_SNAP as a stayer in the headline
and as a mover three paragraphs later. Both cannot hold: it is defined in dissolve.py.
The 17 are the 16 functions in the table plus _DISSOLVE_SNAP; the 27 are 6 module-level
assignments — _BACKEND and the five _TYPE_* — plus 21 functions. Only the count was
wrong; the move itself verifies on all four axes below.) Both modules are under the
~1,000-line aim — but see "Questions for review": geometry.py at 971 has 29 lines of
headroom, which is worth a decision now rather than at the next PR that touches it.

dissolve.py has no public members, so mortie/__init__.py is untouched by phase 2
and there is no new docs/api/ page (an mkdocstrings page with an empty members: list
renders nothing). docs/api/geometry.md's members: list is therefore already the exact
partition — all seven names it lists stayed in geometry.py.

DAG direction, and what forced it

geometry → dissolve, one direction, and nothing forced it — it fell out of the call
graph.
Measured rather than assumed:

  • Not one of the 16 moved functions references any name that stays in geometry.py
    no _require_backend, no _require_shapely, no _ring_latlon. _nest_and_build and
    _dissolved_polygons take the backend module as a parameter (mod), which is why.
  • Exactly one stayer references the moved set: to_geometry calls _dissolved_polygons.

So dissolve.py imports nothing from mortie.geometry, and geometry.py gains one
module-level line — from .dissolve import _dissolved_polygons. That is the opposite of
phase 1, where the two _rust_* aliases had consumers on both sides and the DAG
dictated the home; here either direction was available and the call graph made one of
them empty.

Module-level rather than function-local (geometry.py's habit for .convert / .orders /
._rustie): those are lazy to keep import time light and dodge cycles, and neither
applies — dissolve.py's own module-level imports are math and numpy only (its
.moc / .orders / ._healpix / ._rustie imports stay function-local, moved as-is),
so importing it costs nothing and closes no loop. This matches moc.py's
from .convert import norm2mort.

Where each private helper went, and why

All 16 are private, and they are one connected component: _dissolved_polygons (the
runtime entry) → _nest_and_build_point_in_ring / _planar_signed_area;
_dissolved_rings_py (the reference oracle) → _reject_hemisphere_cover /
_boundary_rings_xyz_chain_rings_tangent_azimuth, and the antimeridian
cut/stitch chain _cut_at_antimeridian_stitch_segments_next_segment. Nothing
in that component has a consumer outside it except _dissolved_polygons itself, so the
cut needed no judgement call — the only decision was _per_cell_polygons, which stays
(above).

One module-level constant moved: _DISSOLVE_SNAP (with its four-line comment), whose
only consumer is _boundary_rings_xyz. import math moved entirely — its only two
uses in the whole file were _tangent_azimuth:1027 and _chain_rings:1083, so
geometry.py no longer imports it. _BACKEND and the five _TYPE_* ids stay: their
consumers (_require_backend, decompose) stay.

Diff hunks that are not moves — phase 2

Exhaustively, and there are five:

  1. dissolve.py's module docstring — new text by necessity.
  2. geometry.py loses import math (and its blank separator) and gains
    from .dissolve import _dissolved_polygons
    . Its own module docstring is untouched.
  3. mortie/tests/test_geometry.pyfrom mortie import geometry becomes
    from mortie import dissolve, geometry, and five references retarget:
    geometry._dissolved_rings_py ×3, geometry._nest_and_build ×1, and one comment
    naming geometry._dissolved_polygons. No test body logic changed. No shadowing this
    time — dissolve appears in these tests only as a keyword argument
    (to_geometry(..., dissolve=False)), never as a binding, so no alias was needed.
  4. docs/api/geometry.md gains one paragraph saying where the outline machinery
    went and why it has no page of its own. members: unchanged.
  5. benchmarks/verify_pure_move.py gains the SPLITS entry, plus SPLIT_BASES
    see below.

One judgement call worth naming: the section-marker comment block at geometry.py:869-876
(# ── emit: dissolved-boundary outline (phase 4) ── and the six lines explaining
edge-cancellation) moved verbatim into dissolve.py rather than being folded into
the new module docstring. Folding would have been a deletion plus new prose in a
pure-move phase; the module docstring is written to complement it rather than repeat it.
It reads slightly odd as a section marker in a module that is entirely that one section —
say so and it collapses into the docstring in the fold.

Phase 3: the test rename, and the workflow edit @espg approved

Question (2) deferred this because renaming mortie/tests/test_tools.py means editing
.github/workflows/build-wheels.yml, which CLAUDE.md §1 forbids unless the issue names
it. @espg approved both"we should update tests and edit the build-wheels.yml
(this is approved)"
— which is the §1 authorization for the workflow hunk below. It is
recorded here so a later reader sees it was sanctioned rather than smuggled in.

Shape: two files, not three, split by subject at class granularity

test_tools.py (1,041 lines, 17 top-level statements) becomes test_convert.py
(785)
and test_orders.py(288). Three points about that shape:

(Both figures were wrong in an earlier revision of this body — "1,042 lines, 16
top-level statements". wc -l is 1041, and 17 is the count the "17 in, 17 out" claim
below uses: 16 named definitions — 14 classes plus the two helpers — plus the
trailing if __name__ == "__main__" block, which is a top-level statement the verifier
cannot name. ast.parse gives 26 in total, the other 9 being the docstring and 8
imports.)

  • No test_buffer.py, because mortie/tests/test_buffer.py already exists. Phase 1
    moved morton_buffer / morton_buffer_meters into buffer.py, but their tests were
    never in test_tools.py — they were already in their own file. Creating a third file
    would have collided with it.
  • The split is by subject, not by which module a line touches. Nearly every class
    calls both modules — TestClip2Order builds fixtures with convert.norm2mort and
    asserts with convert.mort2norm while testing orders.clip2order — so both
    destination files import both modules. What each class is about is unambiguous
    though, so: test_orders.py takes TestOrder2Res, TestRes2Display, TestUniqOrders,
    TestClip2Order, TestGenerateMortonChildren (plus the _sphere_res and _uniq_at
    helpers, each used by exactly one of them); test_convert.py takes the other nine
    classes.
  • Class bodies are byte-identical. Verified directly, not asserted: 17 top-level
    statements in, 17 out, each one compared as literal source text, with only
    if __name__ == "__main__": pytest.main([__file__, "-v"]) deliberately in both files
    (it is a per-file entry point — __file__ resolves per file). pytest reports the
    same 1334 passed before and after, which is the independent check that nothing was
    lost or duplicated.

That last point is why the alias orders as orders_mod survives into both new files
even though only test_convert.py has the local-variable shadow that forced it in
phase 1. Renaming the alias in test_orders.py would have edited six class bodies to
buy cosmetics.

Workflow references — every one found, and what changed

grep -rn "test_tools\|tools\.py\|mortie\.tools" .github/ returns exactly one
match, at build-wheels.yml:429 (line 426's cp -r mortie/tests /tmp/mortie_tests is
the directory copy, unaffected). It ran the wheel smoke test as:

pytest -v mortie_tests/test_tools.py mortie_tests/test_polygon_regression.py

and now runs:

pytest -v mortie_tests/test_convert.py mortie_tests/test_orders.py \
  mortie_tests/test_polygon_regression.py

A 1 → 2 substitution and nothing else. I deliberately did not add
mortie_tests/test_buffer.py: it existed before this PR and was never in the wheel smoke
set, so adding it would be a scope change hiding inside a rename. The edited YAML was
re-parsed with yaml.safe_load to confirm the line continuation is valid.

Where the risk actually sits: CI never runs this line — not on the PR, and not on
main either.
The test-wheels job is skipped on both, so merging will not exercise
it; its first real execution is a tag/release run, where a mistake blocks the PyPI
publish rather than turning a branch red.

  • run 31261344791 (push to
    main): one job, matrix name unexpanded — Test wheels on ${{ matrix.os }} - Python ${{ matrix.python-version }}skipped
  • run 31226132063 (push, tag
    0.9.4): 12 expanded Test wheels on … - Python … jobs, all success
  • publish-testpypi, publish-pypi and Create GitHub Release all carry
    needs: […, test-wheels]

So local emulation is the verification of record, not CI. Running the job's steps
exactly (cp -r mortie/tests /tmp/mortie_tests; cp pyproject.toml /tmp; cd /tmp; pytest -v …) gives 85 passed for test_convert.py test_orders.py test_polygon_regression.py and 85 passed for the pre-split test_tools.py test_polygon_regression.py — the same count, so the substitution moved the whole smoke
set and nothing in it depended on a conftest or fixture left behind.

Diff hunks that are not moves — phase 3

  1. Two module docstrings for the new files. Each names its subject and the split;
    the "Key constraints" block (base-4 encoding, not all integers are valid, tests focus
    on consistency) is carried verbatim into both, since it describes morton indices
    rather than either module.
  2. Two new import blocks, computed from what each file actually uses (so no F401).
    Both are isort-clean, which is why ruff check mortie drops from 14 findings to
    13: origin/main carried an I001 on test_tools.py:14, and that file is gone.
    This is the first phase that changes the ruff count at all.
  3. .github/workflows/build-wheels.yml:429 — above. Not a pure move, and the
    only hunk in four phases that touches CI.
  4. test_main_api.py::test_geo2mort_vs_toolstest_geo2mort_vs_convert. One
    line; the docstring under it already said mortie.convert.geo2mort from phase 1.
  5. benchmarks/verify_pure_move.py gains a "known limitations" bullet — see below.

The verifier does not cover the test split, and why not

check_moves indexes a pytest class fine — it is a top-level ClassDef, so a
class-granular split indexes exactly like a function-granular one, and its print line
comes back 16/16 definitions accounted for. But the hole is total, not partial: it is
not only check_pinned_bases that cannot take the split.
An earlier revision of this
section said check_moves "would handle it" and attributed all 18 failures to the fourth
arm. The split is actually:

  • check_moves: 3 failures. The trailing if __name__ == "__main__" block is a
    top-level statement top_level_defs cannot name, so it is reported as not comparable —
    once in the source and once in each destination. (This is the phase-1 fold's fail-loud
    behaviour working as designed, 889cf97.)
  • check_pinned_bases: 15 more. Its contract is that a pinned base differs from
    --base in body-level import statements alone, and test_tools.py does not satisfy
    it: phase 1 rewrote the call sites inside every one of its bodies (tools.geo2mort
    convert.geo2mort), so 15 classes "differ beyond imports".

Making either pass would mean weakening the arm or allow-listing fifteen classes, and
those arms exist precisely to stop a pinned base being trusted rather than checked. So
the test split is not in SPLITS.

Which means phase 3 is review-gated, not machine-gated — read its numbers that way.
"17 in, 17 out, byte-identical" is a hand-verified completeness argument, not a property
this script reproduces on every run the way the other three phases' numbers are. And
pytest is no second half here, unlike everywhere else in this PR: the only thing it
can catch in a test move is a test that stops passing, and a weakened test still
passes. Demonstrated — turning assert parent == 7 into assert parent == parent inside
a moved class in test_convert.py leaves verify_pure_move.py printing "Pure move
verified" at exit 0 and pytest mortie/tests/test_convert.py mortie/tests/test_orders.py
at its usual 75 passed. Both gates green on a silently gutted assertion.

This is now stated in the script's docstring too (70a7d87), which is the artefact that
outlives this PR body, so the next split does not rediscover it.

Phase 4: codec.py

@espg ruled question (5) option (b) — "we can also add codec.py to help with
headroom."
Recomputed from the current source rather than from the review's estimate
(phase 3 did not touch geometry.py, so the numbers held):

module lines contents
geometry.py 772 decompose, _ring_latlon, _polygon_rings, #157's WKB plumbing (_wkb_bytes, _rings_from_wkb, _cover_parts), the public ingest/emit API (from_geometry/from_wkb/from_wkbs/from_wkt, to_geometry/to_wkb/to_wkt), _per_cell_polygons, and the five _TYPE_* ids
codec.py 225 _BACKEND, _require_backend, _require_shapely, _strip_ewkt_srid, and the quartet _geometry_from_wkb / _geometry_from_wkt / _geometry_to_wkb / _geometry_to_wkt

geometry.py goes 971 → 772, which is the headroom question (5) asked about: 228
lines under the aim rather than 29. The three-way decomposition of the original
1,665-line module is now 772 + 718 (dissolve.py) + 225 (codec.py).

codec.py has no public members — confirmed by scanning it, not assumed; every name
begins with _, the quartet having been privatized under @espg's ruling on #157. So it
gets no docs/api/ page, exactly like dissolve.py, and docs/api/geometry.md's
members: list is untouched and still partitions exactly (all seven names stayed).
mortie/__init__.py is untouched by this phase too.

DAG direction, and what forced it

geometry → codec, one direction, and nothing forced it — the call graph made the
other direction empty.
Measured:

  • Not one of the eight moved definitions references a name that stays. They reference
    only each other: _require_shapely_require_backend_BACKEND,
    _geometry_from_wkt_require_backend + _strip_ewkt_srid, _geometry_to_wkb
    _require_backend + _require_shapely.
  • Exactly five stayers reach into the moved set: decompose and to_geometry (both
    _require_shapely), from_wkt (_geometry_from_wkt), to_wkb (_geometry_to_wkb),
    to_wkt (_geometry_to_wkt). So geometry.py gains a four-name import.
  • dissolve.py needs none of it, confirmed by scan — it takes the backend module as
    a parameter (mod), which is the same property that made phase 2 one-way.

Module-level import graph after four phases: geometry → {codec, dissolve}, and
codec → {}, dissolve → {}. No cycles, and codec.py needs no module-level imports
at all
— the backend is imported lazily inside _require_backend, which is the whole
point of the gate.

Where each constant landed

_BACKEND moves: its only readers/writers are _require_backend and the tests, all of
which move or rewire with it. This matters more than it looks — _BACKEND is mutated
module-globally by test_wkb_no_backend.py, so a test left pointing at
geometry._BACKEND would silently stop gating anything rather than fail.

All five _TYPE_* ids stay. Rechecked now that the gate is moving, not carried over
from phase 2's finding: their only consumer anywhere is decompose, which stays. No
constant has consumers on both sides, so nothing needed the DAG reasoning the _rust_*
aliases got in phase 1.

_strip_ewkt_srid moves with the quartet: its only consumer is _geometry_from_wkt.

Diff hunks that are not moves — phase 4

  1. codec.py's module docstring — new text.
  2. geometry.py gains from .codec import (...) (four names). It loses no import —
    numpy is still used by the stayers.
  3. mortie/tests/test_geometry.py — import gains codec, 15 references retarget
    from geometry._X to codec._X, and one function-local import mortie.geometry as gm becomes import mortie.codec as gm. No test body logic changed.
  4. mortie/tests/test_wkb_no_backend.pyfrom mortie import geometry becomes
    from mortie import codec, and five references retarget. (This entry originally read
    "it referenced nothing else on that module" — wrong: no_geometry_backend's
    docstring named mortie.geometry two lines above the code it describes, and the phase-4
    hunk retargeted the five code references while leaving the prose. Fixed in the phase 3–4
    fold, c3122c4.)
  5. docs/api/geometry.md — the sentence phase 2 added about dissolve now names
    codec too. members: unchanged.
  6. benchmarks/verify_pure_move.pycodec.py added to the geometry.py split's
    destination list. No new SPLIT_BASES entry: phase 3 did not touch
    geometry.py, so phase 1's head (011816c) is still the correct pre-move base for
    everything cut out of it, and one pin covers phases 2 and 4 together. Repinning to
    phase 3's head would have compared phase 2's moves against a tree that already had
    phase 2 applied.

The pure-move verifier

The acceptance bar here comes from #160, whose review AST-compared all 11 moved
definitions against their pre-move originals by hand. benchmarks/verify_pure_move.py
is that check made re-runnable, so this review and the next split do not re-derive it.
It checks three claims against a git base (origin/main by default):

  1. Verbatim — every top-level definition in a destination module that also exists
    in the source at the base compares equal as an AST (ast.dump, so formatting and
    line numbers are ignored) and as literal source text (so comments inside a
    definition are covered too).
  2. Complete — every definition the source module had lands in exactly one
    destination; nothing lost, nothing duplicated, and no destination gains a definition
    that was not there before. A top-level statement the scanner cannot name and compare
    — a tuple-target assignment, an if TYPE_CHECKING: block, a try/except ImportError
    shim, a loop, an __all__ += — is reported as a failure rather than skipped, so
    this arm fails loud instead of open (011816c's parent 889cf97; annotated
    assignments are now compared like plain ones).
  3. Public surface pinnedset(mortie.__all__) equals the base's and every name in
    it resolves as an attribute of mortie. The base package is extracted with
    git archive, the built _rustie is copied in, and it is imported in a subprocess,
    so this compares two real imports rather than two static readings of __init__.py
    (which is not literally evaluable: __all__ += list(_ARROW_NAMES) + [...]). The
    subprocess asserts its own mortie.__file__ is inside the extracted tree, and the
    parent asserts its own is inside the repo — an editable install shared across
    worktrees otherwise makes this compare a tree against itself.
$ python benchmarks/verify_pure_move.py
mortie/tools.py: 32/32 definitions accounted for across mortie/convert.py, mortie/orders.py, mortie/buffer.py
__all__: 69 names, all resolvable, equal to origin/main's

Pure move verified.

Known limitations, stated in the script's docstring. First: a comment block sitting
between two top-level definitions belongs to no definition's source segment and is not
compared. Comments inside a definition body are. For this split that gap is empty
review extracted the residue (every line belonging to no definition) by a method the
script does not use, and all five module-level comment blocks survive verbatim: the
ResolutionLevel banner, the EARTH_RADIUS_KM provenance note, the kernel-bridge note,
the MAX_ORDER note, # Public API - uses Rust, and # Earth mean radius in meters.
The only line the residue loses is the old one-line module docstring, replaced by three
new ones.

Second, added in d470bc4 because review found it: only the moves are checked, not the
"plus import rewiring" half of the claim.
Deleting geometry.py's
from .dissolve import _dissolved_polygons leaves all 76 definitions verbatim and all 69
__all__ names resolvable, so the script still exits 0. pytest is what catches it —
test_emit_dissolve_is_the_default raises NameError: name '_dissolved_polygons' is not defined. The combined gate holds, but a green verifier run is half of it, not the whole
one, and the docstring now says so.

Phase 2 needed one more thing than the predicted one-line SPLITS entry. Verifying
the geometry split against origin/main reported three real differences —
_per_cell_polygons, _boundary_rings_xyz and _reject_hemisphere_cover — because
phase 1 had already rewired their function-local imports (from .tools import
from .convert / from .orders). Allow-listing them would have blinded the arm to any
genuine change in three of the largest moved functions. Instead each split now pins the
commit it was actually cut from:

SPLIT_BASES = {
    "mortie/geometry.py": "011816ca3553c3743c3e92fc5300ed23c6b3a514",  # phase 1 head
}

So the tools arm still verifies against origin/main and the geometry arm verifies
against phase 1's head — both at full strength, 76 definitions compared with nothing
allow-listed. EXPECTED_NEW is still empty.

The phase-2 arm was mutation-tested the way review tested phase 1's, 9 injected
mutations, 9 caught
, tree byte-identical afterwards: a logic edit inside a moved body;
a moved definition deleted; a docstring reworded; a comment added inside a body
(AST-equal, source text differs); the same definition left in both destinations;
a top-level for loop (the 889cf97 fail-loud arm); _DISSOLVE_SNAP changed;
to_geometry dropped from __all__; and a moved definition silently renamed.

The pin is now checked too, not trusted (check_pinned_bases, b7fe794)

Pinning is strictly better than allow-listing, but it left the pinned tree trusted
rather than verified
— and review built the failure rather than arguing it. A change
introduced into geometry.py by phase 1 sits in both the pinned base and dissolve.py,
so check_moves compares it against itself. With a mutation planted inside
_boundary_rings_xyz in a synthetic phase-1 head and mirrored into dissolve.py, phase
1's own verifier reported 32/32 "Pure move verified" exit 0 (its SPLITS has no
geometry.py arm at all) and phase 2's reported 44/44 exit 0. Both green, mutation
shipped
— so 44/44 verbatim read as "verbatim since phase 1", not "since origin/main".

A fourth arm closes it. Every pinned source is now indexed at --base as well, and each
definition must be equal after body-level Import/ImportFrom statements are
dropped
— precisely the class of difference phase 1 legitimately introduced (three
functions, import statements only), and nothing else. That turns the pin from a claim into
a check without reintroducing an allow-list, and the arm reports what it tolerated:

$ python benchmarks/verify_pure_move.py
mortie/tools.py@origin/main: 32/32 definitions accounted for across mortie/convert.py, mortie/orders.py, mortie/buffer.py
mortie/geometry.py@011816c: 44/44 definitions accounted for across mortie/geometry.py, mortie/dissolve.py
mortie/geometry.py@011816c vs origin/main: 44 definitions equal modulo imports (3 import-rewired)
__all__: 69 names, all resolvable, equal to origin/main's

Pure move verified.

Rebuilding review's synthetic seam against the new arm now exits 1:

1 failure(s):
  - <synthetic sha>:mortie/geometry.py: _boundary_rings_xyz differs from origin/main:mortie/geometry.py beyond its imports (source text — comments or formatting)

The new arm was mutation-tested the same way: 16 mutations, 14 expected-catches all
caught, 2 expected-misses both by design
, each planted in geometry.py at a synthetic
pin and mirrored into its destination so check_moves stays green. Caught: a comment
added inside a moved body; a logic edit (AST); a docstring reworded; a blank line added
inside a body; trailing whitespace; _DISSOLVE_SNAP changed; a stayer body edit
(_per_cell_polygons) and a stayer constant change (_TYPE_MULTIPOLYGON)
— the 27
stayers are compared, not just the 17 movers; a definition added by the pin; a definition
deleted by the pin; a re-indented continuation line; an import nested inside an if
rather than at body level; and a body-level import added together with a blank line.
Missed by design: a body-level import added with no blank-line change, and a comment
appended to a body-level import line — dropping is by whole line, so anything on an
import line goes with it. Tree byte-identical afterwards.

SPLIT_BASES no longer fails with a traceback (cbb1a49)

The pinned sha is a branch commit, so a squash-merge (or rebase) invalidates it — and
until now that surfaced as a raw subprocess.CalledProcessError ... exit status 128, the
same fail-with-a-traceback class the fold closed in 011816c. It is now a failure entry
naming the fix, in all three places a revision is read (check_moves,
check_pinned_bases, and check_public_surface's git archive):

$ python benchmarks/verify_pure_move.py          # pin repointed at an unreachable sha
1 failure(s):
  - deadbeef...:mortie/geometry.py is not reachable in this clone — repoint or drop its SPLIT_BASES entry — a squash-merge rewrites a branch sha

Two smaller things on the same finding:

  • The caveat is now genuinely in the script. The previous revision of this body said
    the squash-merge condition "is noted in the script" — it was not, anywhere in the file.
    It is now an eight-line CAVEAT: comment block immediately above SPLIT_BASES, which
    is the artefact that outlives this PR body.
  • --help now says --base is partial. It was silently so once SPLIT_BASES is
    non-empty: --base <anything> still verifies geometry.py's move against 011816c.
    The help text now spells out that a pinned split compares its move against its pin, and
    only the pin itself against --base.

tools.py is deleted, not shimmed

Per the issue. mortie.tools disappears as an attribute, not just as an import target:
comparing a real import of origin/main against this branch, dir(mortie) loses
tools and gains buffer / convert / orders. Every surviving
from mortie.tools import ... raises ModuleNotFoundError on the next mortie release.

Merge-order gate — restated, because the version above this line was wrong.
Re-derived against englacial/zagg@main rather than taken from issue #159:

  • The tracked blast radius is five files, six import sites, not "six files, all
    importing mort2polygon". Five sites import mort2polygon; notebooks/aoi_mask.ipynb:69
    imports mort2geo
    . That distinction matters here because Split tools.py and geometry.py into domain modules mirroring the Rust tree #159 moves the two names
    independently (both land in convert.py, both stay flat-exported, both are in __all__
    on either side — so the remedy is identical, but the enumeration was not). Issue Split tools.py and geometry.py into domain modules mirroring the Rust tree #159
    also names data/conus/plot_conus_shardmap.py, which does not exist on
    englacial/zagg@main and imports nothing.
  • The blast radius was non-zero for longer than the four-site fix suggested.
    land the AOI/CONUS shardmap builders (issue #372) englacial/zagg#395 ("land the AOI/CONUS shardmap builders", issue #372) merged
    2026-08-07 20:16Z and added data/ to the repo, after the branch carrying the
    four-site fix had already forked at 196867b. So for a day there were two sites no
    in-flight fix could have covered:
    data/build_aoi_shardmap.py:129          from mortie.tools import mort2polygon
    data/conus/build_conus_shardmap.py:110  from mortie.tools import mort2polygon
    
    Worth keeping on the record because of why they were invisible: data/ holds
    standalone builder scripts that no CI on either side imports, so neither zagg's
    suite nor mortie's would have caught the ModuleNotFoundError — it would have surfaced
    the first time someone ran a shardmap build against a released mortie.

The gate is now CLEAR — englacial/zagg#411 closed it on 2026-08-08

An earlier revision of this section concluded "#159 must not merge until
englacial/zagg#398 lands and those two data/ sites are fixed."
Both halves are
now wrong
, and the resolution went a different route:

  • zagg#398 did not land, and is not close to landing. It closes zagg#390 (refuse a
    sub-second windowing epoch), whose design is entangled with zagg#410 (temporal/timespan
    representation) and is unsettled. @espg ruled that #406's purely mechanical import fix
    must not sit behind a temporal-design decision, so it was lifted off #398.
  • englacial/zagg#411 landed instead
    (merged 2026-08-08T17:47:11Z), cherry-picking ae408ea with authorship preserved —
    applied clean — plus a second commit for the two data/ builder sites. All six sites
    in one PR
    , not four-plus-two across two.
  • Verified against englacial/zagg@main = e9698f32: git grep "mortie\.tools"
    returns nothing. englacial/zagg#406
    is CLOSED (2026-08-08T17:47:13Z).
  • ae408ea becomes a no-op on #398 whenever that branch rebases.

So #159 / this PR carries no external merge-order precondition any more — it is free
to merge on its own merits.

One Rust doc-comment, deliberately

src_rust/src/lib.rs:230 described rust_geo2mort's bindings-vs-wrapper split by naming
the mortie.tools.geo2mort wrapper. PyO3 compiles /// into __doc__, so that stale
module path is runtime-visible, not just a source comment:

$ python -c "import mortie; print('mortie.tools' in mortie._rustie.rust_geo2mort.__doc__)"
False    # was True before c3c8ac7

It now says mortie.convert.geo2mort — the same one-word retarget as the two on
docs/specification.md, and the last mortie.tools reference anywhere outside the
strings that deliberately name the old module (the new modules' "Split out of
mortie.tools" docstrings, the docs pages, verify_pure_move.py's SPLITS key,
test_tools.py's docstring).

This means the earlier claim "the Rust tree is untouched" no longer holds literally.
It was the cheaper of the two options: leaving it would have shipped a __doc__ pointing
at a module that no longer exists, in a PR whose body claims to enumerate every non-move
hunk. No code changed, so cargo fmt --check / cargo clippy --lib / cargo test --lib
are unchanged (re-run below), and the extension was rebuilt with maturin develop --release
before any gate here was quoted.

(CLAUDE.md:84 still says (tools.py), but as narrative about Python fallbacks removed
historically — correct as written, left alone.)

(Through phases 1–2 mortie/tests/test_tools.py kept its filename even though
mortie/tools.py was gone, because .github/workflows/build-wheels.yml:429 named it and
CLAUDE.md §1 forbids touching a workflow the issue does not name; the same reasoning left
test_main_api.py::test_geo2mort_vs_tools. Both were flagged as question (2), @espg
approved both, and phase 3 did them
— this paragraph is superseded and kept only so the
earlier reasoning is not lost.)

How it was tested

Everything below ran in a dedicated worktree with its own venv and a
maturin develop --release build, with mortie.__file__ asserted into that worktree
before any result was trusted.

gate result
python benchmarks/verify_pure_move.py 32/32 + 44/44 verbatim across five destination modules, 44 equal modulo imports (3 import-rewired), __all__ 69 names equal to origin/main's; 9/9 phase-2 and 7/7 phase-4 move-arm mutations caught, 16/16 pin-arm mutations as expected
pytest (full suite) 1334 passed, 16 skipped
flake8 mortie --select=E9,F63,F7,F82 clean
flake8 mortie --max-line-length=88 61 findings, all pre-existing (origin/main count matches file for file)
ruff check mortie 13 findings — one fewer than origin/main's 14. The set is otherwise identical: tools.py:975 F841 is now convert.py:670 F841 and geometry.py:1356 D205 is now dissolve.py:521 D205, both travelling with their moved function; the one that disappears is origin/main's pre-existing I001 on test_tools.py:14, whose file phase 3 replaced with two isort-clean ones
ruff check benchmarks/verify_pure_move.py clean
numpydoc lint on convert.py, orders.py, buffer.py, dissolve.py, codec.py, geometry.py, __init__.py, verify_pure_move.py clean
cargo fmt --check / cargo clippy --lib / cargo test --lib clean / 1 pre-existing warning (sort_by_key) / 317 passed

All of the above re-run on phase 4's head (83de969) against a fresh
maturin develop --release, with mortie.__file__ asserted into the worktree first, and
again on the phase 3–4 fold head (634f114, and 20f5274 after the question (6)
comment): pytest 1334 passed, 16 skipped;
flake8 mortie --select=E9,F63,F7,F82 clean; ruff check mortie 13, unchanged;
ruff check benchmarks/verify_pure_move.py clean; numpydoc lint clean on the six
modules plus the script; verify_pure_move.py 32/32 + 44/44 + 44 equal modulo imports
(3 import-rewired) + 69 names, exit 0, "Pure move verified". The fold touches
docstrings and one print branch only, so no Rust and no gate moved.
One cargo test --lib run failed 1 of 317 while the Python suite was still finishing and
passed cleanly (317/317) on rerun; #155's known sub_hemisphere_cover_still_dissolves
flake lives in that suite and this PR changes no Rust, so it was not chased per the
instruction. The Rust tree carries exactly one doc-comment word (above);
the cargo gates show it changed nothing.

CI note — the CodSpeed Performance Analysis check is red, and it is not this PR.
Every GitHub Actions job is green on 011816c, including the CodSpeed Benchmarks
workflow itself; the red status is CodSpeed's own analysis, flagging two Rust benchmarks
(coverage_triangle[4] −12.35%, coverage_square[4] −10.33%). It cannot be caused by
the fold: phase 1 (32548b0) carried the same failure at a larger magnitude
("Performance Regression: −13.4%") while touching zero Rust code, and the only Rust
change since is one doc-comment word in rust_geo2mort, which coverage_bench.rs does
not exercise. CodSpeed's own summary names the cause — "Different runtime environments
detected: some benchmarks with significant performance changes were compared across
different runtime environments, which may affect the accuracy of the results."
Flagged
rather than chased, per CLAUDE.md §4; it needs an acknowledge on CodSpeed, which is
@espg's to give.

Still true on the phase 3–4 fold head (20f5274), and CodSpeed now says why itself.
The final CI state there is 17 success, 6 skipped, 1 failure, and the single failure
is this same check: coverage_triangle[4] −12.91%, with 70 untouched benchmarks
and 1 skipped. Two things put it beyond doubt:

  • The Rust benchmarks GitHub Actions job is success on 20f5274 — as are
    test (3.10/3.11/3.12), ruff, numpydoc validation, every wheel build, the sdist,
    arro3-no-pyarrow, and codecov patch+project. Only CodSpeed's analysis status is red.
  • CodSpeed compared against the wrong base and footnotes it: "No successful run was
    found on main (20f5274) during the generation of this report, so 2f28a90 was used
    instead as the comparison base. There might be some changes unrelated to this pull
    request in this report."
    2f28a90 is mortie.arrow.from_wkbs (issue mortie.arrow.from_wkbs: arrow skin over the WKB batch, so a geoparquet column skips Python bytes objects #163) — an
    unrelated commit — so the −12.91% is measured across a diff this PR does not own.

The fold commits touch three module docstrings, one test docstring, a limitations bullet,
one print branch and one comment block. No Rust, no coverage_bench.rs, no code path
a benchmark executes.
Unchanged position: flagged, not chased; the acknowledge is
@espg's.

test_wkb_batch_memory.py::test_the_byte_cap_holds_on_the_real_antarctic_basins failed
once on the pre-change baseline run of origin/main and passes in every run since,
including the full post-change run — it samples a resident-memory peak and is
load-sensitive on macOS. Not chased, and not caused by this change.

Review fold

Four adversarial-review findings, all folded on this branch. One commit each.

finding disposition commit
[medium] merge-order precondition is wrong fixed, then overtaken by events — the restatement (four sites fixed, two data/ sites outstanding) was accurate when written; englacial/zagg#411 has since closed all six and the gate is CLEAR. Section rewritten above PR body only (no tree change)
[low] stale mortie.tools.geo2mort in lib.rs:230, live as __doc__ fixed — chose the retarget over preserving the "no Rust change" property; rationale above c3c8ac7
[low] top_level_defs fails open on non-def top-level statements fixedAnnAssign compared, everything else reported as a failure 889cf97
[low] a lost public definition surfaces as an ImportError traceback fixed — move findings are bound and reported first; the import failure is a failure entry 011816c

Verified after the fold: python benchmarks/verify_pure_move.py still exits 0 at 32/32
and 69 names. Both new arms were mutation-checked — a top-level for appended to
buffer.py gives mortie/buffer.py: For at line 123 is not comparable, and deleting
public morton_buffer now reports

3 failure(s):
  - origin/main:mortie/tools.py: morton_buffer landed in none of mortie/convert.py, ...
  - origin/main:mortie/tools.py: _EARTH_RADIUS_M landed in none of mortie/convert.py, ...
  - importing the working-tree package failed: cannot import name 'morton_buffer' from 'mortie.buffer'

instead of a traceback. Both mutations were reverted (git diff --stat clean).

Independent checks that came back stronger than this PR claimed — recorded so they
are not re-derived:

  • The verifier can fail: 7/7 mutations caught in an isolated clone — altered
    constant, altered function-body identifier, changed default, deleted public def,
    deleted private constant, duplicated def, added __all__ name, removed __all__
    name. Its shared-editable-install guard also fired for real, refusing to run when the
    import resolved outside the worktree.
  • 32/32 definitions verbatim by an independent AST-dump + source-text compare that
    went past the script's own coverage: decorator lines (which
    ast.get_source_segment excludes) and an explicit ast.get_docstring compare.
  • Public surface checked deeper than the script checks it — a real import of
    origin/main vs this branch across all 69 names: identical name set, type,
    inspect.signature, and rendered docstring. Only __module__ moves.
    import mortie.toolsModuleNotFoundError; no package file still imports it.
  • The import-cycle argument is empirical, not assumed. Building the alternative
    (aliases in convert.py) dies with
    ImportError: cannot import name 'MAX_ORDER' from partially initialized module 'mortie.orders'.
  • The docs members: lists are an exact partition — 19 base entries → 7 + 10 + 2,
    none dropped or duplicated, every entry resolving in the module whose page declares it.
  • The test rewiring is mechanical at token level — identical token counts modulo
    import reordering; the only substitutions are toolsconvert (50) and
    toolsorders/orders_mod (4). No test-body identifier changed, so the
    orders_mod aliasing did not leak.
  • Open question (2) is real: .github/workflows/build-wheels.yml:429 genuinely names
    test_tools.py, and it is the only workflow reference to it.

Ruff bot (4 inline comments): all four are pre-existing on origin/main, surfaced
only because this PR touches those files — I001 in test_mort_inverse.py,
test_polygon_regression.py, test_tools.py and D205 in geometry.py. Running
ruff check mortie over a git archive origin/main tree reports the identical 14
findings with the identical codes and files; only line numbers shift (geometry.py:1356
:1357, test_tools.py:14:15). Left as-is per CLAUDE.md §4 ("do not fix
pre-existing CI failures unrelated to your change; flag them instead") and because
fixing them would put non-move hunks in a pure-move PR. Answered individually on each
thread.

Phase 2 fold

Three adversarial-review findings, all folded. One commit each; the third is a body
correction with no tree change.

finding disposition commit
[low] the SPLIT_BASES pin is itself verified against nothing — a change made in phase 1 to a phase-2-moved function escapes both arms (demonstrated) fixed — new check_pinned_bases arm requires the pin to equal --base modulo body-level imports; 16 mutations, 14 caught + 2 missed by design; review's synthetic seam now exits 1 b7fe794
[low] an unresolvable pinned sha raises a raw CalledProcessError; the squash-merge caveat this body claimed was "noted in the script" was not in the file fixed — failure entry in all three read sites, the caveat now sits above the sha, --help says --base is partial cbb1a49
[low] the headline checksum is off by one in both halves — 27 stay / 17 move, not 28/16 fixed — corrected in the phase-2 section; count only, the move itself verifies PR body only (no tree change)

Plus one finding recorded as a documented limitation rather than a fix: the verifier
does not check the import rewiring, only the moves (d470bc4, detail above).

Independent checks that came back stronger than phase 2 claimed — recorded so they are
not re-derived:

  • Phases 1 and 2 together are a verified pure move of geometry.py against
    origin/main, not merely against each other.
    Review closed the seam by hand before
    the script could: indexing all 44 top-level definitions of
    origin/main:mortie/geometry.py against wherever they now live, exactly three differ
    _boundary_rings_xyz, _per_cell_polygons, _reject_hemisphere_cover — and every
    difference is import statements only
    , matching git diff origin/main 011816c -- mortie/geometry.py (4 insertions, 3 deletions). Nothing missing, nothing added. This is
    the claim a reviewer of the merged result wants, and check_pinned_bases now asserts
    it on every run rather than leaving it as a one-off.
  • Independent pure-move verdict: clean. 44 in, 44 out, no overlap between the two
    destinations, nothing missing or added, and every definition equal on four axes —
    ast.dump, ast.get_source_segment, a decorator-inclusive source slice, and
    ast.get_docstring. Module-level residue exact, which confirms the section-marker
    block and the four-line _DISSOLVE_SNAP comment moved verbatim.
  • The DAG is exact in both directions. All 17 dissolve.py definitions reference none
    of the 27 that stayed and none import .geometry; walking the 27 stayers, to_geometry
    is the only one reaching _dissolved_polygons. _per_cell_polygons's characterization
    holds independently — no spherical reasoning, groups by order and delegates to
    mort2polygon, sole caller to_geometry at geometry.py:906.
  • Public surface identical on type, inspect.signature and __doc__ across all 69
    names, with phase 2 changing zero public __module__ (it moves only private
    helpers). import math is fully clean — no remaining use in geometry.py. The five
    non-move hunks listed above are complete. ruff check mortie reports 14 findings with a
    code multiset identical to origin/main's.

Phase 3–4 fold

Four adversarial-review findings, all [low], and the review verified every
substantive claim it checked — so this was a small fold, not a rework. One commit per
finding that needed one; two are body-only.

finding disposition commit
[low] three docstrings still say the codec quartet / backend gate live in mortie.geometry — and geometry.py's is published via docs/api/geometry.md fixed — retargeted at mortie.codec in geometry.py (module docstring, incl. the title), dissolve.py:15-16, and test_wkb_no_backend.py's no_geometry_backend. Same class as the lib.rs:230 finding, no behaviour c3122c4
[low] the _BACKEND gate is live but order-dependent — this file alone is 9 passed, two of them vacuous left, deliberately — pre-existing from #157's fixture, not created by this phase; see the reply on the thread. Stands for @espg
[low] question (8) is right, but the hole is total: check_moves alone cannot take the test split either, and pytest catches nothing in a test move fixed as asked — docstring/body, not the tool. Both now say plainly that phase 3 is review-gated, not machine-gated. Verifier deliberately not weakened or extended; question (8) stays open for @espg 70a7d87
[low] the build-wheels.yml edit is correct, but CI never runs it — on PRs or main; first execution is a release run fixed (body only) — corrected in the phase-3 section: skip evidence, the needs: test-wheels chain, and local emulation named as the verification of record PR body only (no tree change)

Also folded, from the review's "noted but did not file": on a check_pinned_bases
failure stdout still printed 44 definitions equal modulo imports while stderr reported
the mismatch — correct exit code, but it read as success to anyone eyeballing stdout. It
now prints MISMATCH — N failure(s), listed below instead, mirroring
check_public_surface's existing __all__: MISMATCH (634f114). Mutation-checked by
repointing SPLIT_BASES at 440a8fa: stdout was
mortie/geometry.py@440a8fa vs origin/main: MISMATCH — 17 failure(s), listed below,
exit 1; mutation reverted, git diff clean.

Independent checks from this review that came back stronger than this PR claimed:

  • The byte-identical phase-3 claim reproduces. Indexing 8f13e08^:test_tools.py
    against both destinations: 17 in, 17 out, each equal on ast.dump,
    ast.get_source_segment and a decorator-inclusive source slice. Collected node IDs
    match too — 1350 at 440a8fa, 1350 at 83de969; after normalising the file component
    the only difference in the whole set is test_geo2mort_vs_tools
    test_geo2mort_vs_convert.
  • The _BACKEND gate does fail loud when warm. Adding _BACKEND = None back to
    geometry.py and pointing the fixture at geometry._BACKEND gives 2 failed, 105 passed, 4 skipped; commenting out the clear alone does the same.
  • The workflow substitution moves the whole smoke set — 85 passed either side of it.
  • test_geometry.py::test_backend_gate_message is not order-dependent
    (monkeypatch.setattr raises on a missing attribute), so a stale module reference
    cannot pass everywhere.

Questions for review

All eight are ruled. No code change follows from any ruling except (6), which added
the merge-commit note above SPLIT_BASES (20f5274).

  1. orders.py holds the two _rust_* kernel-bridge aliases.resolved:
    @espg approved as-is.
    The dependency DAG forces the direction (see above), and a
    morton ↔ NESTED bridge arguably reading as a conversion does not outweigh it. No
    duplicate alias lines
    ; the single definition in orders.py stands. No change.

  2. test_tools.py keeps its nameresolved: @espg approved the rename and
    the workflow edit
    , done as phase 3 above. The one judgement left inside it is the
    shape: two files split by subject, not three (test_buffer.py already existed).
    Say if you would rather it had been a single test_convert_orders.py.

  3. convert.py at 860 lines is the largest survivor.resolved: @espg ruled
    it stays at 860. Do not peel cell_geom.py, and do not file an issue for it.

    The premise of the question has been removed: the argument for peeling was that
    mocs_to_orders: ragged batch moc_to_order — plus an API sweep for bulk-by-default operators #156's plural twins (mort2polygons / mort2bboxes) would land in convert.py and
    push it past the aim, but @espg's batch.py ruling —
    #170 — sends them to batch.py instead.
    So batch.py caps convert.py's growth rather than shrinking it. Measured on
    this branch: convert.py is 13 top-level defs across 860 lines and none of them is
    bulk today
    (largest is mort2bbox at 110 lines), so nothing is pulled out by Resurrect mortie/batch.py as the consolidated home for the bulk operators, with cross-linked scalar/plural docstrings #170
    either. 860 also sits comfortably under CLAUDE.md's 1,200 cap, with sub-1,200
    overages pre-approved. The residual argument is cohesion — mort2bbox /
    mort2polygon are cell-geometry emitters in an otherwise conversion-focused module —
    which is real but speculative; Resurrect mortie/batch.py as the consolidated home for the bulk operators, with cross-linked scalar/plural docstrings #170 will reveal whether it bites, and it is the
    right place to find out.

  4. _normalize_antimeridian_polygon's dead on_antimeridian local (F841)
    resolved: @espg ruled no new issue — it is already tracked as item 4 of
    #151
    , and the relocation is recorded at
    #151 (comment).
    The F841 moved verbatim; it is now convert.py:670, not tools.py:975.

    That comment also corrects item 3 of Follow up items for the week of August 1, 2026 #151, which is worth folding in here because
    it is the other half of this PR's ruff arithmetic: Follow up items for the week of August 1, 2026 #151's test_tools.py:14 I001
    entry no longer exists, because phase 3 replaced that file with two isort-clean
    ones. That is precisely why ruff check mortie reports 13 against main's 14
    the delta is not a fix this PR made, it is a pre-existing finding whose file stopped
    existing.

  5. geometry.py lands at 971 — 29 lines under the aim. That is the one number in
    this PR I would not call comfortable, and the split has no more obvious seam: what is
    left is the codec quartet, the backend gate, Parse WKB in Rust: backend-free geometry ingest, plus the plural from_wkbs batch #157's WKB plumbing and the public API,
    and every candidate boundary (wkb.py? backend.py?) cuts a genuinely cohesive
    unit. Options as I see them: (a) accept 971 and treat the next geometry.py change as
    the trigger; (b) peel the backend gate + codec quartet into codec.py (~200 lines)
    now, as a phase 3 here; (c) file it as its own issue against the umbrella. My read is
    (a) — but it is your ruling, and (b) is cheap while this branch is open.
    Resolved: @espg ruled (b), done as phase 4 above — geometry.py is now 772.
    Review's read was (b) too,
    measuring the candidate (_BACKEND, _require_backend, _require_shapely,
    _strip_ewkt_srid, and the codec quartet _geometry_from_wkb / _geometry_from_wkt /
    _geometry_to_wkb / _geometry_to_wkt) at ~178 lines of definitions, which lands
    geometry.py near 790; the recomputed figure is 772, and the one-way DAG and
    dissolve.py's independence both held under measurement.

  6. The pinned SPLIT_BASES sha.resolved: @espg ruled keep the pin (the
    first option).
    See the merge-commit banner at the top of this description — it is
    the one thing this ruling asks of whoever merges.
    Two facts settled it, and both are
    recorded because the question as originally posed overstated the risk:

    • There is no vacuous-pass risk. The script already fails loudly when a pin
      stops resolving — unreachable() returns "… is not reachable in this clone — repoint or drop its SPLIT_BASES entry" and the run exits 1. An orphaned sha
      degrades the check to "cannot run", never to "passed". That was added by the phase-2
      fold (cbb1a49) and documented in the comment above SPLIT_BASES; the question was
      drafted as though it were still an open hazard.
    • 011816ca is an ancestor of this PR's head, verified with
      git merge-base --is-ancestor. So it survives a merge commit untouched, and is
      orphaned only by squash or rebase — a narrower exposure than "a squash-merge
      invalidates it" implied.

    Fallback if it is squashed or rebased anyway: retire the SPLIT_BASES entry and the
    mortie/geometry.py arm of SPLITS
    at merge, which is the lifecycle the script's
    own comment already documents. The file itself stays either way — which disposes
    of the third option in the original question, deleting verify_pure_move.py on merge:
    #170 is another pure-move refactor and
    will want this tool. The allow-list option is strictly weaker than the pin with
    nothing left to recommend it, and is dropped. Recorded in the code at SPLIT_BASES
    (20f5274).

  7. The moved section-marker comment in dissolve.pyresolved: @espg ruled it
    moves verbatim, as implemented. No change.
    The reasoning worth recording is that the
    marker is not a one-off: origin/main:mortie/geometry.py carried four of them, and
    each travelled with its own section —

    marker now lives in
    # ── the backend's own codec, wrapped for internal use ── codec.py:130
    # ── ingest: geometry → morton coverage ── geometry.py:142
    # ── emit: morton coverage → geometry ── geometry.py:619
    # ── emit: dissolved-boundary outline (phase 4) ── dissolve.py:34

    so it is a consistent scheme across all three destination modules. Folding only
    dissolve.py's into its module docstring would break the pattern across three files
    to tidy one.

    Flagged as a possible future small-fix, explicitly out of scope here:
    dissolve.py:34's marker reads (phase 4), meaning a phase of the dissolve
    algorithm
    — which now collides confusingly with this PR's phase 4, the one that
    created codec.py. The text is pre-existing and moved verbatim, so renaming it does
    not belong in a pure-move PR. Flagged, deliberately not fixed.

  8. The test split is outside the verifier's modelresolved: @espg ruled no
    verifier exemption.
    The tool is not weakened or extended; instead both the script's
    docstring (70a7d87) and the phase-3 section above now say plainly that phase 3 is
    review-gated, not machine-gated
    , so its "17 in, 17 out, byte-identical" is not read
    as a machine-checked property the way the other three phases' numbers are. The
    adversarial review's independent reproduction of that count is the substance of the
    gate, and is recorded in the phase 3–4 fold above.

@espg espg added the implement label Aug 8, 2026
Comment on lines 4 to 9
import pytest
import numpy as np
from mortie import tools
from mortie import convert, orders


class TestMort2Geo:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [ruff] <I001> reported by reviewdog 🐶
Import block is un-sorted or un-formatted

Suggested change
import pytest
import numpy as np
from mortie import tools
from mortie import convert, orders
class TestMort2Geo:
import numpy as np
import pytest
from mortie import convert, orders
class TestMort2Geo:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Pre-existing on origin/main, left as-is deliberately. This file's import block was already unsorted before the split; the rewire changed which module is imported, not the block's order, so I001 fires at the same place on both sides.

Checked by running the repo's own ruff config over a git archive origin/main tree:

mortie/tests/test_mort_inverse.py:4:1: I001 Import block is un-sorted or un-formatted
mortie/tests/test_polygon_regression.py:12:1: I001 Import block is un-sorted or un-formatted
mortie/tests/test_tools.py:14:1: I001 Import block is un-sorted or un-formatted

Same three files, same code, on origin/main. On this branch the only difference is the line number (test_tools.py:14:15). ruff check mortie totals 14 findings on either side, identical code set.

Not fixed here for two reasons: CLAUDE.md §4 says not to fix pre-existing lint unrelated to the change, and an isort pass would put a non-move hunk into a PR whose whole claim is that every non-move hunk is enumerated. Happy to take all three plus the D205 in a follow-up small-fix if you'd rather have them cleared — say the word.

Comment thread mortie/geometry.py

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [ruff] <D205> reported by reviewdog 🐶
1 blank line required between summary line and description

mortie/mortie/geometry.py

Lines 1357 to 1365 in 32548b0

"""Mirror of the Rust hemisphere guard (``src_rust/src/dissolve.rs``, issue
#108): exterior/hole classification keys off the sign of the mod-4π
spherical signed area, which is ambiguous once the cover nears 2π — fail
loud on the exact covered area (Σ π/(3·4^depth), cells are equal-area)
instead of silently swapping shells and holes. Assumes disjoint,
non-duplicated cells (the dissolve precondition anyway — duplicate words
would break edge cancellation); duplicates double-count. Returns the
exact covered area (steradians) for the wrap cross-check downstream.
"""

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Pre-existing on origin/main, left as-is deliberately. This is _reject_hemisphere_cover's docstring, which this PR does not touch — the only change inside that function is the one-line import rewire at what is now line 1367 (from .orders import _rust_mort2nested). The rewire shifted the docstring down by one line, which is why reviewdog surfaces it here.

Running the repo's ruff config over a git archive origin/main tree:

mortie/geometry.py:1356:5: D205 1 blank line required between summary line and description

Same finding, one line up. ruff check mortie totals 14 findings on either side, identical code set.

Not fixed here per CLAUDE.md §4 (pre-existing lint unrelated to the change), and because reflowing a docstring would put a non-move hunk into a PR that claims to enumerate every non-move hunk. Note this docstring is also load-bearing narrative about the Rust hemisphere guard (issue #108), so the fix is a real reflow rather than a blank-line insert. Happy to take it with the three I001s in a follow-up small-fix if you want them cleared.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [ruff] <I001> reported by reviewdog 🐶
Import block is un-sorted or un-formatted

import pytest
import numpy as np
from pathlib import Path
from numpy.testing import assert_array_equal
from mortie import convert

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Pre-existing on origin/main, left as-is deliberately. This file's import block was already unsorted before the split; the rewire changed which module is imported, not the block's order, so I001 fires at the same place on both sides.

Checked by running the repo's own ruff config over a git archive origin/main tree:

mortie/tests/test_mort_inverse.py:4:1: I001 Import block is un-sorted or un-formatted
mortie/tests/test_polygon_regression.py:12:1: I001 Import block is un-sorted or un-formatted
mortie/tests/test_tools.py:14:1: I001 Import block is un-sorted or un-formatted

Same three files, same code, on origin/main. On this branch the only difference is the line number (test_tools.py:14:15). ruff check mortie totals 14 findings on either side, identical code set.

Not fixed here for two reasons: CLAUDE.md §4 says not to fix pre-existing lint unrelated to the change, and an isort pass would put a non-move hunk into a PR whose whole claim is that every non-move hunk is enumerated. Happy to take all three plus the D205 in a follow-up small-fix if you'd rather have them cleared — say the word.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [ruff] <I001> reported by reviewdog 🐶
Import block is un-sorted or un-formatted

import math
import numpy as np
import pytest
from numpy.testing import assert_allclose, assert_array_equal
from mortie import _healpix as hp
import mortie
from mortie import convert
from mortie import orders as orders_mod

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Pre-existing on origin/main, left as-is deliberately. This file's import block was already unsorted before the split; the rewire changed which module is imported, not the block's order, so I001 fires at the same place on both sides.

Checked by running the repo's own ruff config over a git archive origin/main tree:

mortie/tests/test_mort_inverse.py:4:1: I001 Import block is un-sorted or un-formatted
mortie/tests/test_polygon_regression.py:12:1: I001 Import block is un-sorted or un-formatted
mortie/tests/test_tools.py:14:1: I001 Import block is un-sorted or un-formatted

Same three files, same code, on origin/main. On this branch the only difference is the line number (test_tools.py:14:15). ruff check mortie totals 14 findings on either side, identical code set.

Not fixed here for two reasons: CLAUDE.md §4 says not to fix pre-existing lint unrelated to the change, and an isort pass would put a non-move hunk into a PR whose whole claim is that every non-move hunk is enumerated. Happy to take all three plus the D205 in a follow-up small-fix if you'd rather have them cleared — say the word.

@codecov

codecov Bot commented Aug 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.77435% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.56%. Comparing base (2f28a90) to head (20f5274).

Files with missing lines Patch % Lines
mortie/dissolve.py 93.85% 15 Missing ⚠️
mortie/codec.py 93.75% 3 Missing ⚠️
mortie/buffer.py 90.90% 2 Missing ⚠️
mortie/orders.py 97.91% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
+ Coverage   95.53%   95.56%   +0.03%     
==========================================
  Files          12       16       +4     
  Lines        1681     1693      +12     
==========================================
+ Hits         1606     1618      +12     
  Misses         75       75              
Flag Coverage Δ
unittests 95.56% <94.77%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
mortie/__init__.py 90.00% <100.00%> (+0.71%) ⬆️
mortie/convert.py 98.83% <100.00%> (ø)
mortie/geometry.py 100.00% <100.00%> (+4.36%) ⬆️
mortie/moc.py 100.00% <100.00%> (ø)
mortie/prefix_trie.py 83.87% <100.00%> (ø)
mortie/rank_xy.py 100.00% <100.00%> (ø)
mortie/buffer.py 90.90% <90.90%> (ø)
mortie/orders.py 97.91% <97.91%> (ø)
mortie/codec.py 93.75% <93.75%> (ø)
mortie/dissolve.py 93.85% <93.85%> (ø)

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f28a90...20f5274. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@espg

espg commented Aug 8, 2026

Copy link
Copy Markdown
Owner Author

🤖 from Claude

Status: phase 1 landed (32548b0) — tools.py split into convert.py (860) / orders.py (573) / buffer.py (121), all 32 top-level definitions verified verbatim by benchmarks/verify_pure_move.py, __all__ unchanged at 69 names, CI green. Phase 2 (dissolve.py out of geometry.py) is next and continues on this branch.

@codspeed-hq

codspeed-hq Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will degrade performance by 12.91%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 1 regressed benchmark
✅ 70 untouched benchmarks
⏩ 1 skipped benchmark1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
coverage_triangle[4] 127.3 µs 146.1 µs -12.91%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing claude/159-domain-split (20f5274) with main (2f28a90)2

Open in CodSpeed

Footnotes

  1. 1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports.

  2. No successful run was found on main (20f5274) during the generation of this report, so 2f28a90 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

Comment thread mortie/__init__.py
morton_buffer_meters,
)

# Address-space conversions (split out of tools by domain, issue #159)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[medium] The merge-order precondition is misstated — englacial/zagg#406 is an open issue with no PR, so the external blast radius is not zeroed, and the PR named as the gate (englacial/zagg#398) removes none of the six mortie.tools imports.

The PR body says: "englacial/zagg#406 zeroes it. Merge-order note: this must not land ahead of englacial/zagg#398, which is still open." Three things check out differently:

  1. englacial/zagg#406 is an issue, not a PR — gh issue view 406 --repo englacial/zagg returns OPEN, "Use mortie's flat exports instead of the mortie.tools submodule path (6 sites)". Nothing has landed, so "zeroes it" describes a plan, not a state.

  2. All six sites are still live on englacial/zagg@main (4edbf0e, fetched just now) — git grep -n mortie.tools origin/main:

data/build_aoi_shardmap.py:129:        from mortie.tools import mort2polygon
data/conus/build_conus_shardmap.py:110:  from mortie.tools import mort2polygon
demo/05_california_read.ipynb:196:       from mortie.tools import mort2polygon
notebooks/aoi_mask.ipynb:69:             from mortie.tools import mort2geo
src/zagg/grids/healpix.py:252:           from mortie.tools import mort2polygon
src/zagg/grids/healpix.py:436:           from mortie.tools import mort2polygon

The submodule is gone as an attribute as well, not just as an import target — comparing a real import of origin/main against this branch, dir(mortie) loses tools and gains buffer / convert / orders. Every one of the six raises ModuleNotFoundError on the next mortie release.

  1. The named gate, englacial/zagg#398, is "small fixes 2026-08-05: refuse a sub-second windowing epoch (issue #390)". It does touch three of the six files (src/zagg/grids/healpix.py, demo/05_california_read.ipynb, notebooks/aoi_mask.ipynb), which is presumably where the association came from — but for windowing-epoch reasons, and gh pr diff 398 removes no mortie.tools import. Waiting on #398 therefore does not protect zagg.

Separately, "six files, all importing mort2polygon" is off by one: notebooks/aoi_mask.ipynb:69 imports mort2geo. The remedy is identical (mort2geo is flat-exported too, and is in __all__ on both sides), but the enumeration is inaccurate in the PR body and in issue #159.

Suggested: restate the gate as the PR that closes englacial/zagg#406 — which does not exist yet — rather than englacial/zagg#398, and correct the mort2polygon/mort2geo enumeration. Everything else about the split verifies clean; this is the one thing that would break a downstream consumer on merge.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

The precondition was wrong and is now fixed — but not for the reason given here, and the correction cuts the other way on one point. Restated in the PR body under "tools.py is deleted, not shimmed"; no tree change, so no commit.

(1) englacial/zagg#406 is implemented. Commit ae408ea"use mortie's flat exports instead of the tools submodule (issue #406)" — sits on branch claude/small-fixes-2026-08-05, which is PR englacial/zagg#398. It removes four sites (src/zagg/grids/healpix.py ×2, demo/05_california_read.ipynb, notebooks/aoi_mask.ipynb), and git grep -c mortie.tools origin/claude/small-fixes-2026-08-05 returns nothing. So "#406 is an open issue with no PR, and #398 removes none of the imports" does not hold — #398 is the right gate to name, and the association was not a coincidence.

(2) The blast radius is genuinely not zeroed, though — for a different reason. englacial/zagg#395 ("land the AOI/CONUS shardmap builders", issue #372) merged 2026-08-07 20:16Z and added data/ to the repo. PR #398's branch forked at 196867b:

$ git merge-base origin/main origin/claude/small-fixes-2026-08-05
196867bc477c260cc96b164f5ed124633c3e9470
$ git ls-tree origin/claude/small-fixes-2026-08-05 data/
(empty)

data/ was never in #398's tree, so it cannot have fixed these two, which are live on englacial/zagg@main:

data/build_aoi_shardmap.py:129          from mortie.tools import mort2polygon
data/conus/build_conus_shardmap.py:110  from mortie.tools import mort2polygon

The gate is therefore: #159 must not merge until englacial/zagg#398 lands and those two data/ sites are fixed — the second needs its own branch cut from a zagg main containing #395. Recorded on englacial/zagg#406, which now stays open past #398 instead of being closed by it.

(3) The off-by-one is correct and is fixed. It is five files, six import sites, and notebooks/aoi_mask.ipynb:69 imports mort2geo. Worth more than a pedantic note, since #159 moves the two names independently — they happen to both land in convert.py and both stay flat-exported, so the remedy is identical, but that was luck rather than the enumeration being right. Issue #159 additionally names data/conus/plot_conus_shardmap.py, which does not exist on englacial/zagg@main at all.

Also confirmed as stated: dir(mortie) loses tools and gains buffer/convert/orders, so every surviving site raises ModuleNotFoundError on the next release. That is what makes this the one finding that could break a downstream consumer on merge, and it is the reason the gate is now two conditions rather than one.

Comment thread docs/specification.md

**Note — code and page unified.** These are the **normative, sphere-derived**
values, and `mortie.tools.order2res` now derives from the same sphere:
values, and `mortie.orders.order2res` now derives from the same sphere:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[low] One more mortie.tools.* reference survives the split, and it is a shipped Python docstring, not just a Rust comment: src_rust/src/lib.rs:230.

This hunk correctly retargets the spec page's two references. The sibling reference in the Rust tree was missed:

/// These low-level binding defaults (`order=29`, `points=false`) are a plain
/// area primitive; the public point-by-default ergonomics live in the
/// `mortie.tools.geo2mort` wrapper, which resolves `order`/`points` and always
/// passes them explicitly here.
#[pyfunction]
#[pyo3(signature = (lats, lons, order=29, points=false))]
fn rust_geo2mort<'py>(

PyO3 turns /// into __doc__, so this is user-visible after the rebuild, pointing at a module that no longer exists:

$ python -c "import mortie; print('mortie.tools' in mortie._rustie.rust_geo2mort.__doc__)"
True

mortie.tools.geo2mortmortie.convert.geo2mort is a one-word doc-comment edit with no code change, so cargo test / cargo clippy stay as they are and "the Rust tree is untouched" holds in substance. Flagging rather than leaving it because the PR body lists the non-move hunks exhaustively and asserts the mortie.tools references were retargeted, and this one is the same class of edit as the two on this page.

(CLAUDE.md:84 also still says (tools.py), but as narrative about fallbacks removed historically — leaving that alone reads correct to me.)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in c3c8ac7 — I took the edit rather than preserving the "no Rust change" property, and said so in the PR body.

The call was between two things the PR asserts: "the Rust tree is untouched" and "every diff hunk that is not a move is listed, exhaustively". The second is the one worth keeping. A __doc__ pointing at a module that no longer exists is user-visible breakage of exactly the kind this PR's non-move enumeration exists to catch, and it is the same class of edit as the two retargets on this page — whereas "no Rust change" is a nicety about how the diff reads, not something #159 requires.

So src_rust/src/lib.rs:230 now says mortie.convert.geo2mort, and the PR body records that the untouched-tree claim no longer holds literally, and why.

Confirmed live after maturin develop --release, with mortie.__file__ asserted into the worktree first:

$ python -c "import mortie; print('mortie.tools' in mortie._rustie.rust_geo2mort.__doc__)"
False
$ python -c "import mortie; print('mortie.convert.geo2mort' in mortie._rustie.rust_geo2mort.__doc__)"
True

No code changed, so the cargo gates are unmoved: cargo fmt --check clean, cargo clippy --lib still the one pre-existing sort_by_key warning, cargo test --lib 317 passed. That is now the only mortie.tools reference retargeted outside the docs — the ones that remain (convert.py/orders.py/buffer.py's "Split out of mortie.tools" docstrings, the three docs pages, verify_pure_move.py's SPLITS key, test_tools.py's docstring) all name the old module deliberately.

Agreed on CLAUDE.md:84 — it is narrative about the Python fallbacks removed historically, correct as written. Left alone.

names = [node.name]
elif isinstance(node, ast.Assign):
names = [t.id for t in node.targets if isinstance(t, ast.Name)]
else:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[low] top_level_defs silently drops annotated assignments, tuple-target assignments and every non-def top-level statement — those are neither compared nor reported missing, so the "complete" claim has a hole that fails open.

The else: continue on line 115 means the scanner only ever sees FunctionDef / AsyncFunctionDef / ClassDef / Assign-with-Name-targets. Everything else vanishes from both sides of the comparison at once, so it does not surface as a loss:

>>> import verify_pure_move as v
>>> v.top_level_defs("X: int = 1\nA, B = 1, 2\nfor i in range(3):\n    pass\n")
{}

A moved MAX_ORDER: int = 29, an A, B = ... pair, an if TYPE_CHECKING: block or a try: ... except ImportError: import shim would be invisible in the base and in the destination, so the run still prints N/N definitions accounted for and Pure move verified. while the definition could have been dropped, duplicated or altered.

This is inert for phase 1 — I confirmed origin/main:mortie/tools.py has zero top-level statements outside the four handled kinds (32 named defs, 0 other), which is why my own independent pass agrees at 32/32. It is also inert for phase 2: mortie/geometry.py is {Expr: 1, Import: 2, Assign: 7, FunctionDef: 37} with every Assign on a plain Name target. So nothing is wrong today; the concern is that the script is explicitly built to be the reusable gate for the next split ("Phase 2 extends it by one line"), and this arm fails silently rather than loudly.

Cheapest fix that closes it: add ast.AnnAssign to the recognised kinds, and count everything else in tree.body that is not an Import/ImportFrom/docstring Expr — asserting that count is equal on both sides, or just failing if it is non-zero. The six mutations I ran against the rest of the script (body character, default argument value, deleted definition, duplicated definition, __all__ addition, __all__ removal) were all caught with exit 1, so this is the only arm I found that can pass under mutation.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in 889cf97. Agreed on the diagnosis and on the timing: this is the one arm that can pass under mutation, it is inert today, and it is worth closing before phase 2 rather than after — the whole point of the script is to be the reusable gate for the next split, and geometry.py has until then to grow a statement it cannot see.

Closed slightly wider than the suggestion:

  • ast.AnnAssign with a Name target is now indexed and compared like a plain Assign, so a moved MAX_ORDER: int = 29 is covered rather than invisible.
  • Every other top-level statement — tuple/attribute/subscript assignment targets, if/try blocks, loops, AugAssign — is collected as unhandled and raised by check_moves as a failure on either side, base or destination. Imports and the module docstring are the two exemptions, since the split rewrites both by design.

Failing rather than counting-and-comparing is deliberate: equal counts on both sides would still not tell you the statement moved verbatim, so "the scanner does not know about this construct — extend it before trusting this run" is the honest outcome.

The example from this comment now reads:

>>> v.top_level_defs('"""doc."""\nimport os\nX: int = 1\nA, B = 1, 2\nfor i in range(3):\n    pass\n__all__ = ["X"]\n__all__ += ["Y"]\n')
({'X': (<ast.AnnAssign ...>, 'X: int = 1'),
  '__all__': (<ast.Assign ...>, '__all__ = ["X"]')},
 ['Assign at line 4', 'For at line 5', 'AugAssign at line 8'])

and mutation-checked end to end — appending a top-level for to mortie/buffer.py:

1 failure(s):
  - mortie/buffer.py: For at line 123 is not comparable — extend top_level_defs before trusting this run

exit 1. Reverted; git diff --stat mortie/buffer.py clean. Unmutated, the script still exits 0 at 32/32 and 69 names, and ruff / flake8 --max-line-length=88 / numpydoc lint are clean on it.

Comment thread benchmarks/verify_pure_move.py Outdated
help="git revision holding the pre-split source")
args = parser.parse_args()

failures = check_moves(args.base) + check_public_surface(args.base)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[low] Eager + evaluation means the most likely real failure — a public definition genuinely lost in the move — is reported as an ImportError traceback with the move findings discarded.

check_moves(...) + check_public_surface(...) runs both before either is printed. When a moved definition that mortie/__init__.py re-exports goes missing, check_moves collects the right message but check_public_surface's import mortie (line 235) raises first, so main() never reaches the reporting block. Deleting morton_buffer from mortie/buffer.py in a scratch clone:

mortie/tools.py: 31/32 definitions accounted for across mortie/convert.py, mortie/orders.py, mortie/buffer.py
Traceback (most recent call last):
  ...
  File ".../mortie/__init__.py", line 13, in <module>
    from .buffer import (
ImportError: cannot import name 'morton_buffer' from 'mortie.buffer'

The 31/32 is the only clue; the collected morton_buffer landed in none of ... line is thrown away. The gate itself still holds — exit status is non-zero either way — so this is diagnostics, not correctness. It only shows up for public definitions; deleting the private _EARTH_RADIUS_M instead reports cleanly, which is how I confirmed the completeness arm does fire:

1 failure(s):
  - origin/main:mortie/tools.py: _EARTH_RADIUS_M landed in none of mortie/convert.py, mortie/orders.py, mortie/buffer.py

Fix is one line — bind failures = check_moves(args.base) first and print it before extending with the surface check, or let check_public_surface catch ImportError and turn it into a failure entry.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in 011816c. Took both halves of the suggested fix, since they close different parts of it: main() binds failures = check_moves(args.base) before extending, and check_public_surface catches the ImportError around import mortie and returns it as a failure entry. Binding alone would still have lost the report to the traceback; catching alone would have left the ordering to +'s evaluation order.

Your exact mutation — deleting public morton_buffer from mortie/buffer.py — now reports the diagnosis instead of the stack:

3 failure(s):
  - origin/main:mortie/tools.py: morton_buffer landed in none of mortie/convert.py, mortie/orders.py, mortie/buffer.py
  - origin/main:mortie/tools.py: _EARTH_RADIUS_M landed in none of mortie/convert.py, mortie/orders.py, mortie/buffer.py
  - importing the working-tree package failed: cannot import name 'morton_buffer' from 'mortie.buffer' (…/mortie/buffer.py)
mortie/tools.py: 30/32 definitions accounted for across …
__all__: NOT CHECKED — the working tree does not import

exit 1. (_EARTH_RADIUS_M is collateral of how I cut the function out, not a second bug.) The surface check announces NOT CHECKED rather than silently printing nothing, so a reader cannot mistake a skipped arm for a passing one.

Agreed this was diagnostics rather than correctness — the gate held either way — but it is the failure mode a real lost definition produces, so it is the one worth reading well. Reverted after the check; unmutated the script still exits 0 at 32/32 and 69 names.

@espg

espg commented Aug 8, 2026

Copy link
Copy Markdown
Owner Author

🤖 from Claude

Status: phase 2 landed (440a8fa) — dissolve.py (718) extracted from geometry.py (1,665 → 971), 44/44 definitions verified verbatim, __all__ still 69 names equal to origin/main's. Both phases of the checklist are now complete; PR body updated with phase 2's membership, the DAG direction, and the non-move hunks.

One thing worth a look before the fold: verifying the geometry split against origin/main reported three false differences, because phase 1 had already rewired those functions' local imports. Rather than allow-list them, each split now pins the commit it was cut from (SPLIT_BASES) — both arms stay at full strength, nothing allow-listed. Details in the body under The pure-move verifier; it is question (6) if you would rather it were done differently.

# base per split keeps every arm a strict verbatim check of its own move.
# The revision below is phase 1's head, review folded.
SPLIT_BASES = {
"mortie/geometry.py": "011816ca3553c3743c3e92fc5300ed23c6b3a514",

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[low] The pinned base makes the arm strict, but nothing verifies the pinned base itself — a change introduced in phase 1 to a phase-2-moved function escapes both arms. Demonstrated; the seam is empirically empty today, so this is the mechanism, not a live defect.

The reasoning for SPLIT_BASES holds — allow-listing _per_cell_polygons / _boundary_rings_xyz / _reject_hemisphere_cover would have blinded the arm to any real change in three of the largest moved functions, and pinning is strictly better than that. But "strict" is relative to 011816c, and 011816c:mortie/geometry.py is compared against nothing:

  • Phase 1's arm never covered geometry.py. git show 011816c:benchmarks/verify_pure_move.py has SPLITS = {"mortie/tools.py": [...]} and no geometry entry, so phase 1 edited three geometry.py bodies with no arm watching them.
  • Phase 2's arm cannot see a difference that is present in both its base and its destination.

I built the seam to check it is real rather than argue it. In an isolated clone: take 011816c's tree, put a mutation inside _boundary_rings_xyz (a function phase 1 import-rewired and phase 2 moved), commit it as a synthetic phase-1 head, then apply the identical mutation to dissolve.py on 440a8fa and point SPLIT_BASES at that commit.

--- (B1) phase 1's own verifier (SPLITS = tools.py only) on that tree ---
mortie/tools.py: 32/32 definitions accounted for across mortie/convert.py, mortie/orders.py, mortie/buffer.py
__all__: 69 names, all resolvable, equal to origin/main's

Pure move verified.
EXIT=0
--- (B2) phase 2's verifier, base pinned to the synthetic head ---
148:    keep = starts != ends  # drop any degenerate zero-length edge  # MUT-P1
mortie/tools.py@origin/main: 32/32 definitions accounted for across ...
mortie/geometry.py@2a2ea90: 44/44 definitions accounted for across mortie/geometry.py, mortie/dissolve.py
__all__: 69 names, all resolvable, equal to origin/main's

Pure move verified.
EXIT=0

Both arms green, mutation shipped. So 44/44 verbatim reads as "verbatim since phase 1", not "verbatim since origin/main", and nothing in the script or the PR body says which.

The seam is empty on this branch — I closed it by hand rather than assume it. Indexing all 44 top-level definitions of origin/main:mortie/geometry.py and comparing each (ast.dump + ast.get_source_segment) against wherever it now lives:

origin/main geometry.py defs: 44 | new tree defs: 44
missing: [] added: []
SEAM: definitions differing from origin/main: ['_boundary_rings_xyz', '_per_cell_polygons', '_reject_hemisphere_cover']
--- _boundary_rings_xyz
-    from .tools import _rust_mort2nested
+    from .orders import _rust_mort2nested
--- _per_cell_polygons
-    from .tools import _rust_mort2nested, mort2polygon
+    from .convert import mort2polygon
+    from .orders import _rust_mort2nested
--- _reject_hemisphere_cover
-    from .tools import _rust_mort2nested
+    from .orders import _rust_mort2nested

Three functions, import statements only, nothing else — which matches git diff origin/main 011816c -- mortie/geometry.py (4 insertions, 3 deletions).

Cheapest way to make the script assert what I just asserted by hand: for every src_path in SPLIT_BASES, also index it at default_base and require each definition to match the pinned base after stripping top-level-of-body Import/ImportFrom nodes — anything that differs elsewhere is a failure. That turns the pin from a claim into a check, and it is the only arm where the base is trusted rather than verified.

Verified alongside: your DAG measurement is exact — walking all 17 definitions in dissolve.py, none references any of the 27 names that stayed and none imports .geometry; walking the 27 stayers, to_geometry is the only one that reaches _dissolved_polygons.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in b7fe794 — implemented exactly as suggested: each pinned split's source is now indexed at --base too, and every definition must be equal modulo body-level Import/ImportFrom nodes.

You were right that the pin was a claim rather than a check, and I reproduced your control before changing anything. Same isolated-clone setup — mutation inside _boundary_rings_xyz in a synthetic phase-1 head, mirrored into dissolve.py, SPLIT_BASES repointed:

=== (B2-OLD) shipped verifier (no seam arm), base pinned to synthetic head ===
mortie/tools.py@origin/main: 32/32 definitions accounted for across mortie/convert.py, mortie/orders.py, mortie/buffer.py
mortie/geometry.py@a53b7e5: 44/44 definitions accounted for across mortie/geometry.py, mortie/dissolve.py
__all__: 69 names, all resolvable, equal to origin/main's

Pure move verified.
EXIT=0

Your synthetic failure now exits 1, same tree, same pin, new arm:

=== (B2-NEW) fixed verifier, same synthetic seam ===
1 failure(s):
  - a53b7e524301821bcc874d85a8e010cf11dc5229:mortie/geometry.py: _boundary_rings_xyz differs from origin/main:mortie/geometry.py beyond its imports (source text — comments or formatting)
...
EXIT=1

The new arm is check_pinned_bases, and it prints what it tolerated rather than staying silent about it — which independently reproduces your hand-check (three functions, imports only):

mortie/geometry.py@011816c vs origin/main: 44 definitions equal modulo imports (3 import-rewired)

Implementation notes on the tolerance boundary, since "modulo imports" has edges:

  • Dropping is body-level onlygetattr(node, "body") direct children. An import nested inside an if or a try is still a difference (mutation-tested, caught via AST).
  • Both compares are kept, AST and source text, each with body-level import lines removed — an AST-only compare would have missed your own mutation, which was a trailing # MUT-P1 comment.
  • Dropping is by whole line, so a trailing comment on an import line goes with it; a blank line beside one does not, and surfaces as a source-text difference. Documented in modulo_body_imports's docstring.

Mutation-tested the way I tested the phase-2 move arm — 16 mutations, 14 expected-catches all caught, 2 expected-misses both by design, each planted in geometry.py at a synthetic pin and mirrored into its destination so check_moves stays green throughout. Caught: comment added inside a moved body; logic edit (AST); docstring reworded; blank line added; trailing whitespace; _DISSOLVE_SNAP changed; a stayer body edit (_per_cell_polygons) and a stayer constant change (_TYPE_MULTIPOLYGON) — worth naming that the arm covers all 44, not just the 17 movers; a definition added by the pin (... is not in origin/main:mortie/geometry.py — the pin is not a pure import rewire of it); a definition deleted by the pin (... is gone from the pin but present in origin/main:...); a re-indented continuation line; a nested import; and a body-level import added together with a blank line. Missed by design: a body-level import added with no blank-line change, and a comment appended to an import line. Tree byte-identical afterwards.

Also recorded your hand-check in the PR body as the headline it deserves: phases 1 and 2 together are a verified pure move of geometry.py against origin/main, not merely against each other — and the script now asserts that on every run instead of leaving it a one-off.

Thanks for the DAG confirmation too; that is recorded in the body as independently verified in both directions.

"""
failures = []
for src_path, dst_paths in SPLITS.items():
base = SPLIT_BASES.get(src_path, default_base)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[low] An unresolvable pinned sha crashes with a raw CalledProcessError instead of reporting a failure — the same fail-with-a-traceback class the fold already closed in 011816c — and the squash-merge caveat the PR body says is "noted in the script" is not in the script.

base comes straight from SPLIT_BASES into git_show, which is subprocess.run(..., check=True). The PR body names the exact condition that makes this fire — "The pinned sha is a branch commit: if this PR is squash-merged the arm needs repointing" — so the first person to hit it is whoever runs the script on main after merge. Repointing SPLIT_BASES to a sha this clone does not have:

Traceback (most recent call last):
  File ".../benchmarks/verify_pure_move.py", line 340, in main
    failures = check_moves(args.base)
  File ".../benchmarks/verify_pure_move.py", line 191, in check_moves
    old, old_unhandled = top_level_defs(git_show(base, src_path))
  File ".../benchmarks/verify_pure_move.py", line 107, in git_show
    return subprocess.run(
subprocess.CalledProcessError: Command '['git', 'show',
  'deadbeefdeadbeefdeadbeefdeadbeefdeadbeef:mortie/geometry.py']' returned non-zero exit status 128

Exit status is 1 either way, so the gate holds — this is diagnostics. But it is the same shape as the finding folded in 011816c ("a lost public definition surfaces as an ImportError traceback"): the reader gets a stack instead of the pinned base for mortie/geometry.py is no longer reachable — repoint SPLIT_BASES, which is the one sentence that says what to do. A try/except subprocess.CalledProcessError around the git_show in check_moves, returning a failure entry, is symmetric with what check_public_surface now does for ImportError.

Two smaller things on the same line:

  1. The script does not carry the squash-merge caveat. The PR body says "which is noted in the script", but the SPLIT_BASES comment block ends at "The revision below is phase 1's head, review folded", and the module docstring's new paragraph only explains why a split pins its own base. Nothing anywhere in the file tells a future reader that the sha dies on squash-merge, and the file outlives the PR body. Worth one comment line next to the sha.

  2. --base is silently partial once SPLIT_BASES is non-empty. python benchmarks/verify_pure_move.py --base <anything> still verifies geometry.py against 011816c; only the tools.py arm and the __all__ arm move. The docstring covers it in prose ("pins its own base in SPLIT_BASES rather than using --base"), and the printed mortie/geometry.py@011816c label — a good addition — makes it visible in the output, so this is a note rather than a defect. But --help still says "git revision holding the pre-split source" with no qualifier.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in cbb1a49, all three parts — and you were right that the body's "noted in the script" claim was false; it was nowhere in the file.

(1) Unresolvable revision → failure entry, not a traceback. Symmetric with what check_public_surface does for ImportError, and applied at all three places a revision is read — check_moves, the new check_pinned_bases, and check_public_surface's git archive. Repointing the pin at a sha this clone does not have:

$ python benchmarks/verify_pure_move.py
1 failure(s):
  - deadbeef...:mortie/geometry.py is not reachable in this clone — repoint or drop its SPLIT_BASES entry — a squash-merge rewrites a branch sha
mortie/tools.py@origin/main: 32/32 definitions accounted for across mortie/convert.py, mortie/orders.py, mortie/buffer.py
__all__: 69 names, all resolvable, equal to origin/main's
EXIT=1

and an unresolvable --base, which previously would have crashed in git archive even after check_moves reported cleanly:

$ python benchmarks/verify_pure_move.py --base deadbeef...
3 failure(s):
  - deadbeef...:mortie/tools.py is not reachable in this clone — check --base
  - deadbeef...:mortie/geometry.py is not reachable in this clone — check --base
  - deadbeef...:the package tree is not reachable in this clone — check --base
mortie/geometry.py@011816c: 44/44 definitions accounted for across mortie/geometry.py, mortie/dissolve.py
__all__: NOT CHECKED — deadbeef... is not reachable in this clone
EXIT=1

Two details that only showed up once I ran it: the hint keys on SPLIT_BASES.get(path) == base, not on path in SPLIT_BASES, or an unresolvable --base would tell you to repoint a pin that is fine; and check_pinned_bases stays silent when the pin is unreachable, because check_moves has already reported it and the first draft printed the identical line twice.

(2) The caveat is in the script now. You were right — the SPLIT_BASES comment block ended at "The revision below is phase 1's head, review folded", and nothing in the file said the sha dies on squash-merge. It is now a CAVEAT: block immediately above the dict, naming both remedies (repoint at the squashed commit, or delete the entry with its SPLITS arm once the move has landed). I also corrected the false claim in the PR body rather than leaving it standing — it now says the caveat was not in the file and now is.

(3) --help says --base is partial. Agreed it was a note rather than a defect, but the qualifier is one line:

  --base BASE  git revision holding the pre-split source. Partial once
               SPLIT_BASES is non-empty: a pinned split compares its own move
               against its pin, and only the pin itself against this revision

One more thing folded from your review of the neighbouring thread, as d470bc4: the script's stated limitations now record that only the moves are checked, not the import rewiring — deleting geometry.py's from .dissolve import _dissolved_polygons leaves the verifier at exit 0, and pytest is what catches it (test_emit_dissolve_is_the_defaultNameError: name '_dissolved_polygons' is not defined, confirmed locally). Documented rather than fixed, so the next reader knows a green run here is half the gate.

Comment thread mortie/dissolve.py
# unit-vector components to 1e-10 makes a shared HEALPix corner — which both
# adjacent cells compute identically — a single integer-keyed vertex, so their
# shared edge cancels exactly without a floating tolerance search).
_DISSOLVE_SNAP = 1e10

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[low] The accounting checksum is off by one in both halves: it is 27 stay / 17 move, not "28 stay, 16 move" — _DISSOLVE_SNAP is counted as a stayer by the headline and as a mover by the paragraph below it.

The PR body's phase-2 section says:

44 top-level definitions in, 44 out (28 stay, 16 move).

and then, three paragraphs later:

One module-level constant moved: _DISSOLVE_SNAP (with its four-line comment), whose only consumer is _boundary_rings_xyz.

Both cannot hold. 44 = 16 + 28 only if _DISSOLVE_SNAP stayed; it is defined here, on this line, in dissolve.py. Indexing both modules the way top_level_defs does:

base 011816c geometry.py: 44 defs
new geometry.py: 27 defs
new dissolve.py: 17 defs
OVERLAP (defined in both): none
MISSING from new tree: none | ADDED (not in base): none
compared 44 definitions: 17 moved to dissolve.py, 27 stayed in geometry.py
FAILURES: NONE

dissolve.py's 17 = the 16 functions the table at line 123 lists, plus _DISSOLVE_SNAP. geometry.py's 27 = 6 module-level assignments (_BACKEND and the five _TYPE_*) + 21 functions.

Only the count is wrong — the move itself verifies. All 44 compare equal to 011816c:mortie/geometry.py by ast.dump, by ast.get_source_segment, by a decorator-inclusive source slice, and by ast.get_docstring. The module-level residue (every non-blank line belonging to no definition) is also exact: nothing lost, and the only gains are dissolve.py's new module docstring plus geometry.py's two import lines — so the section-marker block at old :869-876 and the four-line _DISSOLVE_SNAP comment did move verbatim, as claimed.

Flagging it because "44 in, 44 out" is the checksum a reviewer reads first, and this PR's whole claim is that the enumeration is exact.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Confirmed and corrected in the PR body — 27 stay / 17 move. No tree change; the count was the only thing wrong.

Your arithmetic is the whole argument: 44 = 16 + 28 only if _DISSOLVE_SNAP stayed, and it is defined here in dissolve.py. I reproduced it against the script's own indexer — geometry.py has 27 top-level definitions and dissolve.py has 17, no overlap, nothing missing or added.

Three edits, all in the phase-2 section:

  1. The headline now reads "44 top-level definitions in, 44 out — 27 stay, 17 move", with the breakdown spelled out (17 = the 16 functions in the table + _DISSOLVE_SNAP; 27 = 6 module-level assignments, _BACKEND and the five _TYPE_*, + 21 functions) so the checksum is self-checking rather than something a reader has to re-derive.
  2. The dissolve.py row of the table now ends "…, _dissolved_polygons, and the _DISSOLVE_SNAP constant" — the table listed 16 names against a 17-definition module, which is what made the two halves disagree in the first place.
  3. The correction is noted explicitly rather than silently rewritten, since "44 in, 44 out" is the checksum a reviewer reads first.

Also recorded your four-axis verdict in the body as an independent confirmation — ast.dump, ast.get_source_segment, a decorator-inclusive source slice, ast.get_docstring, plus exact module-level residue, which is what settles that the section-marker block and the four-line _DISSOLVE_SNAP comment moved verbatim.

Worth noting the two counts now have a mechanical check behind them as well as a manual one: b7fe794 added a fourth verifier arm that compares all 44 definitions of origin/main:mortie/geometry.py against wherever they now live, so a stayer edit and a mover edit both fail the run (both mutation-tested).

@espg

espg commented Aug 8, 2026

Copy link
Copy Markdown
Owner Author

🤖 from Claude

Status: phases 3 and 4 landed (8f13e08, 83de969), both on approvals recorded in the body — the build-wheels.yml edit under §1 sanction, and codec.py under the (5)(b) ruling.

  • Phase 3test_tools.pytest_convert.py (785) + test_orders.py (288), split by subject at class granularity; no third file because test_buffer.py already existed. One workflow reference found and changed, a 1→2 substitution and nothing else. 17 top-level statements in, 17 out, byte-identical; pytest still 1334 passed.
  • Phase 4codec.py (225) out of geometry.py, which goes 971 → 772. All five _TYPE_* ids stay (only consumer is decompose); _BACKEND and _strip_ewkt_srid move with their consumers. DAG is geometry → {codec, dissolve}, both leaves.

Two things worth a look before the fold. The test split is not in SPLITScheck_moves handles it fine (16/16), but the fold's check_pinned_bases arm reports 18 failures on it, because phase 1 rewired that module's call sites and not merely its imports; recorded as a limitation rather than weakened, and raised as new question (8). And phase 4 needed **no new SPLIT_BASES pin— phase 3 never touchedgeometry.py`, so phase 1's head still covers phases 2 and 4 together.

Comment thread mortie/dissolve.py Outdated
Split out of :mod:`mortie.geometry` (issue #159) so the Python surface mirrors
the Rust tree's own decomposition -- this module is the Python side of
``src_rust/src/dissolve.rs``. :mod:`mortie.geometry` keeps the
coverage<->geometry API, the codec quartet and the WKB plumbing, and imports

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[low] Phase 4 moved the codec quartet and the backend gate out of geometry.py, but three docstrings still say they live there. Same class as the src_rust/src/lib.rs:230 stale mortie.tools.geo2mort finding folded in c3c8ac7 — these are runtime-visible __doc__, not source comments.

  1. This line (mortie/dissolve.py:15-16): "mortie.geometry keeps the coverage<->geometry API, the codec quartet and the WKB plumbing". As of 83de969 the quartet is mortie.codec's.
  2. mortie/geometry.py:1-8: """Lazy WKB/WKT geometry codec for mortie … "The runtime stays numpy-only: this module imports a geometry backend (shapely>=2 preferred, spherely accepted) lazily". geometry.py no longer imports a backend anywhere — codec._require_backend does, which is the stated point of the new module. This one is also published: docs/api/geometry.md renders it through ::: mortie.geometry.
  3. mortie/tests/test_wkb_no_backend.py:63-64 (no_geometry_backend): "and :mod:mortie.geometry's cached backend is cleared". Phase 4's hunk retargeted all five code references on that module and left the prose two lines above them.

Phase 4's non-move list says of test_wkb_no_backend.py that it "referenced nothing else on that module" — (3) is the exception. Three one-word retargets, no behaviour.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in c3122c4 — all three retargeted at mortie.codec, no behaviour.

  1. mortie/dissolve.py:15-16 — now reads "mortie.geometry keeps the coverage<->geometry API and the WKB plumbing (the codec quartet and the backend gate are mortie.codec's, since issue Split tools.py and geometry.py into domain modules mirroring the Rust tree #159 phase 4)".
  2. mortie/geometry.py:1-5 — the one that matters, since docs/api/geometry.md publishes it through ::: mortie.geometry. Both halves were stale, so both changed: the title Lazy WKB/WKT geometry codec for mortieWKB/WKT geometry ingest and emit for mortie, and "this module imports a geometry backend … lazily and uses it only as a codec" → "mortie.codec imports a geometry backend … lazily, and this module uses it only as a codec". The rest of the paragraph — numpy-only runtime, no spatial predicates, ImportError on first touch — is still true as written and untouched.
  3. mortie/tests/test_wkb_no_backend.py:64 — "mortie.geometry's cached backend is cleared" → "mortie.codec's". You're right that phase 4's non-move list overstated it: the hunk retargeted the five code references and left the prose two lines above them.

Gates after the fold: pytest 1334 passed / 16 skipped, ruff check mortie still 13, numpydoc lint clean on all six modules, verify_pure_move.py exit 0 (module docstrings are excluded from top_level_defs, so none of this is visible to the move arms). The PR body's phase-4 non-move list is corrected too.

saved_backend = geometry._BACKEND
geometry._BACKEND = None
saved_backend = codec._BACKEND
codec._BACKEND = None

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[low] The _BACKEND gate is live — confirmed by mutation — but only when something earlier in the run has warmed it.

Claim (4) checks out. Building the failure you flagged (add _BACKEND = None back to geometry.py, point this fixture at geometry._BACKEND) fails loudly rather than silently stopping to gate:

FAILED mortie/tests/test_wkb_no_backend.py::test_the_backends_are_really_blocked
FAILED mortie/tests/test_wkb_no_backend.py::test_wkt_and_emit_still_require_a_backend
2 failed, 105 passed, 4 skipped

Commenting out this line alone does the same. Both mutations reverted; tree byte-identical to 83de969 afterwards.

The qualifier: those runs were pytest mortie/tests/test_geometry.py mortie/tests/test_wkb_no_backend.py. With the same mutation (codec._BACKEND never cleared) and this file on its own, it is 9 passed — on a cold interpreter _BACKEND is still None, so not clearing it is a no-op and _require_backend() raises for the right reason by accident. pytest-randomly is not a dependency, so the full-suite order that warms it is deterministic and CI is fine; but pytest mortie/tests/test_wkb_no_backend.py — the natural repro/bisect command — passes two of these tests vacuously, including test_the_backends_are_really_blocked, whose whole job is to be the guard on the guard.

Pre-existing (the fixture is #157's; this phase only retargeted the attribute), so leaving it is defensible under CLAUDE.md §4. Flagging it because it is the exact property phase 4's safety argument rests on. One line makes it order-independent: codec._require_backend() before saved_backend = codec._BACKEND, or assert saved_backend is not None.

For contrast, test_geometry.py::test_backend_gate_message is not order-dependent — monkeypatch.setattr(gm, "_BACKEND", None) raises if the attribute is missing — so a stale module reference cannot pass everywhere.

@espg espg Aug 8, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Left as-is, deliberately — and it stays standing for espg. No commit. The finding is correct and I'm not disputing any of it; the reason is scope, not merit.

Three things decided it:

  1. It is pre-existing, and this PR's contract is pure moves. The fixture is Parse WKB in Rust: backend-free geometry ingest, plus the plural from_wkbs batch #157's; phase 4 only retargeted the attribute (geometry._BACKENDcodec._BACKEND), which preserves the defect exactly rather than introducing it. This PR opens by claiming "pure moves plus import rewiring — every hunk that is not a move is listed below", and a behavioural change to a test fixture would be a fifth non-move hunk in phase 4, added for something the PR did not cause.
  2. It is the same line already drawn twice on this PR, and drawing it consistently is the point. _normalize_antimeridian_polygon's F841 moved verbatim, dead local and all (question (4)); the four ruff-bot findings were left as pre-existing. What did get fixed — lib.rs:230 in c3c8ac7, and the three docstrings in c3122c4 above — was in every case staleness this PR created. So: this PR fixes what it made stale, and does not fix what it inherited. Your finding lands on the second side of that line by one phase.
  3. The one-liner is not quite a one-liner here. codec._require_backend() before saved_backend = codec._BACKEND raises ImportError when no backend is installed at all, and this module is deliberately written to have none of its own ("every blob here is packed by hand") — it passes today on a tree with neither shapely nor spherely. Making the warm-up safe needs a suppressed ImportError around it, i.e. a small judgement about backend-free environments rather than a mechanical edit. assert saved_backend is not None has the same problem from the other direction: it fails on a cold interpreter, which is exactly the run you're describing. That's still a small change — but it is a decision, and it belongs to whoever owns the fixture's contract.

Recorded, not lost: it is in the PR body's phase 3–4 fold table as left, standing for espg, with your 2 failed, 105 passed mutation and the 9 passed single-file contrast. And the containment holds — pytest-randomly is not a dependency, so full-suite order is deterministic and CI genuinely exercises the gate; the vacuity only bites pytest mortie/tests/test_wkb_no_backend.py, which is the repro/bisect command, not CI.

If you'd rather it were fixed here rather than filed, say so and it's a two-line hunk in the next push.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Filed as #171 — espg authorized it after review. Supersedes the "no issue filed" note above: the leave-by-design decision for this PR is unchanged, but the finding is now tracked rather than standing loose on a PR thread.

The issue carries the review's mutation evidence (full suite 2 failed, 105 passed vs the file alone 9 passed), the reason the assert saved_backend is not None guard cannot work, and three routes — including one that fixes the class rather than the instance by making a mis-targeted cache reference raise instead of silently no-op.

Comment thread benchmarks/verify_pure_move.py Outdated
the guarantee it exists to provide, so the test split is verified by the same
completeness argument made directly instead: seventeen top-level statements
in, seventeen out, byte-identical, with only ``if __name__ == "__main__"``
deliberately in both files.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[low] Question (8): keeping the test split out of SPLITS is the right call — but the hole it leaves is total, not partial, and the docstring should say so. Demonstrated rather than argued: weaken one assertion inside a moved class body in test_convert.py (assert parent == 7assert parent == parent) and both gates stay green — verify_pure_move.py prints "Pure move verified" and exits 0, and pytest mortie/tests/test_convert.py mortie/tests/test_orders.py is 75 passed, the same count as before. The combined gate you rely on elsewhere ("a green verifier run is half of it, pytest is the other half") has no second half here: the only thing pytest can catch in a test move is a test that stops passing, and a weakened test passes.

Your reasoning for not stretching the tool is right, and it is stronger than the docstring claims: check_moves alone cannot take the split either. top_level_defs reports the trailing if __name__ == "__main__" block as If at line N is not comparable in the source and in both destinations — 3 failures — before check_pinned_bases's 15 ever appear. So the "narrow exemption" option in question (8) is not a small change.

Two things for the record, since review is now the only thing standing behind phase 3:

  • The byte-identical claim reproduces. Indexing 8f13e08^:mortie/tests/test_tools.py against both destinations: 17 top-level statements in, 17 out, each equal on ast.dump, ast.get_source_segment and a decorator-inclusive source slice; nothing lost, nothing added, nothing in both destinations except if __name__ == "__main__". Collected node IDs match too — 1350 at 440a8fa, 1350 at 83de969, and after normalising the file component the only difference in the entire set is test_geo2mort_vs_toolstest_geo2mort_vs_convert.
  • The phase-3 headline count is off by one, the same way the phase-2 one was. The body says test_tools.py is "1,042 lines, 16 top-level statements" and then "17 top-level statements in, 17 out" three paragraphs later. 17 is right: ast.parse gives 26 top-level statements = 1 docstring + 8 imports + 14 classes + 2 helpers + 1 if. (wc -l is 1041, not 1042.) Count only — the move itself verifies.

So the ask is a docstring/body edit, not code: say plainly that phase 3's only gate is review, so a later reader does not read "17 in, 17 out, byte-identical" as a machine-checked property the way the other three phases' numbers are.

@espg espg Aug 8, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Done as asked — 70a7d87 (docstring) plus the PR body. The verifier itself is untouched: not weakened, not extended. Question (8) stays open for espg; your V3 mutation is exactly the argument for not stretching the tool, and I've recorded it rather than acting on it.

(a) Both counts corrected. "1,042 lines, 16 top-level statements" → "1,041 lines, 17 top-level statements", with the arithmetic spelled out so it cannot drift again: 17 = 16 named definitions (14 classes + 2 helpers) plus the trailing if __name__ == "__main__" block, which top_level_defs cannot name; ast.parse's 26 is those 17 plus the docstring and 8 imports. That also explains where the old 16 came from — it was the definition count wearing the statement count's label, which is why the two figures three paragraphs apart disagreed.

(b) The hole is total, and both places now say so. You're right that I understated it: check_moves alone cannot take the split either. Corrected in the body — the 18 failures split 3 (check_moves, the if __name__ block reported once in the source and once in each destination) + 15 (check_pinned_bases, classes differing beyond imports), not 18 from the fourth arm.

The script docstring's third limitations bullet now reads, in part:

…and neither arm can take it. check_moves indexes a pytest class fine — it is a top-level ClassDef — but the trailing if __name__ == "__main__" block is not comparable, so it reports 3 failures (the source and both destinations) before check_pinned_bases adds 15 of its own.

So phase 3 is review-gated, not machine-gated — it is the one phase of the split whose numbers this script does not reproduce, and they should not be read as a machine-checked property the way the other three phases' are. pytest is no second half here either: the only thing it can catch in a test move is a test that stops passing, and a weakened one still passes — turning assert parent == 7 into assert parent == parent inside a moved class leaves this script at exit 0 and the suite at its usual count.

The body says the same, and names your demonstration concretely (exit 0 + 75 passed, both gates green on a gutted assertion). The docstring is the copy that outlives this PR body, which is why the plain statement went there rather than only here.

Thanks for reproducing the byte-identical claim independently — 17/17 on ast.dump, ast.get_source_segment and a decorator-inclusive slice, plus the 1350/1350 node-ID match with test_geo2mort_vs_toolstest_geo2mort_vs_convert as the only difference. Since review is now the only thing standing behind phase 3, that reproduction is the substance of the gate, and it is recorded in the body under the phase 3–4 fold.

cd /tmp
pytest -v mortie_tests/test_tools.py mortie_tests/test_polygon_regression.py
pytest -v mortie_tests/test_convert.py mortie_tests/test_orders.py \
mortie_tests/test_polygon_regression.py

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

[low] The substitution is correct — but this line is not exercised by CI on main either. It runs only on a tag/release run, and the PR body does not say so.

The edit itself verifies two ways:

  • yaml.safe_load parses the job, and the run block comes back as one command with the backslash continuation intact.
  • Emulating it exactly (cp -r mortie/tests /tmp/mortie_tests; cp pyproject.toml /tmp; cd /tmp; pytest -v …) gives 85 passed for test_convert.py test_orders.py test_polygon_regression.py and 85 passed for the pre-split test_tools.py test_polygon_regression.py. Same count, so the 1 → 2 substitution moved the whole smoke set and nothing depends on a conftest or fixture that stayed behind. grep -rn "test_tools\|tools\.py\|mortie\.tools" .github/ is now empty, so this was the only reference.

What is worth recording is where the risk actually sits. test-wheels is skipped on push to main as well as on pull requests — merging will not exercise it:

  • run 31261344791 (push, main): a single job, name unexpanded, Test wheels on ${{ matrix.os }} - Python ${{ matrix.python-version }}skipped
  • run 31226132063 (push, tag 0.9.4): 12 expanded Test wheels on … - Python … jobs, all success

and publish-testpypi, publish-pypi and Create GitHub Release all carry needs: […, test-wheels]. So the first real execution of this line is a release run, where a mistake in it blocks the PyPI publish rather than turning a branch red. The phase-3 section documents the edit and the approval for it, but not that CI never runs it here or on main — worth stating alongside the local verification, since it is the only CI-touching hunk in four phases.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in the PR body — no tree change, since you confirmed the edit itself is correct. You're right that the body documented the edit and the approval for it but never said when it runs, which is the part a reviewer actually needs.

The phase-3 section now carries this, verbatim from your evidence:

Where the risk actually sits: CI never runs this line — not on the PR, and not on main either. The test-wheels job is skipped on both, so merging will not exercise it; its first real execution is a tag/release run, where a mistake blocks the PyPI publish rather than turning a branch red.

  • run 31261344791 (push to main): one job, matrix name unexpanded → skipped
  • run 31226132063 (push, tag 0.9.4): 12 expanded Test wheels on … - Python … jobs, all success
  • publish-testpypi, publish-pypi and Create GitHub Release all carry needs: […, test-wheels]

and follows it with local emulation named as the verification of record, not CI — the job's steps run exactly (cp -r mortie/tests /tmp/mortie_tests; cp pyproject.toml /tmp; cd /tmp; pytest -v …) giving 85 passed for test_convert.py test_orders.py test_polygon_regression.py and 85 passed for the pre-split test_tools.py test_polygon_regression.py. Same count, so the 1 → 2 substitution moved the whole smoke set and nothing in it depended on a conftest or fixture left behind.

That framing matters beyond bookkeeping: it means the yaml.safe_load parse and the emulation are not belt-and-braces on top of CI, they are the only checks this hunk gets before a release run — worth stating plainly for the one CI-touching hunk in four phases.

Comment thread mortie/dissolve.py
Comment on lines +522 to +530
"""Mirror of the Rust hemisphere guard (``src_rust/src/dissolve.rs``, issue
#108): exterior/hole classification keys off the sign of the mod-4π
spherical signed area, which is ambiguous once the cover nears 2π — fail
loud on the exact covered area (Σ π/(3·4^depth), cells are equal-area)
instead of silently swapping shells and holes. Assumes disjoint,
non-duplicated cells (the dissolve precondition anyway — duplicate words
would break edge cancellation); duplicates double-count. Returns the
exact covered area (steradians) for the wrap cross-check downstream.
"""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [ruff] <D205> reported by reviewdog 🐶
1 blank line required between summary line and description

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Pre-existing on origin/main, moved verbatim — left, per CLAUDE.md §4. Same answer as the earlier ruff threads on this PR; it has only re-surfaced because the phase 3–4 fold touched dissolve.py's module docstring, which shifted _reject_hemisphere_cover a line down.

This is origin/main's mortie/geometry.py:1356 D205 travelling with its function into dissolve.py. The summary genuinely runs on into the description:

def _reject_hemisphere_cover(morton):
    """Mirror of the Rust hemisphere guard (``src_rust/src/dissolve.rs``, issue
    #108): exterior/hole classification keys off the sign of the mod-4π
    spherical signed area, ...

Byte-identical to the pre-move original — that identity is exactly what benchmarks/verify_pure_move.py asserts on every run (44/44 definitions verbatim, exit 0), so fixing it here would break the pure-move claim this PR is built on: reflowing the docstring makes _reject_hemisphere_cover differ from origin/main:mortie/geometry.py and the verifier reports it. §4's "do not fix pre-existing failures unrelated to your change" and this PR's pure-move contract point the same way.

ruff check mortie is 13 findings, one fewer than origin/main's 14 — the set otherwise identical, with tools.py:975 F841convert.py:670 F841 and this D205 the two that moved. Worth a small-fix issue alongside the F841 in question (4) once the split lands; it is a two-line docstring reflow with no behaviour, just not this PR's to make.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Split tools.py and geometry.py into domain modules mirroring the Rust tree

1 participant