Split tools.py into convert/orders/buffer (issue #159) - #169
Conversation
| import pytest | ||
| import numpy as np | ||
| from mortie import tools | ||
| from mortie import convert, orders | ||
|
|
||
|
|
||
| class TestMort2Geo: |
There was a problem hiding this comment.
There was a problem hiding this comment.
🤖 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.
There was a problem hiding this comment.
🤖 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.
There was a problem hiding this comment.
Import block is un-sorted or un-formatted
mortie/mortie/tests/test_polygon_regression.py
Lines 12 to 17 in 32548b0
There was a problem hiding this comment.
🤖 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.
There was a problem hiding this comment.
Import block is un-sorted or un-formatted
mortie/mortie/tests/test_tools.py
Lines 15 to 24 in 32548b0
There was a problem hiding this comment.
🤖 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 Report❌ Patch coverage is
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
🤖 from Claude Status: phase 1 landed ( |
Merging this PR will degrade performance by 12.91%
|
| 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
Footnotes
-
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. ↩
-
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. ↩
| morton_buffer_meters, | ||
| ) | ||
|
|
||
| # Address-space conversions (split out of tools by domain, issue #159) |
There was a problem hiding this comment.
🤖 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:
-
englacial/zagg#406is an issue, not a PR —gh issue view 406 --repo englacial/zaggreturnsOPEN, "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. -
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.
- 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, andgh pr diff 398removes nomortie.toolsimport. 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.
There was a problem hiding this comment.
🤖 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.
|
|
||
| **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: |
There was a problem hiding this comment.
🤖 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.geo2mort → mortie.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.)
There was a problem hiding this comment.
🤖 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: |
There was a problem hiding this comment.
🤖 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.
There was a problem hiding this comment.
🤖 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.AnnAssignwith aNametarget is now indexed and compared like a plainAssign, so a movedMAX_ORDER: int = 29is covered rather than invisible.- Every other top-level statement — tuple/attribute/subscript assignment targets,
if/tryblocks, loops,AugAssign— is collected asunhandledand raised bycheck_movesas 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.
| help="git revision holding the pre-split source") | ||
| args = parser.parse_args() | ||
|
|
||
| failures = check_moves(args.base) + check_public_surface(args.base) |
There was a problem hiding this comment.
🤖 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.
There was a problem hiding this comment.
🤖 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.
|
🤖 from Claude Status: phase 2 landed ( One thing worth a look before the fold: verifying the geometry split against |
| # 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", |
There was a problem hiding this comment.
🤖 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.pyhasSPLITS = {"mortie/tools.py": [...]}and no geometry entry, so phase 1 edited threegeometry.pybodies 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.
There was a problem hiding this comment.
🤖 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 only —
getattr(node, "body")direct children. An import nested inside anifor atryis 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-P1comment. - 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) |
There was a problem hiding this comment.
🤖 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:
-
The script does not carry the squash-merge caveat. The PR body says "which is noted in the script", but the
SPLIT_BASEScomment 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. -
--baseis silently partial onceSPLIT_BASESis non-empty.python benchmarks/verify_pure_move.py --base <anything>still verifiesgeometry.pyagainst011816c; only thetools.pyarm and the__all__arm move. The docstring covers it in prose ("pins its own base inSPLIT_BASESrather than using--base"), and the printedmortie/geometry.py@011816clabel — a good addition — makes it visible in the output, so this is a note rather than a defect. But--helpstill says "git revision holding the pre-split source" with no qualifier.
There was a problem hiding this comment.
🤖 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_default → NameError: 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.
| # 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 |
There was a problem hiding this comment.
🤖 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.
There was a problem hiding this comment.
🤖 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:
- 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,_BACKENDand the five_TYPE_*, + 21 functions) so the checksum is self-checking rather than something a reader has to re-derive. - The
dissolve.pyrow of the table now ends "…,_dissolved_polygons, and the_DISSOLVE_SNAPconstant" — the table listed 16 names against a 17-definition module, which is what made the two halves disagree in the first place. - 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).
|
🤖 from Claude Status: phases 3 and 4 landed (
Two things worth a look before the fold. The test split is not in |
| 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 |
There was a problem hiding this comment.
🤖 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.
- This line (
mortie/dissolve.py:15-16): "mortie.geometrykeeps the coverage<->geometry API, the codec quartet and the WKB plumbing". As of83de969the quartet ismortie.codec's. mortie/geometry.py:1-8:"""Lazy WKB/WKT geometry codec for mortie… "The runtime stays numpy-only: this module imports a geometry backend (shapely>=2preferred,spherelyaccepted) lazily".geometry.pyno longer imports a backend anywhere —codec._require_backenddoes, which is the stated point of the new module. This one is also published:docs/api/geometry.mdrenders it through::: mortie.geometry.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.
There was a problem hiding this comment.
🤖 from Claude
Fixed in c3122c4 — all three retargeted at mortie.codec, no behaviour.
mortie/dissolve.py:15-16— now reads "mortie.geometrykeeps the coverage<->geometry API and the WKB plumbing (the codec quartet and the backend gate aremortie.codec's, since issue Split tools.py and geometry.py into domain modules mirroring the Rust tree #159 phase 4)".mortie/geometry.py:1-5— the one that matters, sincedocs/api/geometry.mdpublishes it through::: mortie.geometry. Both halves were stale, so both changed: the titleLazy WKB/WKT geometry codec for mortie→WKB/WKT geometry ingest and emit for mortie, and "this module imports a geometry backend … lazily and uses it only as a codec" → "mortie.codecimports a geometry backend … lazily, and this module uses it only as a codec". The rest of the paragraph — numpy-only runtime, no spatial predicates,ImportErroron first touch — is still true as written and untouched.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 |
There was a problem hiding this comment.
🤖 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.
There was a problem hiding this comment.
🤖 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:
- 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._BACKEND→codec._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. - It is the same line already drawn twice on this PR, and drawing it consistently is the point.
_normalize_antimeridian_polygon'sF841moved verbatim, dead local and all (question (4)); the four ruff-bot findings were left as pre-existing. What did get fixed —lib.rs:230inc3c8ac7, and the three docstrings inc3122c4above — 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. - The one-liner is not quite a one-liner here.
codec._require_backend()beforesaved_backend = codec._BACKENDraisesImportErrorwhen 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 suppressedImportErroraround it, i.e. a small judgement about backend-free environments rather than a mechanical edit.assert saved_backend is not Nonehas 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.
There was a problem hiding this comment.
🤖 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.
| 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. |
There was a problem hiding this comment.
🤖 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 == 7 → assert 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.pyagainst both destinations: 17 top-level statements in, 17 out, each equal onast.dump,ast.get_source_segmentand a decorator-inclusive source slice; nothing lost, nothing added, nothing in both destinations exceptif __name__ == "__main__". Collected node IDs match too — 1350 at440a8fa, 1350 at83de969, and after normalising the file component the only difference in the entire set istest_geo2mort_vs_tools→test_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.pyis "1,042 lines, 16 top-level statements" and then "17 top-level statements in, 17 out" three paragraphs later. 17 is right:ast.parsegives 26 top-level statements = 1 docstring + 8 imports + 14 classes + 2 helpers + 1if. (wc -lis 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.
There was a problem hiding this comment.
🤖 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_movesindexes a pytest class fine — it is a top-levelClassDef— but the trailingif __name__ == "__main__"block is not comparable, so it reports 3 failures (the source and both destinations) beforecheck_pinned_basesadds 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.
pytestis 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 — turningassert parent == 7intoassert parent == parentinside 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_tools → test_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 |
There was a problem hiding this comment.
🤖 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_loadparses the job, and therunblock 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 fortest_convert.py test_orders.py test_polygon_regression.pyand 85 passed for the pre-splittest_tools.py test_polygon_regression.py. Same count, so the 1 → 2 substitution moved the whole smoke set and nothing depends on aconftestor 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, tag0.9.4): 12 expandedTest wheels on … - Python …jobs, allsuccess
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.
There was a problem hiding this comment.
🤖 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
maineither. Thetest-wheelsjob isskippedon 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 tomain): one job, matrix name unexpanded →skipped- run
31226132063(push, tag0.9.4): 12 expandedTest wheels on … - Python …jobs, allsuccesspublish-testpypi,publish-pypiandCreate GitHub Releaseall carryneeds: […, 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.
| """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. | ||
| """ |
There was a problem hiding this comment.
🤖 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 F841 → convert.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.
Closes #159.
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
mainate9698f32has zeromortie.tools, zagg#406 closed. Nothing onthis PR is waiting on anyone.
Phases
mortie/tools.py(1,493 lines) intoconvert.py/orders.py/buffer.py, deletetools.py, ship the pure-move verifier.mortie/dissolve.py(the spherical outline machinery) out ofmortie/geometry.py(1,665 lines at phase 1's head).test_tools.pyto match where its subjects now live, and update the wheel-smoke workflow that named it. (Question (2), approved by @espg.)mortie/codec.py(the backend gate + the codec quartet) out ofgeometry.py. (Question (5), ruled (b) by @espg.)Phase 1: recomputed membership and line counts
tools.pygrew past the figures in the proposal (children_oflanded 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.
convert.pyunique2parent,norm2mort,geo2uniq,geo2mort,mort2norm,norm2uniq,uniq2geo,mort2geo,mort2bbox,mort2polygon,mort2healpix_encoder_orders,_normalize_antimeridian_polygon_rust_geo2mortorders.pyorder2res,res2display,orders_of_uniq,orders_of,is_point,infer_order_from_morton,validate_morton,clip2order,generate_morton_children,children_ofResolutionLevel,EARTH_RADIUS_KM,MAX_ORDER,_rust_mort2nested,_rust_nested2mortbuffer.pymorton_buffer,morton_buffer_meters_EARTH_RADIUS_M1,554 lines total against
tools.py's 1,493 — the +61 is three module docstrings andthree 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_of→orders.py. It did not exist when the proposal was written; theproposal's own validation test places it "beside
generate_morton_children", which isorders.py. It is 133 lines, and it is whyorders.pyis 573 rather than theprojected 395.
convert.pyis 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
_encoder_ordersconvert.pygeo2uniqandnorm2uniq, both inconvert.py_normalize_antimeridian_polygonconvert.pymort2polygon_EARTH_RADIUS_Mbuffer.pymorton_buffer_meters_rust_geo2mortconvert.pygeo2mort_rust_mort2nested,_rust_nested2mortorders.pyNothing 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_childreninorders.pyandmort2norminconvert.py;_rust_nested2mort:generate_morton_childrenandnorm2mort). Theylive once, in
orders.py, andconvert.pyimports them.orders.pyrather thanconvert.pybecause the dependency DAG only admits onedirection.
convert.pyalready needsorders_of/orders_of_uniq/MAX_ORDERatmodule scope (
mort2geo,mort2bbox,mort2polygon,unique2parent,uniq2geo,geo2uniq), soorders.pyhas to be the root:orders → convertandorders → buffer, no cycles. Putting the aliases inconvert.pywould needfrom .convert import _rust_mort2nestedinorders.pyand a circular import atpackage-init time. It is worth naming that this is the one place where the
dependency graph, not the domain, picked the home — a
morton ↔ NESTEDbridge readslike a conversion.
Diff hunks that are not moves
Every one, exhaustively:
convert.py,orders.py,buffer.py) and threeimport blocks. New text by necessity —
tools.pyhad one docstring(
"""Functions for morton indexing.""") and one import block.mortie/__init__.py— the singlefrom .tools import (...)block becomes three,in isort position, with the
# Inverse functionsinline comment travelling withinfer_order_from_morton. Two trailing comments that namedtools.pynow nameconvert.py.__all__is untouched.rank_xy.py(MAX_ORDER),moc.py(norm2mort),prefix_trie.py(geo2mort), and three function-localsites in
geometry.py.geometry.py:848needed bothmort2polygonand_rust_mort2nested, which now live in different modules, so that one line becomestwo.
moc.py's import block was re-sorted to keep isort happy (.convertsortsbefore
.coverage).docs/api/tools.md→convert.md+orders.md+buffer.md, with themembers: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.docs/specification.md— two references tomortie.tools.order2res/mortie.tools.EARTH_RADIUS_KMnow saymortie.orders.*.src_rust/src/lib.rs:230is the same edit and is the one place the Rust tree is touched — see
"One Rust doc-comment, deliberately" below.
tools.X→convert.X/orders.Xacross 11 test modules and
benchmarks/generate_morton_reference.py. No test bodylogic changed. Three consequences worth flagging:
test_tools.pyimportsorders as orders_mod. Six test bodies bind a localvariable named
orders(e.g.orders = rng.integers(0, MAX_ORDER + 1, size=30)),which shadows the module and made
flake8 --select=F82fail with F823. Aliasingthe module was the option that changed no test-body identifier; the other files
use the plain
orders/convertnames.tools.→convert.is two characterswider, so three visual-indent continuations became E128 and three lines crossed
88 columns. Re-wrapped; net
flake8 --max-line-length=88count across the suiteis unchanged from
origin/main(see gates).test_rank_xy.py,test_main_api.py) for the same reason as(3).
test_tools.py's module docstring first line now names the two modules ittests rather than the deleted
mortie.tools.benchmarks/verify_pure_move.py— new, see below.Phase 2: recomputed membership and line counts
geometry.pywas 1,665 lines at phase 1's head (011816c), not the 1,664 the issuequotes — 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_polygonsat:827is the nearest neighbour and deliberately stays — itis the
dissolve=Falseemit path, one backendPolygonper cell, with no sphericalreasoning in it at all.
geometry.pydecompose,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_polygonsdissolve.py_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_SNAPconstant44 top-level definitions in, 44 out — 27 stay, 17 move. (An earlier revision of
this body said "28 stay, 16 move", counting
_DISSOLVE_SNAPas a stayer in the headlineand 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-levelassignments —
_BACKENDand the five_TYPE_*— plus 21 functions. Only the count waswrong; the move itself verifies on all four axes below.) Both modules are under the
~1,000-line aim — but see "Questions for review":
geometry.pyat 971 has 29 lines ofheadroom, which is worth a decision now rather than at the next PR that touches it.
dissolve.pyhas no public members, somortie/__init__.pyis untouched by phase 2and there is no new
docs/api/page (an mkdocstrings page with an emptymembers:listrenders nothing).
docs/api/geometry.md'smembers:list is therefore already the exactpartition — 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 callgraph. Measured rather than assumed:
geometry.py—no
_require_backend, no_require_shapely, no_ring_latlon._nest_and_buildand_dissolved_polygonstake the backend module as a parameter (mod), which is why.to_geometrycalls_dissolved_polygons.So
dissolve.pyimports nothing frommortie.geometry, andgeometry.pygains onemodule-level line —
from .dissolve import _dissolved_polygons. That is the opposite ofphase 1, where the two
_rust_*aliases had consumers on both sides and the DAGdictated 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 neitherapplies —
dissolve.py's own module-level imports aremathandnumpyonly (its.moc/.orders/._healpix/._rustieimports stay function-local, moved as-is),so importing it costs nothing and closes no loop. This matches
moc.py'sfrom .convert import norm2mort.Where each private helper went, and why
All 16 are private, and they are one connected component:
_dissolved_polygons(theruntime 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 antimeridiancut/stitch chain
_cut_at_antimeridian→_stitch_segments→_next_segment. Nothingin that component has a consumer outside it except
_dissolved_polygonsitself, so thecut 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), whoseonly consumer is
_boundary_rings_xyz.import mathmoved entirely — its only twouses in the whole file were
_tangent_azimuth:1027and_chain_rings:1083, sogeometry.pyno longer imports it._BACKENDand the five_TYPE_*ids stay: theirconsumers (
_require_backend,decompose) stay.Diff hunks that are not moves — phase 2
Exhaustively, and there are five:
dissolve.py's module docstring — new text by necessity.geometry.pylosesimport math(and its blank separator) and gainsfrom .dissolve import _dissolved_polygons. Its own module docstring is untouched.mortie/tests/test_geometry.py—from mortie import geometrybecomesfrom mortie import dissolve, geometry, and five references retarget:geometry._dissolved_rings_py×3,geometry._nest_and_build×1, and one commentnaming
geometry._dissolved_polygons. No test body logic changed. No shadowing thistime —
dissolveappears in these tests only as a keyword argument(
to_geometry(..., dissolve=False)), never as a binding, so no alias was needed.docs/api/geometry.mdgains one paragraph saying where the outline machinerywent and why it has no page of its own.
members:unchanged.benchmarks/verify_pure_move.pygains theSPLITSentry, plusSPLIT_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 explainingedge-cancellation) moved verbatim into
dissolve.pyrather than being folded intothe 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.pymeans editing.github/workflows/build-wheels.yml, which CLAUDE.md §1 forbids unless the issue namesit. @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) becomestest_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 -lis 1041, and 17 is the count the "17 in, 17 out" claimbelow uses: 16 named definitions — 14 classes plus the two helpers — plus the
trailing
if __name__ == "__main__"block, which is a top-level statement the verifiercannot name.
ast.parsegives 26 in total, the other 9 being the docstring and 8imports.)
test_buffer.py, becausemortie/tests/test_buffer.pyalready exists. Phase 1moved
morton_buffer/morton_buffer_metersintobuffer.py, but their tests werenever in
test_tools.py— they were already in their own file. Creating a third filewould have collided with it.
calls both modules —
TestClip2Orderbuilds fixtures withconvert.norm2mortandasserts with
convert.mort2normwhile testingorders.clip2order— so bothdestination files import both modules. What each class is about is unambiguous
though, so:
test_orders.pytakesTestOrder2Res,TestRes2Display,TestUniqOrders,TestClip2Order,TestGenerateMortonChildren(plus the_sphere_resand_uniq_athelpers, each used by exactly one of them);
test_convert.pytakes the other nineclasses.
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).pytestreports thesame 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_modsurvives into both new fileseven though only
test_convert.pyhas the local-variable shadow that forced it inphase 1. Renaming the alias in
test_orders.pywould have edited six class bodies tobuy cosmetics.
Workflow references — every one found, and what changed
grep -rn "test_tools\|tools\.py\|mortie\.tools" .github/returns exactly onematch, at
build-wheels.yml:429(line 426'scp -r mortie/tests /tmp/mortie_testsisthe directory copy, unaffected). It ran the wheel smoke test as:
pytest -v mortie_tests/test_tools.py mortie_tests/test_polygon_regression.pyand now runs:
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 smokeset, so adding it would be a scope change hiding inside a rename. The edited YAML was
re-parsed with
yaml.safe_loadto confirm the line continuation is valid.Where the risk actually sits: CI never runs this line — not on the PR, and not on
maineither. Thetest-wheelsjob isskippedon both, so merging will not exerciseit; its first real execution is a tag/release run, where a mistake blocks the PyPI
publish rather than turning a branch red.
31261344791(push tomain): one job, matrix name unexpanded —Test wheels on ${{ matrix.os }} - Python ${{ matrix.python-version }}→skipped31226132063(push, tag0.9.4): 12 expandedTest wheels on … - Python …jobs, allsuccesspublish-testpypi,publish-pypiandCreate GitHub Releaseall carryneeds: […, 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 fortest_convert.py test_orders.py test_polygon_regression.pyand 85 passed for the pre-splittest_tools.py test_polygon_regression.py— the same count, so the substitution moved the whole smokeset and nothing in it depended on a
conftestor fixture left behind.Diff hunks that are not moves — phase 3
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.
F401).Both are isort-clean, which is why
ruff check mortiedrops from 14 findings to13:
origin/maincarried anI001ontest_tools.py:14, and that file is gone.This is the first phase that changes the ruff count at all.
.github/workflows/build-wheels.yml:429— above. Not a pure move, and theonly hunk in four phases that touches CI.
test_main_api.py::test_geo2mort_vs_tools→test_geo2mort_vs_convert. Oneline; the docstring under it already said
mortie.convert.geo2mortfrom phase 1.benchmarks/verify_pure_move.pygains a "known limitations" bullet — see below.The verifier does not cover the test split, and why not
check_movesindexes a pytest class fine — it is a top-levelClassDef, so aclass-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 isnot only
check_pinned_basesthat cannot take the split. An earlier revision of thissection said
check_moves"would handle it" and attributed all 18 failures to the fourtharm. The split is actually:
check_moves: 3 failures. The trailingif __name__ == "__main__"block is atop-level statement
top_level_defscannot 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--basein body-level import statements alone, andtest_tools.pydoes not satisfyit: 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
pytestis no second half here, unlike everywhere else in this PR: the only thing itcan catch in a test move is a test that stops passing, and a weakened test still
passes. Demonstrated — turning
assert parent == 7intoassert parent == parentinsidea moved class in
test_convert.pyleavesverify_pure_move.pyprinting "Pure moveverified" at exit 0 and
pytest mortie/tests/test_convert.py mortie/tests/test_orders.pyat 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 thatoutlives 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):geometry.pydecompose,_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_*idscodec.py_BACKEND,_require_backend,_require_shapely,_strip_ewkt_srid, and the quartet_geometry_from_wkb/_geometry_from_wkt/_geometry_to_wkb/_geometry_to_wktgeometry.pygoes 971 → 772, which is the headroom question (5) asked about: 228lines 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.pyhas no public members — confirmed by scanning it, not assumed; every namebegins with
_, the quartet having been privatized under @espg's ruling on #157. So itgets no
docs/api/page, exactly likedissolve.py, anddocs/api/geometry.md'smembers:list is untouched and still partitions exactly (all seven names stayed).mortie/__init__.pyis untouched by this phase too.DAG direction, and what forced it
geometry → codec, one direction, and nothing forced it — the call graph made theother direction empty. Measured:
only each other:
_require_shapely→_require_backend→_BACKEND,_geometry_from_wkt→_require_backend+_strip_ewkt_srid,_geometry_to_wkb→_require_backend+_require_shapely.decomposeandto_geometry(both_require_shapely),from_wkt(_geometry_from_wkt),to_wkb(_geometry_to_wkb),to_wkt(_geometry_to_wkt). Sogeometry.pygains a four-name import.dissolve.pyneeds none of it, confirmed by scan — it takes the backend module asa parameter (
mod), which is the same property that made phase 2 one-way.Module-level import graph after four phases:
geometry → {codec, dissolve}, andcodec → {},dissolve → {}. No cycles, andcodec.pyneeds no module-level importsat all — the backend is imported lazily inside
_require_backend, which is the wholepoint of the gate.
Where each constant landed
_BACKENDmoves: its only readers/writers are_require_backendand the tests, all ofwhich move or rewire with it. This matters more than it looks —
_BACKENDis mutatedmodule-globally by
test_wkb_no_backend.py, so a test left pointing atgeometry._BACKENDwould silently stop gating anything rather than fail.All five
_TYPE_*ids stay. Rechecked now that the gate is moving, not carried overfrom phase 2's finding: their only consumer anywhere is
decompose, which stays. Noconstant has consumers on both sides, so nothing needed the DAG reasoning the
_rust_*aliases got in phase 1.
_strip_ewkt_sridmoves with the quartet: its only consumer is_geometry_from_wkt.Diff hunks that are not moves — phase 4
codec.py's module docstring — new text.geometry.pygainsfrom .codec import (...)(four names). It loses no import —numpyis still used by the stayers.mortie/tests/test_geometry.py— import gainscodec, 15 references retargetfrom
geometry._Xtocodec._X, and one function-localimport mortie.geometry as gmbecomesimport mortie.codec as gm. No test body logic changed.mortie/tests/test_wkb_no_backend.py—from mortie import geometrybecomesfrom mortie import codec, and five references retarget. (This entry originally read"it referenced nothing else on that module" — wrong:
no_geometry_backend'sdocstring named
mortie.geometrytwo lines above the code it describes, and the phase-4hunk retargeted the five code references while leaving the prose. Fixed in the phase 3–4
fold,
c3122c4.)docs/api/geometry.md— the sentence phase 2 added aboutdissolvenow namescodectoo.members:unchanged.benchmarks/verify_pure_move.py—codec.pyadded to thegeometry.pysplit'sdestination list. No new
SPLIT_BASESentry: phase 3 did not touchgeometry.py, so phase 1's head (011816c) is still the correct pre-move base foreverything 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.pyis 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/mainby default):in the source at the base compares equal as an AST (
ast.dump, so formatting andline numbers are ignored) and as literal source text (so comments inside a
definition are covered too).
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, atry/except ImportErrorshim, a loop, an
__all__ +=— is reported as a failure rather than skipped, sothis arm fails loud instead of open (
011816c's parent889cf97; annotatedassignments are now compared like plain ones).
set(mortie.__all__)equals the base's and every name init resolves as an attribute of
mortie. The base package is extracted withgit archive, the built_rustieis 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) + [...]). Thesubprocess asserts its own
mortie.__file__is inside the extracted tree, and theparent asserts its own is inside the repo — an editable install shared across
worktrees otherwise makes this compare a tree against itself.
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
ResolutionLevelbanner, theEARTH_RADIUS_KMprovenance note, the kernel-bridge note,the
MAX_ORDERnote,# 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
d470bc4because review found it: only the moves are checked, not the"plus import rewiring" half of the claim. Deleting
geometry.py'sfrom .dissolve import _dissolved_polygonsleaves all 76 definitions verbatim and all 69__all__names resolvable, so the script still exits 0.pytestis what catches it —test_emit_dissolve_is_the_defaultraisesNameError: name '_dissolved_polygons' is not defined. The combined gate holds, but a green verifier run is half of it, not the wholeone, and the docstring now says so.
Phase 2 needed one more thing than the predicted one-line
SPLITSentry. Verifyingthe geometry split against
origin/mainreported three real differences —_per_cell_polygons,_boundary_rings_xyzand_reject_hemisphere_cover— becausephase 1 had already rewired their function-local imports (
from .tools import→from .convert/from .orders). Allow-listing them would have blinded the arm to anygenuine change in three of the largest moved functions. Instead each split now pins the
commit it was actually cut from:
So the tools arm still verifies against
origin/mainand the geometry arm verifiesagainst phase 1's head — both at full strength, 76 definitions compared with nothing
allow-listed.
EXPECTED_NEWis 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
forloop (the889cf97fail-loud arm);_DISSOLVE_SNAPchanged;to_geometrydropped 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.pyby phase 1 sits in both the pinned base anddissolve.py,so
check_movescompares it against itself. With a mutation planted inside_boundary_rings_xyzin a synthetic phase-1 head and mirrored intodissolve.py, phase1's own verifier reported
32/32"Pure move verified" exit 0 (itsSPLITShas nogeometry.pyarm at all) and phase 2's reported44/44exit 0. Both green, mutationshipped — so
44/44 verbatimread as "verbatim since phase 1", not "sinceorigin/main".A fourth arm closes it. Every pinned source is now indexed at
--baseas well, and eachdefinition must be equal after body-level
Import/ImportFromstatements aredropped — 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:
Rebuilding review's synthetic seam against the new arm now exits 1:
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.pyat a syntheticpin and mirrored into its destination so
check_movesstays green. Caught: a commentadded inside a moved body; a logic edit (AST); a docstring reworded; a blank line added
inside a body; trailing whitespace;
_DISSOLVE_SNAPchanged; a stayer body edit(
_per_cell_polygons) and a stayer constant change (_TYPE_MULTIPOLYGON) — the 27stayers 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
ifrather 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_BASESno 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, thesame fail-with-a-traceback class the fold closed in
011816c. It is now a failure entrynaming the fix, in all three places a revision is read (
check_moves,check_pinned_bases, andcheck_public_surface'sgit archive):Two smaller things on the same finding:
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 aboveSPLIT_BASES, whichis the artefact that outlives this PR body.
--helpnow says--baseis partial. It was silently so onceSPLIT_BASESisnon-empty:
--base <anything>still verifiesgeometry.py's move against011816c.The help text now spells out that a pinned split compares its move against its pin, and
only the pin itself against
--base.tools.pyis deleted, not shimmedPer the issue.
mortie.toolsdisappears as an attribute, not just as an import target:comparing a real import of
origin/mainagainst this branch,dir(mortie)losestoolsand gainsbuffer/convert/orders. Every survivingfrom mortie.tools import ...raisesModuleNotFoundErroron the next mortie release.Merge-order gate — restated, because the version above this line was wrong.
Re-derived against
englacial/zagg@mainrather than taken from issue #159:importing
mort2polygon". Five sites importmort2polygon;notebooks/aoi_mask.ipynb:69imports
mort2geo. That distinction matters here because Split tools.py and geometry.py into domain modules mirroring the Rust tree #159 moves the two namesindependently (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 onenglacial/zagg@mainand imports nothing.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 thefour-site fix had already forked at
196867b. So for a day there were two sites noin-flight fix could have covered:
data/holdsstandalone 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 surfacedthe 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 arenow wrong, and the resolution went a different route:
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.
(merged 2026-08-08T17:47:11Z), cherry-picking
ae408eawith authorship preserved —applied clean — plus a second commit for the two
data/builder sites. All six sitesin one PR, not four-plus-two across two.
englacial/zagg@main=e9698f32:git grep "mortie\.tools"returns nothing. englacial/zagg#406
is CLOSED (2026-08-08T17:47:13Z).
ae408eabecomes 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:230describedrust_geo2mort's bindings-vs-wrapper split by namingthe
mortie.tools.geo2mortwrapper. PyO3 compiles///into__doc__, so that stalemodule path is runtime-visible, not just a source comment:
It now says
mortie.convert.geo2mort— the same one-word retarget as the two ondocs/specification.md, and the lastmortie.toolsreference anywhere outside thestrings that deliberately name the old module (the new modules' "Split out of
mortie.tools" docstrings, the docs pages,verify_pure_move.py'sSPLITSkey,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__pointingat 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 --libare unchanged (re-run below), and the extension was rebuilt with
maturin develop --releasebefore any gate here was quoted.
(
CLAUDE.md:84still says(tools.py), but as narrative about Python fallbacks removedhistorically — correct as written, left alone.)
(Through phases 1–2
mortie/tests/test_tools.pykept its filename even thoughmortie/tools.pywas gone, because.github/workflows/build-wheels.yml:429named it andCLAUDE.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), @espgapproved 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 --releasebuild, withmortie.__file__asserted into that worktreebefore any result was trusted.
python benchmarks/verify_pure_move.py__all__69 names equal toorigin/main's; 9/9 phase-2 and 7/7 phase-4 move-arm mutations caught, 16/16 pin-arm mutations as expectedpytest(full suite)flake8 mortie --select=E9,F63,F7,F82flake8 mortie --max-line-length=88origin/maincount matches file for file)ruff check mortieorigin/main's 14. The set is otherwise identical:tools.py:975 F841is nowconvert.py:670 F841andgeometry.py:1356 D205is nowdissolve.py:521 D205, both travelling with their moved function; the one that disappears isorigin/main's pre-existingI001ontest_tools.py:14, whose file phase 3 replaced with two isort-clean onesruff check benchmarks/verify_pure_move.pynumpydoc lintonconvert.py,orders.py,buffer.py,dissolve.py,codec.py,geometry.py,__init__.py,verify_pure_move.pycargo fmt --check/cargo clippy --lib/cargo test --libsort_by_key) / 317 passedAll of the above re-run on phase 4's head (
83de969) against a freshmaturin develop --release, withmortie.__file__asserted into the worktree first, andagain on the phase 3–4 fold head (
634f114, and20f5274after the question (6)comment):
pytest1334 passed, 16 skipped;flake8 mortie --select=E9,F63,F7,F82clean;ruff check mortie13, unchanged;ruff check benchmarks/verify_pure_move.pyclean;numpydoc lintclean on the sixmodules plus the script;
verify_pure_move.py32/32 + 44/44 + 44 equal modulo imports(3 import-rewired) + 69 names, exit 0, "Pure move verified". The fold touches
docstrings and one
printbranch only, so no Rust and no gate moved.One
cargo test --librun failed 1 of 317 while the Python suite was still finishing andpassed cleanly (317/317) on rerun; #155's known
sub_hemisphere_cover_still_dissolvesflake 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 Analysischeck is red, and it is not this PR.Every GitHub Actions job is green on
011816c, including theCodSpeed Benchmarksworkflow 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 bythe 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, whichcoverage_bench.rsdoesnot 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 benchmarksand 1 skipped. Two things put it beyond doubt:
Rust benchmarksGitHub Actions job issuccesson20f5274— as aretest (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.found on
main(20f5274) during the generation of this report, so2f28a90was usedinstead as the comparison base. There might be some changes unrelated to this pull
request in this report."
2f28a90ismortie.arrow.from_wkbs(issue mortie.arrow.from_wkbs: arrow skin over the WKB batch, so a geoparquet column skips Python bytes objects #163) — anunrelated 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
printbranch and one comment block. No Rust, nocoverage_bench.rs, no code patha 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_basinsfailedonce on the pre-change baseline run of
origin/mainand 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.
data/sites outstanding) was accurate when written; englacial/zagg#411 has since closed all six and the gate is CLEAR. Section rewritten abovemortie.tools.geo2mortinlib.rs:230, live as__doc__c3c8ac7top_level_defsfails open on non-deftop-level statementsAnnAssigncompared, everything else reported as a failure889cf97ImportErrortraceback011816cVerified after the fold:
python benchmarks/verify_pure_move.pystill exits 0 at 32/32and 69 names. Both new arms were mutation-checked — a top-level
forappended tobuffer.pygivesmortie/buffer.py: For at line 123 is not comparable, and deletingpublic
morton_buffernow reportsinstead of a traceback. Both mutations were reverted (
git diff --statclean).Independent checks that came back stronger than this PR claimed — recorded so they
are not re-derived:
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.
went past the script's own coverage: decorator lines (which
ast.get_source_segmentexcludes) and an explicitast.get_docstringcompare.origin/mainvs this branch across all 69 names: identical name set,type,inspect.signature, and rendered docstring. Only__module__moves.import mortie.tools→ModuleNotFoundError; no package file still imports it.(aliases in
convert.py) dies withImportError: cannot import name 'MAX_ORDER' from partially initialized module 'mortie.orders'.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.
import reordering; the only substitutions are
tools→convert(50) andtools→orders/orders_mod(4). No test-body identifier changed, so theorders_modaliasing did not leak..github/workflows/build-wheels.yml:429genuinely namestest_tools.py, and it is the only workflow reference to it.Ruff bot (4 inline comments): all four are pre-existing on
origin/main, surfacedonly because this PR touches those files —
I001intest_mort_inverse.py,test_polygon_regression.py,test_tools.pyandD205ingeometry.py. Runningruff check mortieover agit archive origin/maintree reports the identical 14findings 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 fixpre-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.
SPLIT_BASESpin is itself verified against nothing — a change made in phase 1 to a phase-2-moved function escapes both arms (demonstrated)check_pinned_basesarm requires the pin to equal--basemodulo body-level imports; 16 mutations, 14 caught + 2 missed by design; review's synthetic seam now exits 1b7fe794CalledProcessError; the squash-merge caveat this body claimed was "noted in the script" was not in the file--helpsays--baseis partialcbb1a49Plus 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:
geometry.pyagainstorigin/main, not merely against each other. Review closed the seam by hand beforethe script could: indexing all 44 top-level definitions of
origin/main:mortie/geometry.pyagainst wherever they now live, exactly three differ—
_boundary_rings_xyz,_per_cell_polygons,_reject_hemisphere_cover— and everydifference is import statements only, matching
git diff origin/main 011816c -- mortie/geometry.py(4 insertions, 3 deletions). Nothing missing, nothing added. This isthe claim a reviewer of the merged result wants, and
check_pinned_basesnow assertsit on every run rather than leaving it as a one-off.
destinations, nothing missing or added, and every definition equal on four axes —
ast.dump,ast.get_source_segment, a decorator-inclusive source slice, andast.get_docstring. Module-level residue exact, which confirms the section-markerblock and the four-line
_DISSOLVE_SNAPcomment moved verbatim.dissolve.pydefinitions reference noneof the 27 that stayed and none import
.geometry; walking the 27 stayers,to_geometryis the only one reaching
_dissolved_polygons._per_cell_polygons's characterizationholds independently — no spherical reasoning, groups by order and delegates to
mort2polygon, sole callerto_geometryatgeometry.py:906.type,inspect.signatureand__doc__across all 69names, with phase 2 changing zero public
__module__(it moves only privatehelpers).
import mathis fully clean — no remaining use ingeometry.py. The fivenon-move hunks listed above are complete.
ruff check mortiereports 14 findings with acode multiset identical to
origin/main's.Phase 3–4 fold
Four adversarial-review findings, all
[low], and the review verified everysubstantive claim it checked — so this was a small fold, not a rework. One commit per
finding that needed one; two are body-only.
mortie.geometry— andgeometry.py's is published viadocs/api/geometry.mdmortie.codecingeometry.py(module docstring, incl. the title),dissolve.py:15-16, andtest_wkb_no_backend.py'sno_geometry_backend. Same class as thelib.rs:230finding, no behaviourc3122c4_BACKENDgate is live but order-dependent — this file alone is9 passed, two of them vacuouscheck_movesalone cannot take the test split either, andpytestcatches nothing in a test move70a7d87build-wheels.ymledit is correct, but CI never runs it — on PRs ormain; first execution is a release runneeds: test-wheelschain, and local emulation named as the verification of recordAlso folded, from the review's "noted but did not file": on a
check_pinned_basesfailure stdout still printed
44 definitions equal modulo importswhile stderr reportedthe mismatch — correct exit code, but it read as success to anyone eyeballing stdout. It
now prints
MISMATCH — N failure(s), listed belowinstead, mirroringcheck_public_surface's existing__all__: MISMATCH(634f114). Mutation-checked byrepointing
SPLIT_BASESat440a8fa: stdout wasmortie/geometry.py@440a8fa vs origin/main: MISMATCH — 17 failure(s), listed below,exit 1; mutation reverted,
git diffclean.Independent checks from this review that came back stronger than this PR claimed:
8f13e08^:test_tools.pyagainst both destinations: 17 in, 17 out, each equal on
ast.dump,ast.get_source_segmentand a decorator-inclusive source slice. Collected node IDsmatch too — 1350 at
440a8fa, 1350 at83de969; after normalising the file componentthe only difference in the whole set is
test_geo2mort_vs_tools→test_geo2mort_vs_convert._BACKENDgate does fail loud when warm. Adding_BACKEND = Noneback togeometry.pyand pointing the fixture atgeometry._BACKENDgives2 failed, 105 passed, 4 skipped; commenting out the clear alone does the same.test_geometry.py::test_backend_gate_messageis not order-dependent(
monkeypatch.setattrraises on a missing attribute), so a stale module referencecannot 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).— resolved:orders.pyholds the two_rust_*kernel-bridge aliases.@espg approved as-is. The dependency DAG forces the direction (see above), and a
morton ↔ NESTEDbridge arguably reading as a conversion does not outweigh it. Noduplicate alias lines; the single definition in
orders.pystands. No change.— resolved: @espg approved the rename andtest_tools.pykeeps its namethe 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.pyalready existed).Say if you would rather it had been a single
test_convert_orders.py.— resolved: @espg ruledconvert.pyat 860 lines is the largest survivor.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 inconvert.pyandpush it past the aim, but @espg's
batch.pyruling —#170 — sends them to
batch.pyinstead.So
batch.pycapsconvert.py's growth rather than shrinking it. Measured onthis branch:
convert.pyis 13 top-level defs across 860 lines and none of them isbulk today (largest is
mort2bboxat 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 #170either. 860 also sits comfortably under CLAUDE.md's 1,200 cap, with sub-1,200
overages pre-approved. The residual argument is cohesion —
mort2bbox/mort2polygonare 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.
—_normalize_antimeridian_polygon's deadon_antimeridianlocal (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, nottools.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 I001entry no longer exists, because phase 3 replaced that file with two isort-clean
ones. That is precisely why
ruff check mortiereports 13 againstmain's 14 —the delta is not a fix this PR made, it is a pre-existing finding whose file stopped
existing.
geometry.pylands at 971 — 29 lines under the aim. That is the one number inthis 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 cohesiveunit. Options as I see them: (a) accept 971 and treat the next
geometry.pychange asthe 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.pyis 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 landsgeometry.pynear 790; the recomputed figure is 772, and the one-way DAG anddissolve.py's independence both held under measurement.The pinned— resolved: @espg ruled keep the pin (theSPLIT_BASESsha.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:
stops resolving —
unreachable()returns"… is not reachable in this clone — repoint or drop its SPLIT_BASES entry"and the run exits 1. An orphaned shadegrades the check to "cannot run", never to "passed". That was added by the phase-2
fold (
cbb1a49) and documented in the comment aboveSPLIT_BASES; the question wasdrafted as though it were still an open hazard.
011816cais an ancestor of this PR's head, verified withgit merge-base --is-ancestor. So it survives a merge commit untouched, and isorphaned 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_BASESentry and themortie/geometry.pyarm ofSPLITSat merge, which is the lifecycle the script'sown comment already documents. The file itself stays either way — which disposes
of the third option in the original question, deleting
verify_pure_move.pyon 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).The moved section-marker comment in— resolved: @espg ruled itdissolve.pymoves verbatim, as implemented. No change. The reasoning worth recording is that the
marker is not a one-off:
origin/main:mortie/geometry.pycarried four of them, andeach travelled with its own section —
# ── 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:34so 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 filesto 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 dissolvealgorithm — 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 doesnot belong in a pure-move PR. Flagged, deliberately not fixed.
The test split is outside the verifier's model— resolved: @espg ruled noverifier 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 isreview-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.