API consolidation: one polymorphic function per operation (issue #187) - #195
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #195 +/- ##
==========================================
- Coverage 96.67% 96.53% -0.14%
==========================================
Files 20 20
Lines 2283 2365 +82
==========================================
+ Hits 2207 2283 +76
- Misses 76 82 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial review of fa0c559 ("phase 1 of issue #187"), fresh context, against CLAUDE.md and the ruling at #187 (comment).
No blocking correctness defect. The four delegations are wired correctly and I could not make the polymorphic form disagree with a loop over the scalar form on any value. The findings below are documentation claims that are false as written, and tests that do not discriminate what they claim to.
Everything below was run against a clean extraction of fa0c559 (git archive fa0c559 | tar -x into a scratch dir, with the built _rustie.abi3.so copied in), because the working tree already carries uncommitted phase-2 work and would not have reflected the reviewed commit.
Findings, ranked
| # | severity | finding |
|---|---|---|
| (1) | high — false doc | moc.py:86-88 "The input shape selects the form — there is no separate plural entry point" — both halves false. A keyword selects the form, not the shape; and mocs_to_orders is still in __all__ (__init__.py:163) and still called by this PR's own test at test_polymorphic_api.py:51. |
| (2) | high — false doc | moc.py:439-440 "the message names the lowest-index offending group" — false. common_ancestor(v, offsets=[0,0,3,99]) → group 2: offset 99 exceeds value array length 6 while group 0 is empty. batch.py:697-703 carries the correct qualifier ("within its kind"; layout checked batch-wide first) and this docstring drops it. |
| (3) | medium — test | test_polymorphic_api.py:164 match="group 2|2:" pins nothing — the 2: arm matches any message containing those two characters, including ones naming no group. Same looseness at :75 (match="moc 0" also matches a layout error) and :133 (bare raises, no match). |
| (4) | medium — false doc | test_polymorphic_api.py:7-9 "the error surface passes through unchanged" — message text and validation precedence both change. moc_to_order(v, 99, max_cells=-1) raises the order error; adding offsets= makes it raise the max_cells error instead. |
| (5) | medium — API/test gap | moc_and/moc_intersects lose the scalar's commutativity in the offsets= form, and when len(a) == len(values) a swapped call silently returns a different answer rather than raising. No test pins which operand is the column. |
| (6) | low — coverage | offsets as list / single-group / offsets=[0] / non-monotone / max_cells=None with offsets= all untested (all verified working). The max_cells=None case is the one that would catch a delegation that dropped the forward. Non-integral float offsets truncate silently ([0., 7.9, 8.] partitions at 7). Keyword-only asserted for moc_to_order only. |
| (7) | low — style §4 | Function-local from .batch import … ×4 (:130, 207, 252, 459), no comment. I verified there is no cycle to break and no lazy-load benefit (__init__.py imports mortie.batch anyway). No deadlock risk. |
| (8) | low — consistency | batch.py still calls these four "the scalar (one MOC) form" (e.g. :435) and its module docstring still says "every scalar's docstring points back at its plural here". Stale after this diff — presumably phase 3's sweep, flagging so it is not lost. |
| (9) | low — sequencing | Within moc.py, four functions now carry "Batch vectorized (issue #187)" while moc_or, moc_minus, moc_xor, moc_not, compress_moc, split_base_cells carry nothing, so "unmarked" is ambiguous between not vectorized and not yet swept. Deferred to phase 3 per the PR body, which is reasonable — noting the intermediate state. |
What I checked and found correct
- Argument order into every kernel.
mocs_to_orders(values, offsets, order, max_cells)(batch.py:350) vs the call atmoc.py:131— matches, including theorder/max_cellstransposition relative to the scalar signature.mocs_and(a, values, offsets),mocs_intersect(a, values, offsets),common_ancestors(values, offsets)likewise. max_cellsdefault propagation. Both defaults are_FLAT_COVER_WARN_THRESHOLDimported fromcoverage, and the value is genuinely forwarded — the budget-refusal test at:73would fail if it were dropped.moc_min = common_ancestor(moc.py:466) is a plain alias, so it does inherit the keyword:inspect.signature(mortie.moc_min)→(morton, *, offsets=None).- No value-level disagreement. 500 randomized ragged columns (orders 1-5, 0-40 words, 1-5 groups, empty groups, empty shared covers, empty columns):
moc_and,moc_intersectsandmoc_to_orderin theoffsets=form were byte-identical to a Python loop over the single-item form, 0 mismatches each. Themoc_intersects⟷moc_andagreement claim (moc.py:242-244) also held in all 500. - Are the tests discriminating? Mostly yes — swapping
order/max_cellsintomocs_to_orders, or wiringmoc_andto a different kernel, both trip existing tests. The exceptions are themax_cells=Nonedirection (6) and the operand order (5).
Gate claims in the PR body — verified
Re-run on the clean fa0c559 checkout:
pytest→ 1557 passed, 16 skipped ✅ exactly as claimed.- New file collects 13 tests, all passing ✅ as claimed.
flake8 mortie --select=E9,F63,F7,F82→ clean ✅.numpydoc lint mortie/moc.py→ clean ✅ (offsetsis documented on all four; PR01-PR09/RT01/SS01 pass).mortie/moc.py:220is 100 chars ✅ and is pre-existing docstring prose, correctly left alone per §4.
Unverified: the "baseline on main was 1544 passed, 16 skipped" claim — I did not run the suite on main. 1557 − 13 = 1544 is arithmetically consistent, but that is not an independent check.
Conventions
- §4 module limit:
moc.py531 lines,batch.py864 — both under ~1000. ✅ - §4 dependencies: none added. ✅
- §3 commit message:
phase 1 of issue #187— title only, matches the log. ✅ - §7: the "Batch vectorized" note is in the Extended Summary rather than forced into the parameter table, which is what §7 asks for. ✅
- The
offsets=design is a real deviation from the ruled "numpy semantics exactly — input shape describes the batch". PR Question (2) puts it to review correctly; my only objection is finding (1), that the docstring asserts the ruled design while the code implements the alternative.
Generated by Claude Code
| def test_moc_and_offsets_rejects_offsets_not_covering_values(column): | ||
| values, offsets = column | ||
| shared = values[:2] | ||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
🤖 from Claude (review)
🤖 Coverage gaps for the offsets argument itself. I exercised each of these against fa0c559 and they all behave correctly — so these are missing tests, not bugs. Listing them here because this is the only test in the file that touches offsets validation, and it does so with a bare pytest.raises(ValueError):
| input | behavior I observed | tested? |
|---|---|---|
offsets as a Python list |
works on all four functions | no |
offsets=[0, n] (single group) |
works | no |
offsets=[0] (zero-MOC column) |
(array([], uint64), array([0])); moc_intersects → empty bool array |
no |
offsets=[] |
ValueError: offsets must have at least one element |
no |
non-monotone [0, 8, 4, 8] |
ValueError: moc 1: offsets must be monotonically non-decreasing (4 < 8) |
only via the bare raises here |
max_cells=None with offsets= |
works | no — see below |
Two of these are worth adding specifically:
(1) max_cells=None with offsets=. test_moc_to_order_offsets_keeps_per_item_budget_refusal (:73) proves max_cells is forwarded in the raising direction, but nothing covers the None direction. A delegation that hardcoded the default instead of forwarding — mocs_to_orders(morton, offsets, order) — would still pass every test in this file except that one, and would then refuse a legitimate max_cells=None densify.
(2) Non-integral float offsets truncate silently. np.asarray(..., dtype=np.int64) in the kernel floors rather than raising, so the column is silently mis-partitioned:
mortie.moc_to_order(v8, 3, offsets=np.array([0., 7.9, 8.]))[1]
# array([ 0, 28, 32]) <- partitioned at 7, no warningThat is pre-existing kernel behavior, not introduced here — but this PR is what puts it behind the singular public name, so it is worth a decision (document it, or reject non-integral offsets) rather than leaving it undiscovered.
Also: keyword-only enforcement is asserted for moc_to_order only (:79). moc_and, moc_intersects and common_ancestor have the same * guard and no equivalent test — cheap to parametrize.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Mostly superseded in 6cb2a16; the one remaining gap is closed in cc80347, and one item stays standing for you.
Your coverage table — five of the six rows became tests in 6cb2a16, in a new "Layout and budget edges shared by every offsets form" section: test_offsets_accepts_a_plain_list, test_offsets_single_group_and_empty_column (covers both [0, n] and [0]), test_offsets_rejects_non_monotone_layout, and test_offsets_honours_max_cells_none.
Your (1), max_cells=None with offsets= — that was the sharpest one, since a delegation that hardcoded the default would have passed everything else in the file. It is now pinned by value, not just by not-raising:
got_v, _ = mortie.moc_to_order(values, 12, max_cells=None, offsets=offsets)
assert got_v.size > (1 << 20)Keyword-only enforcement — this was the last open item, and it is fixed in cc80347. It was asserted for moc_to_order only, with a bare pytest.raises(TypeError). It is now parametrized across all five offsets= entry points and pinned to the message:
@pytest.mark.parametrize("call", [
lambda v, o: mortie.moc_to_order(v, 4, None, o),
lambda v, o: mortie.moc_and(v[:2], v, o),
lambda v, o: mortie.moc_intersects(v[:2], v, o),
lambda v, o: mortie.common_ancestor(v, o),
lambda v, o: mortie.toc_reduce(v, o),
], ids=[...])
def test_offsets_is_keyword_only_on_every_form(column, call):
with pytest.raises(TypeError, match=r"positional argument"):
call(values, offsets)Your (2), non-integral float offsets truncating silently — left as designed, and now documented rather than undiscovered. It is the batch family's np.asarray(..., dtype=np.int64) convention, so changing it for one function would split the family; 50f491b spelled it out on the offsets parameter of the packed-column form ("float offsets truncate toward zero"). Rejecting non-integral offsets across the family is an API decision rather than a diff-scoped fix, so that one stays standing for you — it is the "document it, or reject it" fork you named, and I only did the documenting half.
Generated by Claude Code
| Without ``offsets``, ``True`` if the two covers share any area. With | ||
| ``offsets``, a ``bool`` array of length ``len(offsets) - 1``, agreeing | ||
| item-for-item with the non-empty slots of the ``offsets`` form of | ||
| :func:`moc_and`. |
There was a problem hiding this comment.
🤖 from Claude (review)
Verification note, no change requested — this is the one new claim in the diff I most expected to break, and it holds.
``offsets``, a ``bool`` array of length ``len(offsets) - 1``, agreeing
item-for-item with the non-empty slots of the ``offsets`` form of
:func:`moc_and`.
I fuzzed it over 500 randomized ragged columns against fa0c559 — orders 1-5, 0-40 words per column, 1-5 groups per column, including empty groups, empty shared covers, and empty columns — asserting moc_intersects(a, v, offsets=o) == (np.diff(out_offsets) > 0) where out_offsets comes from moc_and(a, v, offsets=o). 0 mismatches.
The same run also pinned the delegation itself: moc_and, moc_intersects and moc_to_order in the offsets= form were byte-identical to a Python loop over the single-item form in every one of the 500 trials (0 mismatches each). So I could not construct an input where the polymorphic form and the loop-over-scalar form disagree on values — the divergences I did find are confined to error messages and validation precedence (separate comment on the test module docstring).
One gap this leaves: test_moc_intersects_offsets_agrees_with_moc_and_slots (mortie/tests/test_polymorphic_api.py:122) exercises exactly one fixture, whose expected answer is [True, True, False, False]. That is discriminating enough for a wiring error, but the claim in this docstring is a general one — a handful of parametrized shapes (empty a, containment in both directions, a column that is entirely empty slots) would make the docstring and the test cover the same ground.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Acknowledged, no change made — you explicitly requested none, and the claim you fuzzed still holds at the current head.
Recording the disposition so this thread is not left silent:
- The
moc_intersectsdocstring claim (theboolarray agrees item-for-item with the non-empty slots ofmoc_and'soffsetsform) is unchanged since your 500-trial run, andtest_moc_intersects_offsets_agrees_with_moc_and_slotsstill asserts it againstmoc_and's output offsets. The delegation it rides on is unchanged too, beyond the phase-4 rename of the kernel to_mocs_intersect. - Your finding that the polymorphic form and the loop-over-scalar form never disagree on values is consistent with everything since: the divergences that did get folded (the separate thread on the test module docstring, fixed in cc80347) are confined to message text and validation precedence, exactly as you scoped them.
Left standing for you: the one gap you named — broadening test_moc_intersects_offsets_agrees_with_moc_and_slots from its single [True, True, False, False] fixture to a parametrized set (empty a, containment in both directions, an all-empty-slot column) so the test covers the same ground as the general docstring claim. That is a test-breadth addition rather than a defect, and this PR is complete and awaiting a merge decision, so I did not widen the diff for it. Say the word and it is a small follow-up.
Generated by Claude Code
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Adversarial review of d9352f3 ("phase 2 of issue #187") only — fa0c559 is covered separately. Everything below was verified by running Python against the installed build, not read off the diff.
The delegation shape is right and matches phase 1: three thin branches, no reimplementation, kernels untouched. toc_reduce is clean. The two real defects are both in decimal_to_word, and both are places where the docstring promises something the code does not do.
Ranked findings
| # | severity | finding |
|---|---|---|
| 1 | blocking | decimal_to_word(..., dtype=MortonIndexScalar) with array input is silently accepted and returns a plain uint64 array. MortonIndexScalar subclasses np.uint64, so np.dtype(MortonIndexScalar) == np.uint64 is True and the new guard passes it. The Raises clause says it raises. The new test only tries dtype=int. → morton_index.py:161 |
| 2 | blocking | A 0-d string array returns a 0-d array, not a scalar — breaking the ruling on question (2) ("numpy semantics exactly (0-d/scalar in → scalar out)", #187 (comment)) and the docstring's own "numpy semantics literally". np.str_ works only because it subclasses str. → morton_index.py:124 |
| 3 | non-blocking | max_cells is silently ignored on the scalar form (max_cells=1 returned 1024 cells). The docstring's justification — "the scalar form's result is bounded by its one parent" — is false: order 3 → order 29 is 4**26 cells. → orders.py:400 |
| 4 | non-blocking | A 2-D parent array is silently flattened: (2, 2) in gives (4, 16), structure gone, no error. decimal_to_word in the same commit preserves shape and has a test for it. One commit, one "numpy semantics" banner, two answers about rank. → orders.py:431 |
| 5 | non-blocking | Docstring claim "passing an array used to silently describe only its first element" — verified against main, and it is true only for target_order > parent_order. For target_order == parent_order the old code returned (1, n) containing all parents; the new form returns (n, 1). Please scope the sentence. → orders.py:390 |
| 6 | non-blocking | decimal_to_word(["12341", 5]) now silently parses 5 as the order-0 id "5" — a call that previously raised. Pre-existing hole in decimals_to_words (whose own comment states the opposite intent), newly reachable through this collapse. Also: the array-form ValueError names the offending string, not its index, though the docstring says "in input order". → morton_index.py:150 |
| 7 | non-blocking | Two new tests use bare pytest.raises(ValueError) where the documented claim is about the message — test_toc_reduce_offsets_refuses_an_empty_group (the docstring promises the group is named; phase 1's sibling test used match=), and test_generate_morton_children_array_honours_max_cells. Both would pass on an unrelated ValueError. → tests :192, :260 |
| 8 | nit | Errors surface the delegate's name, not the called function's: decimal_to_word(None) → "decimals_to_words expects…"; generate_morton_children(arr, 1) → "children_of only refines…"; empty group → "tocs_reduce of an empty segment". If the plurals retire in phase 4, these messages will name functions that no longer exist. |
| 9 | nit | Return-type divergence across the PR: singular toc_reduce → Python int, common_ancestor → np.uint64, decimal_to_word → np.uint64. Documented per-function, inherited rather than ruled. Worth a line under "Questions for review". |
Missing edge cases (none covered today): 0-d array and np.str_ for decimal_to_word; dtype=MortonIndexScalar and the three accepted uint64 spellings for the array form; bytes input; empty-array shapes (decimal_to_word([]) → (0,), generate_morton_children([], k) → (0, 1)); 2-D parents; max_cells with a scalar; the mixed-order refusal through the new path; offsets/max_cells keyword-only enforcement for the phase-2 functions (phase 1 pinned this for its own).
Discrimination check — the happy-path tests do hold up. Inverting if offsets is not None breaks the toc parity test; wiring decimal_to_word to the wrong kernel breaks its plural-and-loop assertion; dropping the max_cells forward makes the budget test fail; widening np.ndim(...) > 0 to >= 0 breaks test_generate_morton_children_scalar_stays_one_dimensional. The gaps are edges, not wiring.
Conventions (§3, §4, §7) — clean
numpydoc lint mortie/toc.py mortie/orders.py mortie/morton_index.py— no output, clean.flake8 mortie --select=E9,F63,F7,F82— clean (rc 0).flake8 --max-line-length=88on the four touched files — one hit,morton_index.py:346 F811 redefinition of unused '_pandas_ext'. Pre-existing: the same F811 is onmain(at line 326 there). Flagging per §4, not asking for a fix in this PR.pytest -qfull suite — 1567 passed, 16 skipped (1544 baseline + 23 in this file).- Module sizes vs the ~1000-line limit:
toc.py765,batch.py864,orders.py442,morton_index.py360 — all under, butbatch.pyis the one to watch before phase 3 adds to it. - Commit message
phase 2 of issue #187— title-only, matches §3 and recentgit log. No new dependencies. No CI/workflow files touched.
One process note
The PR body's Phase 2 checklist lists four pairs — toc_reduce, decimal_to_word, generate_morton_children, from_wkb — and this commit lands three. The checkbox is still unchecked, so I read that as deliberate rather than a miss; worth a one-line status note on the thread either way so the next run does not re-derive it.
For reproducibility: the working tree at review time carried uncommitted phase-3 edits across 13 modules. I diffed them — docstring-only additions, no behavioural change — so every result above is attributable to d9352f3. Line references are to the committed blobs at d9352f3.
Generated by Claude Code
| If ``s`` is a malformed decimal Morton id -- naming the first | ||
| offender, in input order, for the array form. |
There was a problem hiding this comment.
🤖 from Claude (review)
Two smaller accuracy points on the widened Raises contract.
(1) "naming the first offender, in input order" — the message names the string, not its position. Verified:
decimal_to_word(np.array(["12341", "zzz", "9999"]))
-> ValueError: malformed decimal Morton id 'zzz'
There is no index, so on a 10M-row array a caller cannot locate the offending row. Contrast the phase-1 wording elsewhere in this PR ("naming the lowest-index offender") and children_of's word 4217: … style, which do carry the index. Either soften the claim to "naming the first malformed id" or add the index in decimals_to_words. Nothing in the new tests asserts either way.
(2) A mixed list now silently coerces a non-string into an id, on a path that previously raised. Verified:
decimal_to_word(["12341", 5])
-> array([1639310264362860548, 5764607523034234880], dtype=uint64)
The 5 became the order-0 id "5". decimals_to_words guards this only when the whole array coerces to a non-U/O dtype — its own comment at line 236-238 states the intent explicitly ("Do not let numpy's str-coercion silently turn e.g. the integer 1 into the order-0 id") — but numpy types ["12341", 5] as <U…, so the guard never fires. That is a pre-existing hole in the kernel, but this commit is what routes decimal_to_word (previously str-only, and a TypeError for this input) into it, so the widening is new here. A list/tuple element-type check alongside the existing object-array check would close it. Worth a test either way.
Generated by Claude Code
There was a problem hiding this comment.
🤖 from Claude
Your (1) is fixed in cc80347; your (2) still reproduces and is left standing for you, because closing it is an API narrowing rather than a diff-scoped fix.
(1) "naming the first offender, in input order". You were right that the message names the string, not its position, so a caller on a 10M-row array cannot locate the row. I took the soften-the-claim option rather than adding an index, since adding one changes the message text of a shipped kernel. decimal_to_word's Raises now reads:
ValueError
Ifsis a malformed decimal Morton id -- for the array form, naming the first malformed id in input order, not its index, so a wide array gives no row to look at.
That also brings the public docstring into line with the kernel's own, which already said "Naming the first malformed id, in input order".
(2) The mixed list. Re-verified at the current head — unchanged:
decimal_to_word(["12341", 5])
-> array([1639310264362860548, 5764607523034234880], dtype=uint64)
The 5 still becomes the order-0 id "5". Your reading of the cause is right: _decimals_to_words' guard is dtype-driven, and numpy types ["12341", 5] as <U… rather than object, so the guard's own stated intent ("Do not let numpy's str-coercion silently turn e.g. the integer 1 into the order-0 id") is defeated for the mixed case — the object-array branch below it only catches dtype.kind == "O".
I did not fix it because the fix is to start rejecting input that is accepted today, on a public parse surface, which is a scope decision and not mine to take on a review comment (CLAUDE.md §6). The two shapes I can see are (a) reject when a U-dtype array came from a sequence containing non-strings, or (b) document the coercion. Standing for you, along with a note that this is pre-existing kernel behaviour rather than something the polymorphic collapse introduced — as you said, the PR is what puts it behind the singular public name.
Generated by Claude Code
|
🤖 from Claude Phase 3 landed ( Blocking, and both were true
if isinstance(dtype, type) and issubclass(dtype, MortonIndexScalar):
uint64_asked = FalseA 0-d string array returned a 0-d array, not a scalar. That contradicts the ruling this PR is built on — "numpy semantics exactly (0-d/scalar in → scalar out)" (#187 (comment)) — and the docstring's own "numpy semantics literally". Also folded
Not acted on — raising instead
Gates after the fold
Phases 1-3 are done. Phase 4 is genuinely blocked on the five questions in the PR body — chiefly (1) whether the retirement is a removal or a deprecation shim, and (4) how Generated by Claude Code |
Merging this PR will improve performance by 10.45%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | coverage_triangle[4] |
140.2 µs | 126.9 µs | +10.45% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/187-polymorphic-api (9f569a9) with main (feb374e)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(bd3c0c3) during the generation of this report, so feb374e was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
| # _wkb_bytes from this module at import time. | ||
| from .batch import _from_wkbs | ||
|
|
||
| return _from_wkbs(blobs, order=order, tolerance=tolerance, |
There was a problem hiding this comment.
🤖 from Claude (review)
Blocking test gap — the batch delegation's parameter forwarding is not pinned by anything, and tolerance / max_cells are exactly the two the migration hazard corrupts.
The forward here is correct as written. But nothing in the suite would notice if it weren't. I dropped both parameters on the floor:
# mortie/geometry.py, _from_wkb_batch
return _from_wkbs(blobs, order=order, tolerance=None,
max_cells=None, normalize=normalize,
latitude=latitude)and ran the whole suite:
$ python -m pytest -q --no-cov -p no:cacheprovider
1695 passed, 16 skipped, 8024 warnings in 102.70s
Identical to the unmutated run (1695 passed, 16 skipped in 115.99s). No test passes tolerance= or max_cells= through either polymorphic batch form — grep over mortie/tests/ finds them only on marrow.from_wkb (test_arrow_wkb_batch.py:320,352, which reaches _from_wkbs directly, bypassing this function) and on the scalar from_wkb (test_wkb_no_backend.py:160-161). latitude is pinned (test_authalic.py:587) and normalize is pinned via the arrow skin, so those two are covered; tolerance and max_cells are not, in either the sequence form or the offsets= form.
The PR body says the batch is "byte-identical … pinned by delegation-spy and parity tests". The spy (test_polymorphic_api.py:537) takes **kwargs and asserts nothing about them, so it pins the route, not the payload.
Suggested pin, one test, both forms:
def test_from_wkb_batch_forwards_every_parameter(blobs):
for kw in (dict(tolerance=2.0), dict(max_cells=8), dict(normalize=False),
dict(latitude="geodetic-spherical")):
want = _from_wkbs(blobs, order=7, **kw)
for got in (mortie.from_wkb(blobs, order=7, **kw),
mortie.from_wkb(b"".join(blobs), order=7,
offsets=np.cumsum([0] + [len(b) for b in blobs]),
**kw)):
np.testing.assert_array_equal(got[0], want[0])
np.testing.assert_array_equal(got[1], want[1])I verified that assertion passes on the real code (all four kwargs, both forms) — so it is a pure gain, not a redesign.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 77f9e79 — the pin now exists and the mutation you ran fails it.
The spy captures kwargs (calls.append((list(entries), kwargs))) and asserts the exact forwarded dict for both batch forms, and two new tests were added next to it: test_from_wkb_batch_forwards_every_coverage_knob (your suggested parity test, tolerance / max_cells / normalize / latitude, sequence form and offsets= form, against _from_wkbs) and test_from_wkb_batch_coverage_knobs_bind_behaviourally (a behavioural pin: max_cells=8 and tolerance=2.0 produce a strictly smaller cover than the default, so the knobs cannot be inert even if the kernel parity ever became vacuous).
I reproduced your mutation to check it is now caught — with tolerance=None, max_cells=None hardcoded in _from_wkb_batch's forward:
$ python -m pytest -q --no-cov -p no:cacheprovider mortie/tests/test_polymorphic_api.py
FAILED ...::test_from_wkb_batch_routes_through_the_chunked_kernel
FAILED ...::test_from_wkb_batch_forwards_every_coverage_knob
FAILED ...::test_from_wkb_batch_coverage_knobs_bind_behaviourally
3 failed, 44 passed
Unmutated: 1699 passed, 16 skipped.
| _wkb_column_views(data, offsets), moc, order, normalize, | ||
| tolerance, max_cells, latitude, | ||
| ) | ||
| if isinstance(data, (list, tuple)) or ( |
There was a problem hiding this comment.
🤖 from Claude (review)
Undocumented API narrowing: three input shapes from_wkbs accepted now raise TypeError — including a pandas Series, which this function's own docstring names as the motivating case.
The dispatch admits exactly list / tuple / object-ndarray. The retired from_wkbs had no dispatch at all — it just iterated — so it accepted any sequence or iterable. Measured, kernel vs. new entry point, same blobs:
kernel _from_wkbs(pandas Series): OK ((62,), [0, 26, 62])
public from_wkb(pandas Series) : TypeError: WKB input must be bytes, a hex string, or a byte buffer; got Series
kernel _from_wkbs(S-dtype ndarray): OK ((62,), [0, 26, 62])
public from_wkb(S-dtype ndarray) : TypeError: WKB input must be a buffer of bytes; got one of 77-byte items (format '77s')
kernel _from_wkbs(generator): OK ((62,), [0, 26, 62])
public from_wkb(generator) : TypeError: WKB input must be bytes, a hex string, or a byte buffer; got generator
(list, tuple, object-ndarray, list of hex str, list of memoryview, and [] all still work identically — verified.)
Three things make this worth a decision rather than a shrug:
- The docstring claims the opposite. Line 502: "
list/tuple/ object-ndarray— a sequence batch: each entry is coerced exactly as the scalar form accepts (the pandas case)." Apd.Seriesof WKB is the pandas case (gdf.geometry.to_wkb()hands one back);.to_numpy()is required now and nothing says so. - The ruling says both pre-existing call shapes survive as a pure rename. For
Series/S-dtype / generator callers this is a break, not a rename. - Nothing records it. The CHANGELOG row is
from_wkbs(blobs, ...)→from_wkb(blobs, ...)with no accept-list note.
The S/bytes_-dtype case is the sharpest: np.array(blobs) on a list of bytes produces dtype='S' by default, so the most obvious "make it an array" move lands in the error branch, and the message it gets ("must be a buffer of bytes; got one of 77-byte items") describes buffer geometry rather than telling the caller to pass a list or an object array.
Options, cheapest first: (a) widen the sequence branch to np.ndarray with dtype.kind in "OS" plus a collections.abc.Sequence/iterator fallback — restores the old accept-list; (b) keep the narrow rule but add the three shapes to the CHANGELOG and drop "(the pandas case)" from the docstring, or respell it "the pandas case, via .to_numpy()"; (c) keep the rule but make the scalar-path TypeError name the batch spellings when it sees something sequence-shaped. Either way the docstring and the CHANGELOG need to match whichever is chosen.
There was a problem hiding this comment.
🤖 from Claude
Option (b), folded in 9e06135 — the dispatch stays as ruled, the docs and the CHANGELOG now match it.
The narrowing is deliberate, not an oversight: espg's ruling 4 on this PR fixes from_wkb's dispatch as exactly offsets= → packed column, list / tuple / object-ndarray → sequence batch, buffer spellings → one blob, with no wider sniffing. Widening to dtype.kind in "OS" plus a Sequence/iterator fallback (your option (a)) is precisely the sniffing that ruling excludes, so it is not mine to take — it stays standing for @espg if the accept-list should be reopened.
What changed instead, so nobody discovers this at the call site:
from_wkb's docstring — the sequence bullet now reads "the pandas case, viaseries.to_numpy()", and a new paragraph after the three bullets states that they are the whole dispatch and names the three shapes (Series, generator,S-dtype) with the materializing spelling for each. Thedataparameter repeats it where a reader looking up the argument will see it.CHANGELOG.md— thefrom_wkbsrow now carries the note inline ("materialize anything else first:list(gen),series.to_numpy(),arr.astype(object)"), and thefrom_wkbnote paragraph spells out that the dispatch is exhaustive by design and what that costs those three callers.- A test pins it as behaviour rather than prose:
test_from_wkb_batch_dispatch_is_exhaustiveasserts theS-dtype array and the generator raiseTypeError, and that both documented materializations (.astype(object),list(...)) give byte-identical results to thelistform.
I left the scalar-path TypeError text alone (your option (c)). It is shared with the single-blob path, where "must be a buffer of bytes; got one of 77-byte items" is the correct diagnosis; making it name the batch spellings would need a sequence-shape sniff at exactly the site the ruling closed. Your point that np.array(blobs) is the obvious wrong move stands — it is now the first thing the CHANGELOG row says.
| one blob. | ||
| """ | ||
| if moc is not None and not moc: | ||
| raise ValueError( |
There was a problem hiding this comment.
🤖 from Claude (review)
Silent wrong answer on the positional migration the PR itself flags (question 8): the guard tests moc for truthiness, so a tolerance float bound to moc passes through and the tolerance is dropped without a word.
if moc is not None and not moc means every truthy object — a float, a string, an array — reads as "MOC output requested" and proceeds. So from_wkbs(blobs, order, tol) migrated positionally does not raise; it silently returns a different, finer cover:
tight = _from_wkbs([blob, blob2], 7, 2.0) # old: from_wkbs(blobs, order, tolerance)
mig = mortie.from_wkb([blob, blob2], 7, 2.0) # naive positional migration
loose = _from_wkbs([blob, blob2], 7) # no tolerance at all intended (tolerance=2.0) cells: 193
migrated call cells: 883
tolerance dropped entirely: True | equals intended: False
4.6× the cells, no error, no warning. This is the one failure mode worse than a TypeError — and the CHANGELOG note ("tolerance= must be spelled as a keyword") is documentation, not a gate; the caller who reads it did not need it.
A one-line type guard closes it without touching the shape the ruling protects, because the surviving scalar calls all pass a bool:
if moc is not None and not isinstance(moc, (bool, np.bool_)):
raise TypeError(
f"from_wkb's `moc` is a bool (tri-state with None), got "
f"{type(moc).__name__} -- migrating a positional from_wkbs("
f"blobs, order, tol) call? spell it tolerance=."
)I checked what this would cost: mortie.from_wkb(blob, 6, True) and from_wkb(blobs, order=6, moc=np.True_) both still pass, and the whole existing suite exercises moc only as True / False / unset. Placing it in from_wkb (not just _from_wkb_batch) also catches the scalar spelling from_wkb(blob, 8, 0.5), which today silently returns a MOC instead of a flat cover.
If a guard is judged out of scope for phase 4, then at minimum the error message the batch already raises should be reachable here — right now moc=2.0 is the one value that neither raises nor honours what the caller meant.
There was a problem hiding this comment.
🤖 from Claude
Fixed in affa852 — the guard is in, essentially as you wrote it, in from_wkb itself so it catches the scalar spelling too.
if moc is not None and not isinstance(moc, (bool, np.bool_)):
raise TypeError(
"from_wkb's `moc` is a bool (tri-state with None), got "
f"{type(moc).__name__} -- migrating a positional "
"from_wkbs(blobs, order, tol) call? from_wkb's third positional "
"is moc; pass tolerance= as a keyword."
)from_wkb(blobs, 7, 2.0) now raises instead of quietly returning the 883-cell cover where 193 was asked for, and from_wkb(blob, 8, 0.5) — the scalar case you flagged as returning a MOC where a flat cover was meant — raises the same way.
Pinned by test_from_wkb_refuses_a_non_bool_moc: both forms raise on a float third positional; from_wkb(blob, 8, True) and from_wkb(blob, 8, np.True_) still agree byte-for-byte; moc=False still returns the flat 1-D cover; from_wkb(blobs, 8, np.True_) still returns the ragged pair. Full suite 1699 passed, 16 skipped — nothing in it was passing a non-bool moc.
Also documented: from_wkb's Raises gained the TypeError with the reason, and the CHANGELOG's positional-migration sentence now says the migration raises rather than only that tolerance= should be a keyword.
There was a problem hiding this comment.
🤖 from Claude
Ruled (2026-08-24, review question 8) and closed out in e0eba6b: the guard is the accepted design, hardened one notch — the message now names the received value and type (got 0.5 (float)), not just the type, and the pins are widened to what the ruling asked: float/str/int each refused with the message text in all three dispatch forms (scalar, sequence batch, packed column — the guard sits ahead of the dispatch, so one guard covers them), bool/None accepted everywhere, and the arrow skin covered by test_the_skin_has_no_moc_slot_to_poison — it bypasses this entry point but takes no moc at all, so arrow.from_wkbs(col, order, tol) migrated positionally binds tol to tolerance, which is what it meant. CHANGELOG line under the migration table updated to match.
| the endpoint that failed. | ||
| """ | ||
| try: | ||
| view = memoryview(data).cast("B") |
There was a problem hiding this comment.
🤖 from Claude (review)
The documented TypeError never fires: memoryview(x).cast("B") accepts any C-contiguous buffer regardless of item size, so the packed-column path is laxer than the scalar path it claims to mirror.
The docstring above says:
TypeError — If
datais not a contiguous buffer of one-byte items.
cast("B") only requires C-contiguity. A float64 or int32 array is silently reinterpreted byte-wise:
f = np.frombuffer(packed + b"\0" * (8 - len(packed) % 8), dtype=np.float64)
_wkb_column_views(f, [0, len(blob), f.nbytes])
mortie.from_wkb(f, offsets=[0, len(blob), f.nbytes], order=6)
mortie.from_wkb(f, order=6) # the same object, scalar path _wkb_column_views(float64 array) -> ACCEPTED, 2 views, first 010300000001
from_wkb(float64 buffer, offsets=...) -> OK 344 cells [0, 205, 344]
from_wkb(int32 buffer, offsets=...) -> OK 344 cells
scalar from_wkb(float64 array): TypeError WKB input must be a buffer of bytes; got one of 8-byte items (format 'd')
So the same object is refused by name on the scalar path (_wkb_bytes checks itemsize) and accepted on the column path — the opposite of the "the layout checks mirror the batch family's offset validation" framing, and the accept-list widening is undocumented. In practice the reinterpretation usually ends in a confusing WKB parse error at some blob index rather than a clean "that is not a byte buffer", which is a worse diagnostic for the arrow-column callers this form exists for.
Fix is one line, and reuses the check the scalar path already has:
try:
view = memoryview(data)
if view.itemsize != 1 or not view.c_contiguous:
raise TypeError
view = view.cast("B")
except TypeError:
raise TypeError(...) from NoneTwo smaller things in the same function while it is open:
OverflowErrorescapes the documentedRaises.np.asarray(offsets, dtype=np.int64)on a Python int pastint64raises before any validation runs:mortie.from_wkb(packed, offsets=[0, 10**19, len(packed)])→OverflowError: Python int too large to convert to C long. That contradicts the PR body's "every refusal is still a catchableValueError". (The rest of the batch family has the same convention, so this may be a deliberate carry-over — but the docstring'sRaisesshould say so.)- Float offsets are silently truncated.
offsets=[0.0, 26.7, 77.0]is accepted as[0, 26, 77]. Same family convention, so probably fine by design; worth a word in theoffsetsparameter description since this form is fed by external buffers.
The rest of the validation I checked and it is right: offsets[0] != 0, monotonicity by lowest index, per-blob over-run, the exact-cover endpoint, offsets=[], offsets=[0] over empty data, a list passed as data (clean TypeError), and the zero-copy claim (views[0].obj is buf → True).
There was a problem hiding this comment.
🤖 from Claude
Fixed in 50f491b — both halves.
_wkb_column_views now takes memoryview(data) first, refuses itemsize != 1 by name (mirroring _wkb_bytes' message shape), and only then casts to "B" for shape normalization:
>>> mortie.from_wkb(f64_buffer, order=6, offsets=[0, n0, f.nbytes])
TypeError: with offsets, the WKB input must be one packed, contiguous buffer of
bytes (the arrow binary-column layout); got one of 8-byte items (format 'd')
>>> ... int32 ...
... got one of 4-byte items (format 'i')
So the packed path and the scalar path now refuse the same object, and the arrow-column caller gets that instead of a WKB parse error at some blob index.
The int64 coercion is wrapped, so the documented "every refusal is a catchable ValueError" holds:
>>> mortie.from_wkb(packed, offsets=[0, 10**19, len(packed)], order=6)
ValueError: offsets must fit in int64 (arrow list offsets)
Tests in test_from_wkb_packed_column_layout_errors: float64 and int32 buffers both raise TypeError matching got one of \d+-byte items, and the past-int64 offset raises ValueError, not OverflowError.
On float offsets — left as designed (it is the batch family's np.asarray(..., dtype=np.int64) convention, and changing it here alone would split the family), but the offsets parameter now says so explicitly: "Coerced with np.asarray(..., dtype=np.int64) as the rest of the batch family is, so float offsets truncate toward zero." The Raises section gained the int64 ValueError and now says the TypeError is the same by-item-size refusal the scalar path makes.
| with the plural batch names), the MOC coverer's only entry point. The plural | ||
| *MOCs* is the contract: many→many, one MOC per input ring — as against the | ||
| many→**one** union of a multipart ring-set, which is covered through | ||
| `from_geometry` / `from_wkb` / `from_wkt` with `moc=True` (or `mortie.Moc`). |
There was a problem hiding this comment.
🤖 from Claude (review)
This replacement list is not reachable from a numpy-only install, and the one route that is has no order knob — so "multipart/hole MOC at a chosen order" has no public spelling after the retirement.
The same sentence appears in CHANGELOG.md:27, USAGE.md:332 and mortie/batch.py. Enumerating the four routes it names, against what the retired morton_coverage_moc(rings_lats, rings_lons, order=8) did:
| route | multipart/holes | order knob |
numpy-only |
|---|---|---|---|
polygons_to_morton_mocs |
✗ (one MOC per ring) | ✓ | ✓ |
from_geometry(..., moc=True) |
✓ | ✓ | ✗ needs shapely/spherely |
from_wkt(..., moc=True) |
✓ | ✓ | ✗ needs a backend |
from_wkb(..., moc=True) |
✓ | ✓ | ✓ if you already have a blob |
mortie.Moc / Moc.from_polygon |
✓ | ✗ | ✓ |
Measured:
>>> mortie.Moc.from_polygon(rings_lats, rings_lons) # numpy-only, but no order=
<495568 words> # the order-18 default
>>> mortie.Moc.from_polygon(rings_lats, rings_lons, order=8)
TypeError: Moc.from_polygon() got an unexpected keyword argument 'order'
>>> mortie.polygons_to_morton_mocs(np.concatenate(rings_lats), np.concatenate(rings_lons), [0, 4, 8], order=8)[1]
[0, 339, 411] # two separate MOCs, not a donutThe old one-line call produced 411 words; the surviving numpy-only route produces 495,568 — 1,200× larger, because it is a different order. So a numpy-only caller who wants a hole-carved MOC at order 8 has to hand-encode a WKB blob to get back what morton_coverage_moc gave directly.
That matters more here than it would elsewhere because this very page sells the numpy-only property two paragraphs down ("mortie parses the bytes itself, so no geometry backend is involved"), and the module docstring for from_wkb leads with "no backend needed".
The PR body raises the shape of this as question 7 but frames it as "no public array-in spelling", without the numpy-only angle or the Moc-has-no-order gap, and the docs present the list as a complete replacement. Two ways out, both small:
(a) add order= to Moc / Moc.from_polygon (it already forwards tolerance / max_cells / latitude to the same kernel, so this is a passthrough), which makes the numpy-only route complete and lets the docs stand as written; or
(b) take the ruled escape hatch — a thin public multipart wrapper over the batch — and keep morton_coverage_moc's ring-set signature under a new name.
If neither lands in this PR, the sentence should say plainly which routes need a geometry backend and that Moc covers at the default order only, so a reader does not discover it at the call site.
There was a problem hiding this comment.
🤖 from Claude
Prose corrected in e2e0270 (and the CHANGELOG half in 3ffe81f); the API gap itself stays standing for @espg.
Your options (a) order= on Moc and (b) a public multipart wrapper are both new public surface, which is a scope decision, not a review fold — and it is already the PR body's open question 7. So I did what the last paragraph of your comment asks: made the docs say plainly which route needs what, instead of presenting the list as a complete replacement.
docs/coverage_methods.md now enumerates the union routes as a list rather than a run-on:
from_wkb(blob, moc=True, order=...)— the numpy-only route, when the geometry is already WKB bytes.from_geometry/from_wktwithmoc=True— same result at a chosen order, but both need a geometry backend.mortie.Moc/Moc.from_polygon— numpy-only, from GeoJSON or a list of ring arrays, but noorder: it covers at the default finest order (tolerance/max_cellsare its only knobs).
and then states the gap outright: "a numpy-only caller who holds ring arrays and wants a hole-carved MOC at a chosen order has no one-line spelling today — that gap is issue #187's open question 7."
The same correction went everywhere the claim repeats: USAGE.md (the polygons_to_morton_mocs paragraph), README.md:125 ("only from_wkb (bytes in) and Moc are backend-free, and Moc has no order knob"), mortie/batch.py's polygons_to_morton_mocs docstring (which named from_geometry first without saying it needs a backend), and CHANGELOG.md's note paragraph.
There was a problem hiding this comment.
🤖 from Claude
Ruled (2026-08-24, review question 7): defer — no public array-in multipart-MOC spelling is added now. The thin scalar convenience over the batch remains the additive post-1.0 escape hatch if a consumer appears; none exists today — zagg's fallback uses only the single-ring form, and its migration onto the surviving names is tracked as englacial/zagg#513. The kernel stays private (mortie.coverage._morton_coverage_moc), the documented routes stand as swept. Recorded as RESOLVED in the PR body.
| """Batch MOC coverage over an Arrow WKB column (issue #163). | ||
|
|
||
| The Arrow skin of :func:`mortie.from_wkbs`: a geoparquet / STAC geometry | ||
| The Arrow skin of :func:`mortie.from_wkb`'s batch form (renamed from |
There was a problem hiding this comment.
🤖 from Claude (review)
Docs sweep miss: the sibling function in this same file still names mortie.morton_coverage_moc twice, and it renders on the published API page.
from_wkb here was swept, but mortie.arrow.polygons_to_morton_mocs (lines 367 and 391) was not:
mortie/arrow.py:367: caller (and covered with :func:`mortie.morton_coverage_moc`'s
mortie/arrow.py:391: :func:`mortie.morton_coverage_moc` on that ring. A
Both are in the public docstring of a member listed in docs/api/arrow.md, so they survive into the rendered site. I built the docs from this commit (mkdocs build --strict — green, as the PR body says) and grepped the output:
site/api/arrow/index.html:1378: caller (and covered with :func:<code>mortie.morton_coverage_moc</code>'s
site/api/arrow/index.html:1497: :func:<code>mortie.morton_coverage_moc</code> on that ring. A
Line 367 is worse than a stale cross-reference — it is actionable advice pointing at a function that no longer exists ("decompose such a footprint yourself and cover it with morton_coverage_moc's list-of-rings form"). Line 391 is a byte-identity claim against a name a reader cannot call. mortie/batch.py got exactly this fix (its equivalent lines now say "the retired scalar morton_coverage_moc" / point at from_geometry); arrow.py did not.
--strict does not catch it because mkdocstrings renders :func: roles as literal text rather than resolving them, so a dangling reference is invisible to the build. Per the phase-4 ruling ("a plural surviving in rendered docs after removal is a defect"), this is one.
One more, non-rendered but same sweep: mortie/coverage.py:39 still says the threshold is "the pre-emptive refusal in mortie._moc.moc_to_order / mortie.batch.mocs_to_orders".
There was a problem hiding this comment.
🤖 from Claude
Fixed in b2fffc8 — both arrow.py sites and the coverage.py one.
arrow.py:367 was the worse of the two, as you say: actionable advice pointing at a function that no longer exists. It now points at the surviving multipart routes and notes the Moc caveat, which is the same treatment batch.py got:
caller (and, if the union is what is wanted, covered through a multipart route instead:
mortie.from_wkb/mortie.from_geometrywithmoc=True, ormortie.Moc's list-of-rings form, which takes noorder).
arrow.py:391's byte-identity claim is now made against a callable name: "byte-identical to mortie.polygons_to_morton_mocs on that ring alone (the identity the retired scalar morton_coverage_moc used to pin)" — the retired name survives only as history, never as a :func: role a reader might try to call.
coverage.py:39 now reads "mortie._moc.moc_to_order (and in its batch kernel, the private mortie.batch._mocs_to_orders)".
Rebuilt the docs to confirm: grep morton_coverage_moc site/api/arrow/index.html is down to the one "the retired scalar morton_coverage_moc" history mention plus its source listing, and grep -rn ":func:\mortie.morton_coverage_moc`" mortie/ docs/ *.md(excluding tests) is now empty repo-wide. Noted your point that--strict` cannot see this class of defect — the grep is what I checked against, not the build.
|
|
||
| Coverage is **multi-order by default** — coarse cells inside, fine cells | ||
| along the boundary, down to :func:`~mortie.morton_coverage_moc`'s default | ||
| along the boundary, down to :func:`~mortie.coverage._morton_coverage_moc`'s default |
There was a problem hiding this comment.
🤖 from Claude (review)
The public Moc API page now sends readers to a private function seven times.
The rename was applied mechanically, so Moc's class docstring, its Parameters, its Raises, its See Also, and Moc.from_polygon all now point at mortie.coverage._morton_coverage_moc — lines 367, 396, 399, 410, 417, 474 here, plus 131 / 199 / 202 / 275 in the module's helpers. Moc is a rendered public member (docs/api/moc_object.md), so these publish:
site/api/moc_object/index.html:1720: along the boundary, down to :func:<code>~mortie.coverage._morton_coverage_moc</code>'s default
site/api/moc_object/index.html:1780: drops to this value; see :func:<code>~mortie.coverage._morton_coverage_moc</code>.
site/api/moc_object/index.html:1843: rings (see :func:<code>~mortie.coverage._morton_coverage_moc</code>), or a coverage knob
site/api/moc_object/index.html:1856: mortie.coverage._morton_coverage_moc : the coverage kernel this wraps.
...
A See Also entry naming a leading-underscore function is a dead end for a reader — mkdocstrings does not render it (it is excluded from the API pages by [tool.numpydoc_validation]'s private-member exclusion and by docs/api/coverage.md, which correctly dropped the member), and the CHANGELOG explicitly says "private names carry no compatibility promise". So the public documentation for the object layer is now anchored to something the docs deliberately do not document and the project reserves the right to change.
mortie/batch.py and mortie/coverage.py handled the same problem the other way — they describe the coverer by what it is and point at the surviving public routes. The same treatment reads better here, e.g. line 367:
Coverage is multi-order by default — coarse cells inside, fine cells along the boundary, down to the MOC coverer's default finest order
and the See Also pointing at mortie.from_geometry / mortie.polygons_to_morton_mocs rather than the kernel. The import at line 54 and the two call sites (348, 495) are of course fine as they are — this is only about the prose.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 1c2be68 — prose only, no code touched, as you scoped it.
All six public-facing references are gone; the rendered docs/api/moc_object.md now carries exactly one mention of the private name, in the See Also parenthetical where it is honest commentary rather than a destination:
- 367 → "down to the MOC coverer's default finest order"
- 396 / 399 (
tolerance/max_cells) → "the coverage kernel's angular stop criterion, as onmortie.polygons_to_morton_mocs" / "as onmortie.polygons_to_morton_mocs" - 410 (
Raises) → "the coverage kernel rejects the rings" - 417 (
See Also) →mortie.polygons_to_morton_mocs(the public coverer, withorder) andmortie.from_geometry, with the kernel named once parenthetically: "this class wraps the same coverage kernel (mortie.coverage._morton_coverage_moc, private)" - 474 (
from_polygon) → "resolved by the coverage kernel's one even-odd descent: disjoint parts union, a nested ring carves a hole" — which also says what the rule is, rather than deferring to a page that does not render
The four references in private helpers (131, 199, 202, 275, in _geojson_ring_groups / _ring_latlons / _reject_coverage_knobs) are left as-is: internal commentary between private functions, not published.
Rebuilt the docs: site/api/moc_object/index.html is down from seven prose hits to the one parenthetical, plus the two syntax-highlighted source listings of the import and call sites, which are unavoidable and correct.
| crosses the Python/Rust boundary **once**, the GIL is released for the | ||
| batch, and Rust parallelizes across parents. Row ``i`` is bit-identical to | ||
| :func:`generate_morton_children` on ``words[i]`` alone. | ||
| :func:`mortie.generate_morton_children` on ``words[i]`` alone. |
There was a problem hiding this comment.
🤖 from Claude (review)
This line lost its docstring indentation — it starts at column 0.
$ grep -n "^:func:" mortie/batch.py
17::func:`polygons_to_morton_mocs`, whose ragged batch-native signature has no
28::func:`mortie.arrow.polygons_to_morton_mocs` take pyarrow columns and stay in
768::func:`mortie.generate_morton_children` on ``words[i]`` alone.
17 and 28 are module-docstring continuations and are fine. 768 is inside _children_of's function docstring, whose body is indented four spaces, so inspect.cleandoc / numpydoc see a dedented line mid-paragraph:
batch, and Rust parallelizes across parents. Row ``i`` is bit-identical to
:func:`mortie.generate_morton_children` on ``words[i]`` alone.
The preceding line was rewrapped in this commit (The dense batch kernel of :func:mortie.generate_morton_children (issue / #156): the whole parent array) and the continuation was left behind. numpydoc lint does not catch it (private members are excluded), which is why it is still here.
While the paragraph is being rewrapped: line 277 in this file is now 100 characters —
mortie/batch.py:277: (100) the scalar form of :func:`mortie.from_wkb` for those. Also for ``order`` outside 1-29, both
— which is new in this commit. ruff is configured at line-length = 88 and the PR gate ignores E501, so nothing fails, but the PR body calls out the one pre-existing 100-char line as deliberately left alone; this is a second one added rather than inherited.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 2716d0f — a pure formatting commit, no prose changed.
The _children_of paragraph is rewrapped whole, so the continuation is back inside the docstring body:
The dense batch kernel of :func:`mortie.generate_morton_children` (issue
#156): the whole parent array crosses the Python/Rust boundary **once**,
the GIL is released for the batch, and Rust parallelizes across parents.
Row ``i`` is bit-identical to :func:`mortie.generate_morton_children` on
``words[i]`` alone.
grep -n "^:func:" mortie/batch.py is now just 17 and 28 — the two module-docstring continuations, which are fine.
Line 277 is rewrapped too. While there I checked the whole file against the parent commit rather than only that line: awk 'length > 88' on dd49f02^:mortie/batch.py reports nothing, so the 91- and 92-char lines at what are now _mocs_to_orders and _common_ancestors were also added by this commit, not inherited. Both are rewrapped in the same commit; the only line over 88 left in the file is a doctest +ELLIPSIS output line, which cannot be wrapped without breaking the doctest.
| calls = [] | ||
| real = batch._from_wkbs | ||
|
|
||
| def spy(entries, **kwargs): |
There was a problem hiding this comment.
🤖 from Claude (review)
The delegation spy proves the route but not the payload — **kwargs is swallowed and never asserted on.
def spy(entries, **kwargs):
calls.append(list(entries))
return real(entries, **kwargs)calls records only the entries, so the test's two assertions (len(calls) == 2, and that the packed form hands over memoryviews) hold no matter what _from_wkb_batch forwards. Combined with the absence of any tolerance= / max_cells= parity test over the polymorphic batch, this is why the suite still passes with both parameters dropped from the delegation (measured — see the comment on mortie/geometry.py:483).
Capturing the kwargs costs one line and turns this into the pin the PR body describes:
def spy(entries, **kwargs):
calls.append((list(entries), kwargs))
return real(entries, **kwargs)
...
assert [k for _, k in calls] == [
dict(order=6, tolerance=None, max_cells=None, normalize=True,
latitude="authalic")
] * 2Two smaller test-quality notes in this block:
test_from_wkb_offsets_is_keyword_only(line 551) assertspytest.raises((TypeError, ValueError))with nomatch=. Every other refusal in this file is pinned to its message text and anchored; this one passes on any error, including one raised for an unrelated reason.match=r"positional arguments"would pin what it means to test.test_generate_morton_children_refusals_name_the_survivorcovers two of the three kernel messages that carry the retired name. The third —word {i}: is at order {o} but word 0 is at order {p}; children_of returns a dense (n, 4**d) block— goes through the samestr.replaceand is not pinned publicly. I verified it does come out respelled (... ; generate_morton_children returns a dense (n, 4**d) block ...), so this is a missing pin rather than a bug.
There was a problem hiding this comment.
🤖 from Claude
All three fixed, across two commits.
The spy — 77f9e79 (folded with the blocking gap you raised on mortie/geometry.py:483, since they are the same root cause). It captures (list(entries), kwargs) and asserts the exact forwarded dict for both forms, and the calls it makes now carry non-default knobs so the assertion has something to discriminate:
assert [kw for _, kw in calls] == [
dict(order=6, tolerance=2.0, max_cells=None, normalize=False,
latitude="geodetic-spherical")
] * 2test_from_wkb_offsets_is_keyword_only — 1b3ac1d. Now pytest.raises(TypeError, match=r"takes from 1 to 6 positional arguments but 7"), which is the message Python actually raises for the keyword-only slot, so the test fails if offsets ever stops being keyword-only rather than passing on any error.
The third kernel message — 1b3ac1d, added to test_generate_morton_children_refusals_name_the_survivor:
with pytest.raises(
ValueError,
match=r"generate_morton_children returns a dense \(n, 4\*\*d\) block",
):
mortie.generate_morton_children(mixed, 8)with mixed two words at orders 4 and 5. Confirmed live it comes out respelled, as you said: word 1: is at order 5 but word 0 is at order 4; generate_morton_children returns a dense (n, 4**d) block, so every parent must sit at one order.
| | `decimals_to_words(arr)` | `decimal_to_word(arr)` (array in, array out) | | ||
| | `children_of(words, order, max_cells)` | `generate_morton_children(words, order, max_cells=max_cells)` | | ||
| | `from_wkbs(blobs, ...)` | `from_wkb(blobs, ...)` — see below | | ||
| | `morton_coverage_moc(lats, lons, ...)` | `polygons_to_morton_mocs(lats, lons, [0, len(lats)], ...)` for one ring; `from_geometry` / `from_wkb` / `from_wkt` with `moc=True` (or `mortie.Moc`) for multipart/holes | |
There was a problem hiding this comment.
🤖 from Claude (review)
Minor: this row's "call instead" changes the return shape, and the row does not say so.
morton_coverage_moc returned a bare uint64 array; polygons_to_morton_mocs returns a (values, out_offsets) pair. A caller who applies this row literally gets a tuple bound where an array was:
moc = mortie.polygons_to_morton_mocs(lats, lons, [0, len(lats)], order=10)
len(moc) # 2, not the cell countEvery other row in the table is a drop-in respelling, so this one reads like one too. polygons_to_morton_mocs(lats, lons, [0, len(lats)], ...)[0] — or values, _ = ... as README.md and USAGE.md correctly spell it — would make the row self-contained.
I ran every row in this table against the branch. All nine migrations produce byte-identical results to their retired originals (including this one once unpacked, with tolerance / max_cells / normalize variants, and mortie.arrow.from_wkb), so the table's semantics are right — this is purely about the spelling of the one row whose arity changed.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 3ffe81f — the row unpacks now, as README.md / USAGE.md already did:
values, _ = polygons_to_morton_mocs(lats, lons, [0, len(lats)], ...)for one ring — the batch-native call returns the ragged(values, out_offsets)pair, so the one ring's MOC isvalues, not the return itself; for multipart/holes see the note below
Two things fell out of rewriting it. The multipart half was crowding the cell and was also the sentence flagged as inaccurate on docs/coverage_methods.md:43 (numpy-only reachability, Moc having no order), so it moved down into the note paragraph where there is room to say which route needs a geometry backend and which has no order knob. And the row is now the only one in the table that is not a bare drop-in, which is the point — the values, _ = prefix makes the arity change visible at a glance rather than in prose.
|
🤖 from Claude (review) Fresh-context adversarial review of 10 inline comments posted. Ranked findings
None of (1)–(10) is a wrong answer from correctly-spelled code; (1) is a gap that lets one become possible, and (3) is the one path where a plausible caller gets a wrong answer silently. What I checked and found correctRetirement completeness. All nine retired names are absent from the package root, from
Byte identity. Scalar default ≡ explicit Error-message rewrites. Both CHANGELOG migration table. I ran all ten rows. Every one produces a byte-identical result to its retired original, including the Docs that must run. Executed the edited
Rewired test files. Read the diffs for Gates re-run on
|
| gate | result |
|---|---|
python -m pytest -q --no-cov |
1695 passed, 16 skipped in 116 s |
flake8 mortie --select=E9,F63,F7,F82 |
clean |
numpydoc lint mortie/*.py |
clean |
ruff check --select=E,F,W,I --ignore=E501 mortie/ |
9 errors — byte-identical to the parent 2de31ed, so zero introduced |
mkdocs build --strict |
green (as claimed). Note it cannot catch finding (6): mkdocstrings renders :func: roles as literal text, so a dangling cross-reference is invisible to --strict |
The PR body reports "1576 passed" at 6cb2a16; the count is now 1695 (phase 4's new tests plus the 2de31ed merge of #197), worth updating before ready-for-review.
Conventions
- Commit message —
phase 4 of issue #187, title-only, matchesgit log. ✅ - Module size —
batch.py899 (was 886),geometry.py818 (was 651). Both under mortie'sCLAUDE.md~1000-line aim;geometry.py's +167 is the largest single-file growth in the diff but leaves headroom. ✅ - Lint / types / docstrings — no new hits on any gate. ✅
- Notebook churn (nit, no comment posted) — both notebooks were re-serialized with
\uXXXXescapes, so ~33 em-dashes and other non-ASCII characters in unrelated cells show as changed lines.morton_set_algebra.ipynbis 21 added / 21 deleted lines of which one is a real edit (morton_coverage_moc→polygons_to_morton_mocsin the closing summary). Re-saving withensure_ascii=Falsewould keep future notebook diffs readable.
Generated by Claude Code
| # Tracks the package floor in pyproject.toml (issue #187): mortie's word | ||
| # scalars need NEP 50. The pinned mortie release below has been numpy-2 | ||
| # compatible since 0.5.2, so this floor costs the notebooks nothing. | ||
| - numpy>=2 |
There was a problem hiding this comment.
🤖 from Claude (review)
(Low) The bump itself is safe — I verified it — but this is not the only conda env in the repo. The root environment.yml still declares a bare, unpinned numpy and is the one declaration in the tree that still permits numpy 1.
environment.yml at the repo root (the conda dev env), unchanged by this commit:
dependencies:
- flake8
- numpy # <- no floor
- pytest
- pytest-covIn practice conda-forge solves that to numpy 2 today and a subsequent pip install -e . would enforce the floor anyway, so this is a consistency defect rather than a live breakage — but the CHANGELOG entry says "binder/environment.yml tracks it" and the sibling env is the obvious place a reader would look next. Making it - numpy>=2 costs nothing.
What I did verify about this file:
- The
0.5.2claim is right.CHANGELOG.md:489under## [0.5.2] - 2025-12-10is "update for numpy 2 compat (update for numpy 2 compat #13)", and 0.8.2 postdates it. mortie==0.8.2really does run under numpy 2, empirically, not just by declaration:$ pip install "mortie==0.8.2" "numpy>=2" && python -c " import numpy as np, mortie print('numpy', np.__version__, 'mortie', mortie.__version__) print(mortie.geo2mort(np.array([45.0]), np.array([10.0]), 8))" numpy 2.2.6 mortie 0.8.2 [1929335043491102728]- The resolution cannot conflict. 0.8.2's PyPI metadata declares
numpy>=1.20with no upper cap, sopip install mortie==0.8.2will not touch a numpy 2 that conda already placed:$ curl -s https://pypi.org/pypi/mortie/0.8.2/json | jq -r '.info.requires_dist[0], .info.requires_python' numpy>=1.20 >=3.10 python=3.11+numpy>=2is satisfiable on conda-forge, and no other pin in this file caps numpy (thehealpy>=1.16floor is a floor, so the solver is free to take a numpy-2-compatible healpy).
There was a problem hiding this comment.
🤖 from Claude
Fixed in 5f07103. The root environment.yml now carries the floor, with the same pointer comment the binder env has:
dependencies:
- flake8
# Tracks the package floor in pyproject.toml (issue #187): mortie's word
# scalars need NEP 50.
- numpy>=2
- pytestAnd the CHANGELOG claim you caught is now accurate for both rather than naming one: "Both conda envs in the tree track it — the dev environment.yml and binder/environment.yml."
I swept for any third declaration so the claim is exhaustive, not just less wrong — git ls-files for env/requirements files returns exactly those two, and the only other tracked numpy mention outside .github/workflows/ is mkdocs.yml:37 docstring_style: numpy, which is unrelated. Both files parse to numpy>=2:
environment.yml -> ['numpy>=2']
binder/environment.yml -> ['numpy>=2']
Thanks for the verification legwork on the binder side — the 0.5.2 provenance, the empirical mortie==0.8.2 under numpy 2, and the requires_dist check that the pip step cannot pull conda's numpy back down. Nothing in it needed changing.
| means one type on every entry point. Mind that ``uint64`` arithmetic is | ||
| not Python's: under NEP 50 a word mixed with a Python ``int`` stays | ||
| ``uint64``, so it **wraps at 2**64** instead of promoting to a big | ||
| integer (the wrap raises a ``RuntimeWarning`` first, so ``-W |
There was a problem hiding this comment.
🤖 from Claude (review)
(Low) The simplified prose keeps a claim that is over-broad: the wrap does not always raise a RuntimeWarning first — shifts truncate silently.
Flagging this because phase 7 is the docstring-honesty commit and it re-committed the sentence into the new paragraph; the substance was inherited from phase 6, so treat it as a note rather than a phase-7 regression. The paragraph is specifically encouraging bit-packed-word operations, which makes the shift case the likely one.
On the installed numpy 2.2.2:
$ python -c "
import warnings, numpy as np
def probe(label, fn):
with warnings.catch_warnings(record=True) as rec:
warnings.simplefilter('always'); r = fn()
print(f'{label}: {r!r} warnings={[w.category.__name__ for w in rec]}')
mx = np.uint64(2**64-1)
probe('max + 1', lambda: mx + 1)
probe('uint64(0) - 1', lambda: np.uint64(0) - 1)
probe('max * 2', lambda: mx * 2)
probe('max << 1', lambda: mx << np.uint64(1))
"
max + 1: np.uint64(0) warnings=['RuntimeWarning']
uint64(0) - 1: np.uint64(18446744073709551615) warnings=['RuntimeWarning']
max * 2: np.uint64(18446744073709551614) warnings=['RuntimeWarning']
max << 1: np.uint64(18446744073709551614) warnings=[] <-- silent
So -W error::RuntimeWarning catches the + - * wraps but not a left shift off the top of the word. Scoping the parenthetical to arithmetic ("the arithmetic wrap raises a RuntimeWarning first…") would make it exact. CHANGELOG.md:107-111 carries the same generalization (phase 6's entry, out of scope for this commit).
Everything else in the rewritten paragraphs I verified on numpy 2.2.2 and it is accurate:
$ python -c "
import numpy as np, mortie
w = mortie.time2toc(10**9)
print('w + 1 ->', repr(w + 1)) # np.uint64(3147483649)
print('w + 1.0 ->', (w + 1.0).dtype) # float64
print('int(w) ->', int(w), type(int(w))) # 3147483648 <class 'int'>
"
$ python -W error::RuntimeWarning -c "import numpy as np; np.uint64(2**64-1) + 1"
RuntimeWarning: overflow encountered in scalar add
I also grepped the rest of the tree for surviving numpy-1 hedges — mortie/*.py, docs/, README.md, USAGE.md, BUILDING.md — for "numpy < 2", "numpy >= 2", "NEP 50", "float64", "1.20". Nothing else hedges across numpy generations; the only other NEP 50 mentions are mortie/convert.py:567 and mortie/tests/test_convert.py:271, both stating numpy-2 behaviour flatly. This docstring was the only hedge, and it is the only place carrying the word-type note.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 238e78b, scoped to arithmetic as you suggested:
integer (an *arithmetic* wrap raises a ``RuntimeWarning`` first, so ``-W
error::RuntimeWarning`` turns it into a failure; a bit shift off the top
of the word truncates silently, with no warning to catch), and mixing
with a Python ``float`` gives ``float64``.
I reproduced your probe on numpy 2.2.2 before editing, and extended it to the Python-int shift spelling since that is the one a call site would actually write — it is silent too:
max + 1 np.uint64(0) warnings=['RuntimeWarning']
uint64(0) - 1 np.uint64(18446744073709551615) warnings=['RuntimeWarning']
max * 2 np.uint64(18446744073709551614) warnings=['RuntimeWarning']
max << 1 np.uint64(18446744073709551614) warnings=[] <-- silent
max << 1 (int) np.uint64(18446744073709551614) warnings=[] <-- silent
I also fixed the same generalization at CHANGELOG.md:97-101 in that commit, even though you scoped it out as phase 6's. It is the identical untrue claim in the more widely read of the two places, the fix is one sentence, and leaving the two texts disagreeing about the same behaviour would be worse than the small scope stretch. It now reads "...but a bit shift off the top of the word (w << 1) truncates silently, with no warning to catch." Flagging it explicitly so it is visible rather than folded in quietly — happy to split it back out if you would rather it rode with a phase-6 fix.
Thanks for the tree-wide sweep confirming this docstring was the only surviving numpy-1 hedge; numpydoc lint mortie/*.py is clean after the edit.
|
🤖 from Claude (review) Phase 7 adversarial review —
|
| # | Sev | Where | Finding |
|---|---|---|---|
| 1 | High | mortie/tests/test_polymorphic_api.py:997 |
test_word_arithmetic_is_exact_near_the_top_of_the_range passes on numpy 1.26.4. big + np.uint64(1) is uint64 on numpy 1 too, and line 999 is pure float64. big + 1 is the discriminating spelling. |
| 2 | Medium | …test_polymorphic_api.py:984 |
word >> np.uint64(32) is not "against a Python int" as the comment two lines up claims; >> is the one bitwise op left uncovered. Test still fails on numpy 1 via word | 1, so no false pass — a coverage/claim gap. |
| 3 | Medium | …test_polymorphic_api.py:972 |
test_numpy_is_at_or_above_the_declared_floor asserts the runtime numpy, never the declaration. Reverting pyproject.toml to numpy>=1.20 leaves it green (reproduced). importlib.metadata is not a usable fix here — stale editable metadata still reports numpy>=1.20; read pyproject.toml instead. |
| 4 | Med-low | CHANGELOG.md:85 |
"the np.uint64 word semantics above" — that entry is at line 95, below this one. Swap the entries or say "below". |
| 5 | Low | binder/environment.yml:18 (about root environment.yml) |
The root conda dev env still declares a bare, unpinned numpy — the one declaration in the tree that still permits numpy 1, while the entry says "binder/environment.yml tracks it". |
| 6 | Low | mortie/_toc.py:129 |
"the wrap raises a RuntimeWarning first" is over-broad: max << 1 truncates silently. Inherited from phase 6 but re-committed in the simplified prose; scope the parenthetical to arithmetic. |
Independent floor-declaration sweep
I swept the whole tree for anywhere a numpy version is declared, pinned, implied or documented — every pyproject.toml section, binder/, all five workflows, the root conda env, Cargo.toml, README/USAGE/BUILDING/docs/**, notebooks, and uv.lock.
| File / location | What it declares | Verdict |
|---|---|---|
pyproject.toml [project].dependencies |
numpy>=2 |
✅ bumped, with rationale |
pyproject.toml requires-python |
>=3.10 |
✅ coherent — numpy 2.0/2.1/2.2 all support 3.10 (2.2.6 requires_python: >=3.10); 2.3+ needs 3.11. Every Python in every matrix (3.10–3.13) has a numpy 2 release. Not unsatisfiable. |
pyproject.toml extras pandas / pyarrow / arro3 / test / bench / examples |
no numpy pin, direct or transitive-capping | ✅ nothing permits or forces numpy 1 |
pyproject.toml [dependency-groups].docs |
mkdocs stack, no numpy | ✅ n/a |
pyproject.toml [build-system].requires |
maturin>=1.0,<2.0 — no numpy build pin |
✅ n/a (maturin/pyo3 build, no oldest-supported-numpy) |
pyproject.toml [tool.maturin] |
module/bindings config only | ✅ n/a |
binder/environment.yml |
numpy>=2 |
✅ bumped; safe (see below) |
binder/postBuild |
mortie==0.8.2 (declares numpy>=1.20, no cap) |
✅ resolves cleanly against numpy 2 and runs on it — verified |
environment.yml (root, conda dev env) |
bare numpy |
|
.github/workflows/test.yml |
pip install -e ".[test]", unpinned |
✅ inherits >=2 |
.github/workflows/build-wheels.yml:410 |
pip install numpy then --no-deps wheel install |
--no-deps means the declared floor is never enforced in that job, but the unpinned install lands numpy 2 on all of 3.10–3.13 |
.github/workflows/codspeed.yml / docs.yml / lint.yml |
pip install -e ".[bench]" / uv sync --group docs / numpydoc only |
✅ no numpy pin |
Cargo.toml:16 |
numpy = "0.22" (rust-numpy crate) |
✅ unrelated to the Python floor; rust-numpy 0.22 links both generations — the built .so imported fine under numpy 1.26.4 in my scratch env |
uv.lock |
already carries { name = "numpy", specifier = ">=2" } |
✅ and it is .gitignored (.gitignore:38), so not a repo declaration |
README.md, USAGE.md, docs/index.md, docs/** |
prose "numpy is the only runtime dependency", no version | ✅ no stale floor |
BUILDING.md:10 |
"Python packages: numpy" (no version) | ➖ no contradiction; a >=2 here would be a courtesy, not a fix |
examples/*.ipynb |
no pip install, no numpy pin |
✅ n/a |
grep -rn "1\.20" over the tree returns only the two deliberate historical references (pyproject.toml:27, CHANGELOG.md:88) plus unrelated holoviews>=1.20 / healpy-1.20 comments. Answer to "did phase 7 miss any declaration": one — the root environment.yml.
Checked and found correct
- Every remaining docstring claim in
_toc.py, on numpy 2.2.2: wrap at 2**64 (np.uint64(2**64-1) + 1 -> np.uint64(0)),RuntimeWarningemitted before the arithmetic wrap,python -W error::RuntimeWarningreally turning it intoRuntimeWarning: overflow encountered in scalar add,w + 1.0 -> float64, andint(w)returning the unbounded Python int. Only the shift caveat (finding 6) is unstated. - No surviving numpy-1 hedge anywhere else. Grepped
mortie/*.py,docs/,*.mdfor "numpy < 2", "numpy >= 2", "NEP 50", "float64", "1.20". The only other NEP 50 mentions (mortie/convert.py:567,mortie/tests/test_convert.py:271) state numpy-2 behaviour flatly. This was the only hedge and the only copy of the word-type note. - The binder bump is safe — verified three ways, not just by declaration: the
0.5.2numpy-2-compat claim matchesCHANGELOG.md:489; 0.8.2's PyPI metadata isnumpy>=1.20with no cap; andpip install mortie==0.8.2 "numpy>=2"+geo2mortactually runs (numpy 2.2.6,[1929335043491102728]). - The test-block comment's claim "nothing else in the suite would notice a downgrade" is true, and I would not have believed it without checking. Full suite on a scratch copy of
4edc7b2under numpy 1.26.4:1813 other tests pass on numpy 1. That is the justification for adding these tests at all — and it is also why finding 1 matters: with only two of the three discriminating, the exactness test is decorative.2 failed, 1813 passed, 40 skipped in 155.63s FAILED ...::test_numpy_is_at_or_above_the_declared_floor FAILED ...::test_word_arithmetic_stays_uint64_under_nep50 - Version-string parsing in the floor test:
int(np.__version__.split(".")[0])survives"2.0.0rc1","2.3.0.dev0+git…","2.2.6". No defect there — the problem is what it asserts (finding 3), not how it parses.
Gates
| Gate | Result |
|---|---|
pytest -q --no-cov -p no:cacheprovider @ 4edc7b2 |
1845 passed, 16 skipped (115.8s) — +3 over the 1842 baseline at 28721fe, exactly the three new tests |
flake8 mortie --select=E9,F63,F7,F82 |
clean (exit 0) |
ruff check --select=E,F,W,I --ignore=E501 on mortie/_toc.py, mortie/tests/test_polymorphic_api.py |
All checks passed |
ruff check (full repo config, E,F,W,I,D + numpy pydocstyle) on the two changed .py files |
All checks passed |
numpydoc lint mortie/_toc.py |
clean (exit 0) |
Conventions
- Commit message: title-only,
phase 7 of issue #187— matches the repo style. ✅ - CI/deploy config: no
.github/workflows/**file was touched. The diff ispyproject.toml,binder/environment.yml,mortie/_toc.py,mortie/tests/test_polymorphic_api.py,CHANGELOG.md.binder/is a repo2docker notebook runtime, not CI/CD, and the floor bump is espg-ruled, so the right files were touched — with the rootenvironment.ymlomission noted above. ✅ - Tests with the behavioral change: present, but see findings 1–3 on whether they hold.
⚠️ - Module size:
mortie/_toc.pynet −8 lines; no cap concern. ✅
Generated by Claude Code
|
🤖 from Claude Ready for review. Your ruling on the numpy floor landed as phase 7, the review/fold cycle ran on it, Phase 7 —
|
|
🤖 from Claude Cleared the outstanding review-thread backlog on this PR. One commit: cc80347. No phase was added, no scope changed, and the PR stays Ruff botThree reviewdog findings from 2026-08-19 on e0a940a were unanswered. All three are fixed in the docstrings themselves — no
Reproduced each before the fix and re-checked after, with the repo's own config ( Both files are in this PR's diff only because Not fixed, flagged instead (CLAUDE.md §4 — pre-existing, outside the diff context, never reported by the bot): One process note: replies to the two The 17 unanswered review-bot threadsThese were the 2026-08-17 phase-1/phase-2 batches, most marked outdated. I re-read the code at head for each rather than trusting the outdated marker. Outcome: 1 still applied and is fixed here, 13 were already superseded by later commits, 3 are answered but deliberately left open because they carry a decision for you. Still applied → fixed in cc80347 — the test module docstring's claim that "the error surface passes through unchanged". Your four probes still reproduce at head: Superseded (13) — mostly by 6cb2a16, the phase-3 fold, plus later phases: the While replying I closed four small leftovers that the folds had not reached, all in cc80347: keyword-only enforcement is now parametrized across all five Left open for you (3) — each has a reply on its thread explaining why:
Local gatesNo Rust changed, so State
Generated by Claude Code |
|
🤖 from Claude espg's final rulings on the four standing review questions (in-session, 2026-08-24) are applied; the PR body records each as RESOLVED. Two commits:
The rulings of record:
Gates re-run in full on the merged result at All ten review questions are now ruled and recorded; the PR stays ready-for-review and |
|
🤖 from Claude Follow-up: main cut 0.9.11 ( |
2 participants
Refs #187.
Collapses the scalar/batch pairs to one polymorphic function per operation, per the leans ruled on the issue thread (#187 (comment)): the un-suffixed singular name survives, the input shape selects the form, and the docstring says whether the function is batch vectorized.
What this does
The plural twins in
mortie/batch.pyare the kernels — they already carry the ragged validation, the GIL release and the rayon fan-out, and their per-item results are byte-identical to the scalar. So the collapse is a delegation, not a reimplementation.Two shapes of pair, and they need different spellings:
(1) The item is itself an array (every MOC operation, and the toc reduce). A mortie MOC is a
uint64array, so the item and the column have the same rank and there is nothing forasarraycoercion to discriminate on. These take a keyword-onlyoffsets=:offsetsis exactly how the plural already spells the batch, so the collapsed signature is the scalar's signature plus the one argument that distinguishes the forms, both call sites keep their shapes, and the arrow list layout thatpolygons_to_morton_mocsemits andmocs_to_ordersconsumes stays zero-copy. Keyword-only, so it can never be reached positionally by code written against the current signature.(2) The item is a genuine scalar (
decimal_to_word,generate_morton_children). Here numpy semantics apply literally and the rank of the input is the discriminator — no new argument at all.Phase 0 — inventory and classification (the P0 table)
Every scalar/plural pair on the public surface (
mortie/__init__.py's__all__). In-repo reference counts only — this routine's GitHub access is scoped toespg/mortie, so the zagg/moczarr caller counts the P0 plan asked for are not in this table; they need a human pass or a wider scope.moc_to_ordermocs_to_ordersoffsets)moc_andmocs_andoffsets)moc_intersectsmocs_intersectoffsets)common_ancestor(aliasmoc_min)common_ancestorsoffsets)toc_reducetocs_reduceoffsets)decimal_to_worddecimals_to_wordsgenerate_morton_childrenchildren_of(n, 4**d)resultfrom_wkbfrom_wkbsoffsets=) / sequence — the ruled "(a+)" design, question (4)polygons_to_morton_mocsmorton_coverage_mocNot pairs, recorded so the table is exhaustive:
moc_or,moc_minus,moc_xor,moc_not— no batch kernel exists, so nothing to collapse. This is the gap PR Batch MOC set ops: mocs_and / mocs_intersect + scalar moc_intersects (issue #173) #174 deferred and Validation posture: batch family truncates/wraps where toc refuses — unify via #187 #194 routes back through this issue; it stays a separate question (an operation that does not batch, not a pair that needs collapsing). Phase 3's docstring marker now says so on each of them.morton_coverage,compress_moc,split_base_cells,clip2order,orders_of,validate_morton, theconvert.pyencoders — already array-in/array-out with no plural twin.mortie.arrow.from_wkbs/mortie.arrow.polygons_to_morton_mocs— the pyarrow skin is a third axis (issue Batch polygon coverage: polygons_to_morton_mocs (issue #153) #154) and is deliberately out of scope here.Phases
fa0c559) — the P0 table + the ragged MOC family:moc_to_order,moc_and,moc_intersects,common_ancestor/moc_min.d9352f3) — the scalar-item pairs and the toc reduce:toc_reduce,decimal_to_word,generate_morton_children.725807c) — the docstring sweep: 68 docstrings across 13 files now state whether the function is batch vectorized, which is the explicit ask on the issue thread ("we don't have full parity with all functions … so we need to note in the docstrings if the function is batch vectorized or not"). Three markers:**Batch vectorized**: array in, array out, elementwise.(43),**Not batch vectorized**: one <thing> per call.(18, and themoc_or/moc_minus/moc_xorones say there is no batch kernel behind them), and**Batch native**: reached polymorphically by …on the seven plural kernels. Two were deliberately left unmarked as ambiguous — see question (6).6cb2a16) — both adversarial reviews folded, including two blocking defects. Detail in API consolidation: one polymorphic function per operation (issue #187) #195 (comment).dd49f02) — the retirement and thefrom_wkbcollapse, per the 2026-08-19 rulings (recorded under "Questions for review" below). The plural names are removed outright — no shims, no aliases:mocs_to_orders,mocs_and,mocs_intersect,common_ancestors,tocs_reduce,decimals_to_words,children_of,from_wkbs, and the scalarmorton_coverage_moc(the batch-nativepolygons_to_morton_mocssurvives as the MOC coverer's only entry point). The kernels live on as private functions (_mocs_to_orders, …) behind the polymorphic entry points.from_wkbcollapses per the ruled "(a+)" design —offsets=⇒ packed column (zero-copy memoryview slices into the caller's buffer, fed to the existing byte-capped chunking kernel),list/tuple/object-ndarray ⇒ sequence batch, buffer spellings ⇒ one blob;mocis the ruled tri-state (defaultNone: scalar→flat, batch→MOC pair; explicitmoc=Falseon a batch raises naming the missing ragged-flat kernel).mortie.arrow.from_wkbsrenamed tomortie.arrow.from_wkb. Refusal messages that named a retired delegate now name the survivor (toc_reduce of an empty segment,generate_morton_children only refines) — rewritten at the Python delegation sites, no Rust change. Docs (docs/api/*,coverage_methods,benchmarks,index,morton_index_datatype,specification), README, USAGE, CHANGELOG (breaking, with a one-line-per-name migration table), both example notebooks, and thebenchmarks/scripts all swept;mkdocs build --strictgreen.2e09bd7) — the two ruled folds.norm2mortkeeps a length-1 array an array: the form now follows the input rank (scalar out only when both operands are scalars), not the result's size.validate_mortonis marked batch vectorized and itsordercheck covers every element — it comparedorderagainstdepths[0]alone, so a mixed-order array passed on the strength of its first element. Both pinned by mutation-checked tests; both in the CHANGELOG (thenorm2mortshape change is a small break). The fold round extended the rank rule tomort2norm— the documented "exact inverse", which phase 5 had left on the old size rule, making the pair asymmetric.d99fb60) — scalar-return unification onnp.uint64(the phase-3 fold's standing item, espg-approved). The audit sweptmortie.__all__, every non-__all__public in the submodules, and every method/property onMoc/Toc/MortonIndexArray/MortonIndexScalar. Alreadynp.uint64:norm2mort,common_ancestor/moc_min,decimal_to_word. Flipped from Pythonint:time2toc,span2toc,toc_merge,toc_reduce. Deliberately not unified, because they are not words: times in ns (toc2time,from_datetime64,from_gps_ns,to_gps_ns), HEALPix orders (infer_order_from_morton), UNIQ cell ids (geo2uniq,norm2uniq,unique2parent), and the explicit escapesdecimal_to_word(dtype=int)/dtype=MortonIndexScalar/ private_decimal_to_word; the toc set-algebra kernels (toc_normalize,toc_and) always return arrays, so they have no scalar to unify.MortonIndexScalarsatisfies the unification rather than escaping it — it subclassesnp.uint64and overrides only the display dunders. Each flip is pinned by a type-asserting test verified to fail when reverted individually, and the exclusion boundary is pinned too. CHANGELOG entry as a 1.0-window break, with theuint64arithmetic hazards spelled out.4edc7b2) —numpy>=2, per your ruling on question (9) below. NEP 50 is what makes phase 6'suint64word semantics correct; below it a word's arithmetic promotes tofloat64, inexact above 2^53 while mortie words run near 2^62, and bitwise ufuncs against a Pythonintraise outright. The floor is declared in exactly two places and both now agree —pyproject.toml(with the reason in a comment) andbinder/environment.yml(whose pinned mortie release has been numpy-2 compatible since 0.5.2, so the notebooks pay nothing). No CI workflow was touched: every job already installs numpy unpinned, which resolves to numpy 2 and so agrees with the new floor. The phase-6 docstring that had hedged across both numpy generations is simplified to the numpy-2 semantics — the package should not document behaviour it no longer supports. Three tests pin the property the floor exists for (arithmetic and bitwise ops stayuint64; a word near the top of the range survives+1exactly where afloat64round-trip would not), so a downgrade fails loudly instead of silently rounding words. CHANGELOG entry in the same breaking register.77f9e79…3ffe81f), phase 5 → 8 findings (58c7c05…f230413), phase 6 → 8 findings (71bcf0d…28721fe). One commit per finding, a reply on every thread.e0a940a) —origin/maingained Moc object: geometry-first coverage API (issue #196) #197 (theMocobject) and Toc object: temporal coverage composing like Moc (issue #198) #199 (theTocobject, which renamedmortie/toc.py→mortie/_toc.py). Merged as a normal merge commit, four conflicts resolved keeping both sides: notably Toc object: temporal coverage composing like Moc (issue #198) #199's_TocNamespacedeprecation shim rosteredtocs_reduce, which this PR retires, so the name was dropped from the shim roster and a test added that a name this release retires is not shimmed.bd3480f) —origin/maingained ExampleUsage.ipynb cell 15 computes a wrong normalized address — silently wrong for base cells 0-3, raises for 8-11 #142 (the re-executed example notebook), Spec: normative section for the toc word grammar (frozen-for-1.x) #193 (the normative toc word grammar, spec §11) and workspace split: extract mortie-core (pure-Rust codec, no pyo3) from mortie_rustie #200 (themortie-corecrate split). One conflict (CHANGELOG.md), resolved keeping both sides' Unreleased entries. The retirement sweep re-run over what main added caught two reintroductions of retired plural names, fixed in the merge resolution:docs/specification.md§11.5 said "tocs_reducerefuses an empty group" — respelled onto the surviving batch form (toc_reduce(words, offsets=)) — andexamples/toc_temporal_coverage.ipynb(which arrived with Toc object: temporal coverage composing like Moc (issue #198) #199 via the first merge-forward) still rosteredtocs_reducein its flat-API list; the retired name is dropped from the roster. A third round (9f569a9) followed within the hour: main cut the 0.9.11 release (bd3c0c3, the release bot's CHANGELOG/Cargo version sync), so the[0.9.11]heading is merged in with the branch's breaking entries staying under Unreleased — confirming the standing note that the next release after this merges must be 1.0. Gates re-run green on the result.How it was tested
mortie/tests/test_polymorphic_api.py(32 tests) asserts, per operation, that the polymorphic form is byte-identical both to the plural sibling and to a Python loop over the single-item form, over a ragged fixture that includes an empty MOC and a single-cell MOC; that the bare call is exactly the old scalar; thatoffsetscannot be passed positionally; thatmoc_intersects(..., offsets=)agrees slot-for-slot with the non-empty slots ofmoc_and(..., offsets=); that the batch operands are not interchangeable; thatdecimal_to_wordpreserves input shape, keeps a barestrscalar, unwraps 0-d to a scalar, and refuses aMortonIndexScalardtype on array input; thatgenerate_morton_childrengives(4**d,)for a scalar parent and(n, 4**d)for an array, refuses higher rank, and honoursmax_cellsin both forms; plus the layout edges (plain-list offsets, single-group, zero-MOC column, non-monotone,max_cells=None). Every refusal is pinned to its actual message text, anchored.Phases 4-6 added the retirement pins (every retired name gone from the package root, from
__all__, and from its own module), thefrom_wkbdispatch rules (each of the three buckets, themoctri-state including the batchmoc=Falserefusal, byte-identity to both the old scalar and the old batch, the packed column's layout errors, and a delegation spy proving both batch forms reuse the byte-capped chunking kernel with every coverage knob forwarded), the phase-5 rank/every-element pins, and the phase-6 type pins.Gates, on this branch at
e0eba6b(re-run in full on the merged result):pytest— 1854 passed, 30 skipped (the second merge-forward brought main's Spec: normative section for the toc word grammar (frozen-for-1.x) #193/ExampleUsage.ipynb cell 15 computes a wrong normalized address — silently wrong for base cells 0-3, raises for 8-11 #142 tests in; at28721fethis read 1842/16).flake8 mortie --select=E9,F63,F7,F82— clean.numpydoc lint mortie/*.py— clean.ruff check --select=E,F,W,I --ignore=E501— clean on every file this PR touches.mkdocs build --strict— green (the retirement touches docs, so a plural surviving on a rendered page would be a broken build).cargogate applies.Pre-existing hits left alone per §4 (all present on
main, none touched by this diff):F841onon_antimeridianinmortie/convert.py,F811inmortie/morton_index.py, and two--doctest-modulesfailures (mortie/batch.py's+SKIPchain andmortie/linestring.py's cell count) — doctests are not run in CI.One correction to an earlier version of this body: it claimed the "error surface passes through unchanged". That is false and the review caught it — with
offsetsthe message text and the validation precedence both differ (order=99, max_cells=-1raises different errors with and withoutoffsets, because the batch screens layout in its own pass first). What is unchanged is that every refusal is still a catchableValueError.Questions for review
The five questions below were ruled by espg on 2026-08-19 (relayed off-thread); recorded here so the thread is self-contained:
maincarries breaking removals, so the next release cut frommainmust be 1.0 — see the status comment for the release-ordering note.offsets=spelling — ruled (a): keep as implemented.generate_morton_children— ruled (a): numpy semantics as written;children_ofretires with the plurals.from_wkb/from_wkbscollapse — ruled, the "(a+)" design:offsets=present ⇒ packed-column batch (uint8values buffer + arrow list offsets, zero-copy);list/tuple/object-ndarray ⇒ sequence batch (each entry coerced as the scalar accepts);bytes/hex-str/bytearray/memoryview/uint8-ndarray-without-offsets ⇒ one blob. Nobatch=flag.moc=Nonetri-state: scalar defaults to flat cover (oldfrom_wkb), batch defaults to MOC output (oldfrom_wkbs, byte-identical including the chunked memory posture — pinned by delegation-spy and parity tests); explicitmoc=Falseon a batch raises;moc=Trueworks everywhere.mortie.batch.from_wkbsretires.norm2mortlength-1 squeeze — ruled: fold here (phase 5).validate_morton— ruled (a) (phase 5). Scalarnp.uint64unification — approved (phase 6).New questions from phase 4:
mortie.arrow.from_wkbs→mortie.arrow.from_wkbrename stays; the downstream zagg migration is tracked as mortie 1.0 floor bump: migrate retired call sites (arrow from_wkbs rename; shardmap fallback off morton_coverage_moc) englacial/zagg#513. Lean of record: the consolidation's invariant is one name per operation across every skin; the polymorphic core already accepts a packed column under the singular name (viaoffsets=), so a pyarrow column going in under a different name would re-introduce exactly the singular/plural split this PR removes. The skin keeps no dispatch of its own — a column is inherently the batch shape — so the rename is a pure respelling.polygons_to_morton_mocs(one MOC per ring, no holes),from_geometry/from_wkb/from_wktwithmoc=True, andMoc(noorderknob); the kernel survives privately (mortie.coverage._morton_coverage_moc) and the example notebook demonstrates thefrom_geometry(..., moc=True)route.e0eba6b, on top of the review-fold guardaffa852):from_wkb'smocrefuses any value that is notNoneor aboolwith aTypeError(the module's convention for type refusals) naming the parameter, the received value and type, and the positional-migration hazard it guards — a float landing inmocfrom a positionally-migratedfrom_wkbs(blobs, order, tol)call is loud, never a silent tolerance drop. The guard sits at the polymorphic entry point ahead of the dispatch, so all three forms (scalar, sequence batch, packed column) pass through it. The arrow skin does not flow through the entry point (it calls the batch kernel directly) but is safe by signature — it takes nomocat all, so the positional migrationarrow.from_wkbs(col, order, tol)→arrow.from_wkb(col, order, tol)bindstoltotolerance, exactly what it meant, and a straymoc=keyword is refused loudly by Python — pinned bytest_the_skin_has_no_moc_slot_to_poison. Tests pin bool/Noneaccepted in every form and float/str/int refused with the message text in scalar, sequence-batch and packed-column forms; the CHANGELOG line sits under the migration table. (The alternative —mockeyword-only — would have broken scalarfrom_wkb(blob, 8, True)callers, which the phase-4 ruling says survive as-is.)numpy>=2(ruled 2026-08-19, my lean (a) approved). Reasoning of record: thenp.uint64scalar unification is only correct under NEP 50; CI has only ever tested numpy 2 (every job installs it unpinned); so>=1.20was an untested and known-wrong declaration, and 1.0 is the honest moment to state the floor the package actually supports. Landed as phase 7 (4edc7b2) — floor bumped in both places that declare it, the hedged numpy-1 prose deleted rather than maintained, and the NEP 50 property pinned by tests._mocs_and,_from_wkbs,_morton_coverage_moc, …) rather than being rewritten onto the public spellings — the kernels are unchanged and those suites pin kernel behavior (layout errors, GIL release, memory posture); the public delegation surface is pinned separately intest_polymorphic_api.py(retirement pins, dispatch-rule tests, byte-identity to both kernels and scalar loops, the tri-state, and a delegation spy proving both batch forms reuse the chunked kernel).