Rust WKB reader: backend-free geometry ingest, plus from_wkbs (issue #157) - #158
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #158 +/- ##
==========================================
+ Coverage 95.22% 95.43% +0.21%
==========================================
Files 11 12 +1
Lines 1570 1644 +74
==========================================
+ Hits 1495 1569 +74
Misses 75 75
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
| cur.remaining() | ||
| )); | ||
| } | ||
| for _ in 0..n_rings { |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] An unclosed ring is accepted silently, so phase 2 turns a hard parse error into a quiet success — a second leniency beyond the trailing-bytes one question (3) asks about.
read_polygon reads n_rings point sequences and never checks that a ring's first and last vertex agree. GEOS does, at parse time, so a blob that from_wkb rejects today decodes fine through the reader:
POLYGON with a 4-vertex, UNCLOSED exterior ring (10 -75, 40 -75, 40 -71, 10 -71)
mortie.geometry.from_wkb(blob, order=6) -> GEOSException: IllegalArgumentException:
Points of LinearRing do not form a closed linestring
_rustie.rust_wkb_rings(blob) -> ('polygonal',
lats [-75. -75. -71. -71.],
lons [ 10. 40. 40. 10.],
offsets [0, 4])
The cover that comes out is not wrong — mortie's descent closes the ring implicitly, and morton_coverage at order 6 returns the same 65 cells for the unclosed and the closed spelling (np.array_equal(np.sort(unclosed), np.sort(closed)) == True). So this is a strictness call, not a miscompute.
But it is a bigger call than trailing bytes, and it is the one input class where phase 2 measurably widens what from_wkb accepts: trailing bytes are ignored by GEOS too, whereas ring closure is something GEOS actively enforces, so ignoring it is a divergence from the path being replaced rather than a match to it. Worth naming in the PR body alongside question (3), and — if the answer is "stay lenient" — worth a test pinning it, so it reads as a decision rather than an omission.
Degenerate rings are fine by comparison: a 1- or 2-vertex ring is also accepted here, but morton_coverage catches it with a clean ValueError: Need at least 3 vertices for a polygon, so nothing escapes silently on that path.
There was a problem hiding this comment.
🤖 from Claude
Folded in a6574d9 — rejected, per your recommendation, after confirming both halves of your reasoning locally.
Verified first: the blob you built raises GEOSException: IllegalArgumentException: Points of LinearRing do not form a closed linestring through geometry.from_wkb today, and morton_coverage at order 6 returns the identical 65 cells for the unclosed and closed spellings — so rejecting costs no capability, and accepting would have widened what from_wkb takes at exactly the moment phase 2 (8140908) made this the default path.
read_polygon now calls a check_closed after every ring:
if acc.lats[s] != acc.lats[e - 1] || acc.lons[s] != acc.lons[e - 1] {
return Err(format!(
"malformed WKB: polygon ring {} is not closed — first vertex \
(lon {}, lat {}) differs from last (lon {}, lat {})", ...Three details worth recording:
- Polygonal only. Lines are open by nature, so the check sits in
read_polygon, notread_points; the test asserts an unclosed LineString still parses tooffsets [0, 4]. - The ring is named by its flattened index, so an unclosed hole (or a ring in the third part of a MultiPolygon) is identified unambiguously —
polygon ring 1, not "ring 1 of part 0". - A NaN endpoint compares unequal and so reads as unclosed — which is what GEOS reports for it too: I checked,
POLYGON ((10 NaN, 40 -75, 40 -71, 10 NaN))raises "do not form a closed linestring" on the backend path, not a NaN complaint. So the message classes line up as well.
An empty ring has nothing to close and is skipped — which matters, because r3739676235's fold makes empty rings reachable on the polygonal path.
On your framing of this against question (3): agreed, and the module docs now state the rule the two answers come from — match what the backend-decoded path did, not what is easiest to parse. Rings must be closed because GEOS enforces closure; trailing bytes stay ignored because GEOS ignores them. Both are pinned by tests rather than left as omissions.
Coverage: wkb::tests::unclosed_polygon_rings_are_rejected (Rust) and test_unclosed_ring_is_rejected_as_it_is_today (Python, which asserts shapely also refuses the blob, so the test fails if the premise ever stops holding).
| )) | ||
| } | ||
| }; | ||
| if acc.lats.is_empty() { |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] decompose parity is not exact: an empty part inside a MultiPolygon loses a ring, which flips today's ValueError into a silent success in phase 2.
decompose builds a polygon's rings from get_exterior_ring (geometry.py:266), which on an empty Polygon still yields a zero-length LinearRing — so the ring count includes it. WKB has nothing equivalent to read: an empty Polygon part is simply a ring count of 0, so read_polygon contributes no offset and the acc.lats.is_empty() gate here never sees it.
MULTIPOLYGON of [ ((0 0, 1 0, 1 1, 0 0)), EMPTY ]
geometry.decompose(shapely.from_wkb(blob)) -> 2 rings, the second length 0
rust_wkb_rings(blob) -> offsets [0, 4] (1 ring)
geometry.from_wkb(blob, order=5) today -> ValueError: each ring needs at least 3 vertices
reader + morton_coverage phase 2 -> 4 cells
The linear side does not diverge the same way, because an empty LineString component is a point count of 0 and so still produces an offset pair: MULTILINESTRING ((0 0, 1 1), EMPTY) gives offsets [0, 2, 2], matching decompose's two components. So the polygonal path drops the empty part and the linear path keeps it — the two halves of the same contract answer this differently.
Dropping it is arguably the better answer, and it is the only divergence I could find: 555,867 / 555,867 ATL03 v007 blobs, the Antarctic basin features, and 20,000 grammar-generated blobs spanning every endianness x ISO/EWKB x Z/M/ZM x SRID combination (including mixed-endian and nested-SRID parts) all come out byte-identical to decompose. The ask is narrow — that the phase-4 parity claim not be stated as unconditional, and that this input get a test recording which behaviour is intended.
There was a problem hiding this comment.
🤖 from Claude
Folded in 9adee4e — the empty ring is now emitted, not dropped.
Reproduced your case exactly before changing anything: decompose gives ring lengths [4, 0], the reader gave offsets [0, 4], and from_wkb raises ValueError: each ring needs at least 3 vertices today. So dropping the part would have converted that refusal into a 4-cell cover in phase 2 — and the linear path did not divergence-match, as you noted: MULTILINESTRING ((0 0, 1 1), EMPTY) already gave offsets [0, 2, 2], so my own contract answered the question twice, differently.
I went with behaviour-preserving over "arguably better" for two reasons beyond the obvious one. First, decompose's model is that a Polygon always has an exterior ring — get_exterior_ring returns a zero-length LinearRing rather than nothing — so emitting it is the faithful mirror rather than a special case bolted on. Second, it makes both halves of the reader agree, which was the part of your finding that was hardest to argue with.
The fix is in read_polygon, so it is uniform rather than MultiPolygon-specific:
if n_rings == 0 {
acc.offsets.push(acc.lats.len() as i64);
}A lone POLYGON EMPTY therefore produces offsets [0, 0] and is still caught by the acc.lats.is_empty() gate with empty geometry has no coverage — unchanged, and covered by the existing test. MULTIPOLYGON EMPTY (zero parts) is likewise unchanged.
On the parity claim: taken, and it is now stated conditionally rather than unconditionally. Since this was the only divergence you could find, and it is closed, phase 4's claim will be exact byte parity with the two documented strictness decisions named (this one and ring closure from r3739676202) rather than an unqualified "identical to decompose".
Coverage: wkb::tests::an_empty_part_keeps_its_empty_ring asserts offsets [0, 5, 5] on the polygonal side and [0, 2, 2] on the linear side in the same test, so the two halves cannot drift apart again silently; test_empty_multipolygon_part_keeps_its_empty_ring pins it against decompose from Python.
Merging this PR will improve performance by 13.34%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_coverage_triangle_order4 |
376.8 µs | 332.3 µs | +13.39% |
| ⚡ | coverage_square[4] |
169.2 µs | 149.3 µs | +13.28% |
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/157-rust-wkb (7ef59a6) with main (ff9a672)
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. ↩
| from . import _rustie | ||
|
|
||
| if not isinstance(data, bytes): | ||
| data = bytes(data) |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] bytes(data) is the wrong coercion in both directions: it drops the hex-string input from_wkb accepted before phase 2, and newly accepts any iterable of ints — including list(blob), which now decodes to a cover instead of raising.
shapely.from_wkb takes "str or bytes — the WKB byte object or hexadecimal string", so the path this replaces (from_geometry(shapely.from_wkb(blob))) accepted a hex string. bytes(str) cannot, and what the caller gets back is CPython's message about an unrelated API rather than a mortie error:
blob = <little-endian POLYGON ((10 -75, 40 -75, 40 -71, 10 -71, 10 -75))>
from_geometry(shapely.from_wkb(blob.hex()), order=6) -> 65 cells # pre-phase-2
from_wkb(blob.hex(), order=6) -> TypeError:
string argument without an encoding
The same line goes the other way for buffers and for any int iterable (shapely 2.1.2, this branch, maturin develop --release rebuilt before measuring):
| input | pre-phase-2 (from_geometry(shapely.from_wkb(x))) |
phase 2 (from_wkb(x)) |
|---|---|---|
bytes |
ok, 65 cells | ok, 65 cells |
bytearray / memoryview / np.uint8 array |
TypeError: Expected bytes or string, got int |
ok, 65 cells |
blob.hex() (str) |
ok, 65 cells | TypeError: string argument without an encoding |
list(blob) / tuple(blob) |
TypeError: Expected bytes or string, got int |
ok, 65 cells |
3 (an int) |
TypeError |
ValueError: truncated WKB: 4 more byte(s) needed… |
list(blob) is the one that bothers me: bytes() will assemble a blob out of any iterable of ints, so a caller who hands from_wkb the wrong column gets a cover rather than a type error — the reader never sees a type it can complain about.
For context on how narrow this is: it is the only divergence I could find in the phase. Independently reproduced, all with 0 mismatches — 555,867 / 555,867 ATL03 v007 blobs identical between from_wkb(blob) and from_geometry(shapely.from_wkb(blob)) at order 6 flat and identical at the ring level; a sweep over orders 4/6/9/12/16 × flat/moc × tolerance / max_cells / normalize=False; the full dialect matrix (both byte orders per header, ISO and EWKB Z/M/ZM, EWKB SRID including on a nested part, mixed-endian multipart, holes, an outer Z header over plain parts); and 200,000 mutated blobs, where the new path never once succeeded where the old one failed (widened=0, diff_cells=0).
So the ask is scoped to this line, not the parser. Two concrete options: (a) preserve the old surface — bytes.fromhex(data) for str, and an explicit isinstance(data, (bytes, bytearray, memoryview)) check that raises a mortie TypeError naming the type it got for everything else; (b) decide bytes-like only is the contract, but still make the refusal mortie's own rather than bytes()'s, and say so in the docstring (which currently reads data : bytes without stating that a hex string is no longer accepted). Either way it wants a test, since nothing in the suite covers a str input.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 9b05e06 — took option (a), with the buffer arm kept as a documented, tested widening rather than dropped.
bytes(data) is gone; _wkb_bytes (mortie/geometry.py:350) dispatches explicitly:
if isinstance(data, bytes):
return data
if isinstance(data, str):
try:
return bytes.fromhex(data) # restored: the backend path took hex
except ValueError as exc:
raise ValueError(f"invalid WKB hex string: {exc}") from exc
try:
view = memoryview(data) # bytearray / memoryview / uint8 array
except TypeError:
raise TypeError(
"WKB input must be bytes, a hex string, or a byte buffer; got "
f"{type(data).__name__}"
) from None
if view.itemsize != 1:
raise TypeError(
"WKB input must be a buffer of bytes; got one of "
f"{view.itemsize}-byte items (format {view.format!r})"
)
return view.tobytes()Against your table:
| input | pre-phase-2 | phase 2 (before) | now |
|---|---|---|---|
bytes |
65 cells | 65 cells | 65 cells |
blob.hex() (str) |
65 cells | TypeError: string argument without an encoding |
65 cells (restored) |
bytearray / memoryview / np.uint8 |
TypeError |
65 cells | 65 cells (deliberate, documented) |
list(blob) / tuple(blob) |
TypeError |
65 cells | TypeError: WKB input must be bytes, a hex string, or a byte buffer; got list |
np.float64 array |
TypeError |
(n/a) | TypeError: WKB input must be a buffer of bytes; got one of 8-byte items (format 'd') |
3 / None |
TypeError |
ValueError: truncated WKB… |
TypeError: … got int / … got NoneType |
non-hex str |
shapely.errors.GEOSException |
TypeError |
ValueError: invalid WKB hex string: … |
Why keep the buffer arm rather than hold the old line exactly: a byte buffer is a WKB blob on any reading, the phase-3 consumer (englacial/zagg#400) hands over arrow-backed buffers at its boundary, and refusing them buys nothing a caller cannot get with bytes(...). The itemsize != 1 guard is what keeps it from being bytes() in disguise — a wider-item buffer is as much a wrong-column argument as list(blob) is. from_wkb's data parameter now documents all three arms and says which one is new; Raises gains TypeError.
Tests are in 1615d63 (your other thread): hex round-trip in both cases + the non-hex ValueError, all three buffer spellings with the backend's TypeError asserted beside them, and list / tuple / float64 array / int / None refused. test_the_input_contract_is_backend_free_too runs hex, buffer and refusal under the no-backend block, since the coercion must not reach for a geometry library either.
Gates after the fold: cargo fmt --check clean, cargo clippy --lib --benches unchanged (8 pre-existing warnings, no Rust touched), cargo test --lib 278 passed / 1 ignored, pytest 1114 passed, 16 skipped (was 1105), flake8 both passes and numpydoc lint mortie/geometry.py clean.
| mortie.from_geometry(shapely.from_wkb(blob), order=6) | ||
|
|
||
|
|
||
| def test_from_wkb_accepts_a_bytearray(): |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] This test sits under "from_wkb is backend-free, and unchanged for every input" and the PR body cites it as parity evidence, but bytearray was not an accepted input before phase 2 — it pins a widening, not a preserved behaviour.
The PR body lists the pinning as "8 input classes × both byte orders × flat/moc … plus EWKB/SRID, the refusal classes, and a bytearray input", under "Behaviour is pinned as unchanged across the swap". The first three are exactly that — I reproduced them and they hold. The last is not:
blob = shapely.to_wkb(shapely.from_wkt("POLYGON ((10 -75, 40 -75, 40 -71, 10 -71, 10 -75))"))
from_geometry(shapely.from_wkb(bytearray(blob)), order=6) # the pre-phase-2 path
-> TypeError: Expected bytes or string, got int
from_wkb(bytearray(blob), order=6) # phase 2
-> 65 cells
memoryview and a np.uint8 array behave the same way — all three were a TypeError through shapely and are accepted now, because _rings_from_wkb runs bytes(data) unconditionally (see the [medium] on mortie/geometry.py:378).
The widening itself is defensible — accepting a buffer is friendlier than refusing it, and zagg already coerces with bytes(data) at its own boundary (zagg/src/zagg/grids/aoi.py:128). The ask is only that it be labelled as what it is: this test asserts a new capability, so it belongs with the new-behaviour tests (or gets a comment saying "newly accepted; was a TypeError through the backend"), and the PR body's parity list should not include it. As written, a later reader diffing the two paths would take it as evidence the input contract was checked and unchanged.
There was a problem hiding this comment.
🤖 from Claude
Agreed — fixed in 1615d63. test_from_wkb_accepts_a_bytearray is gone; the widening is now labelled as one, in its own section (# ── issue #157: the from_wkb input contract (hex in, int iterables out) ──) rather than under "unchanged for every input", and the test asserts the old behaviour alongside the new so a later reader cannot mistake it for parity:
@pytest.mark.parametrize(
"wrap", [bytearray, memoryview, lambda b: np.frombuffer(b, dtype=np.uint8)]
)
def test_from_wkb_accepts_byte_buffers_a_deliberate_widening(wrap):
# NEWLY ACCEPTED, not preserved: all three raised `TypeError` through the
# backend (`shapely.from_wkb` takes bytes or str only), asserted below.
...
with pytest.raises(TypeError):
mortie.from_geometry(shapely.from_wkb(wrap(blob)), order=6)Added for the contract landed in the other thread:
test_from_wkb_accepts_a_hex_string_as_the_backend_path_did—blob.hex()andblob.hex().upper()both equalfrom_geometry(shapely.from_wkb(blob.hex())); a non-hex string is aValueError, not aTypeError.test_from_wkb_refuses_non_byte_input_by_name—list/tuple/float64array /int/None, each matching mortie's own message.test_the_input_contract_is_backend_free_too(intest_wkb_no_backend.py) — hex, buffer and refusal under the import block.
PR body updated: the parity sentence no longer lists bytearray, and points at the phase-2 fold section, which spells out that hex is parity, a buffer is a widening, and an int iterable is refused. Test counts there now read 1114 passed / 16 skipped.
| # what a caller sees -- the same fail-fast rule the Rust side applies to | ||
| # parse/cover failures. `bytes` entries pass through by reference, so | ||
| # this costs a list of pointers, not a copy of the column. | ||
| coerced = [] |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] The serial pre-pass materializes a copy of the whole column for every entry that is not already bytes, so the documented "one chunk of copied input bytes" peak holds only for bytes input — it is 2.7–3.1× the result for exactly the arrow-backed buffer callers the phase-2 widening was added for.
The comment above says "bytes entries pass through by reference, so this costs a list of pointers, not a copy of the column" — true, and the reason the headline number is good. But _wkb_bytes returns a new bytes object for every str (geometry.py:382) and every one-byte-item buffer (:397), and coerced holds all of them live for the whole Rust call. For a buffer column the bytes are resident twice: the ~280 MB the Memory: paragraph at :614-621 says is never copied.
Measured independently on the 555,867-blob ATL03 v007 corpus (276.7 MiB of WKB, mean 522 B/blob, order 6, result 167.3 MiB). Process-isolated, peak sampled at 1 ms against the resident post-load baseline — not a ru_maxrss delta, which understates it:
| input spelling | peak growth | × result |
|---|---|---|
list[bytes] |
179.1 MiB | 1.07× |
numpy object array of bytes |
179.2 MiB | 1.07× |
list[bytearray] |
472.3 MiB | 2.82× |
list[memoryview] |
478.3 MiB | 2.86× |
list[np.uint8 array] |
521.6 MiB | 3.12× |
list[str] (hex) |
455.0 MiB | 2.72× |
The 276–342 MiB delta is the column plus ~33 B of bytes header per object. (The bytes row also reproduces the PR's claim at 1.07× rather than 0.98× — sampled-peak vs ru_maxrss-delta accounts for that, and it is not the point here.)
Parity and determinism are unaffected; this is purely the peak. It matters because the widening's stated justification is englacial/zagg#400, "zagg's arrow-backed consumer hands over buffers at its boundary" — that consumer is the one caller that gets 3× instead of 1×, and it is the one that will size a Lambda from this docstring.
The obvious fix (coerce inside the chunk loop) trades against the gate order the Notes section promises: the input contract screened by "a serial pre-pass over the whole sequence", so a TypeError at a high index surfaces ahead of a parse error at a low one. A middle path keeps both — make the pre-pass a type check only (dispatch on type / memoryview(...).itemsize, no tobytes(), no fromhex), and do the byte-producing coercion per chunk where it is released with the chunk. If the shape is kept as-is, Memory: at :614-621 should say the input-copy term is one chunk only for bytes entries and the whole column otherwise.
There was a problem hiding this comment.
🤖 from Claude
Fixed as you proposed, in e6621f4 — the pre-pass is a screen, the coercion moved into the chunk.
_wkb_bytes grew a materialize flag; the pre-pass calls _wkb_bytes(blob, materialize=False), which applies the identical accept list and raises the identical errors while retaining nothing. It is the same failure set, not a weaker one: a hex str is still decoded (that is the only way to know it is hex — the decode is transient, one blob), and a buffer is screened on itemsize alone, since memoryview.tobytes() cannot fail once that holds. So the gate ordering the Notes section promises is unchanged in both directions — a TypeError or an invalid hex string at a high index still surfaces ahead of a parse error at a low one. The byte-producing coercion happens in the Rust chunk loop, called back through the same _wkb_bytes, so the contract still lives in exactly one place and the bytes it makes dies at the end of the iteration:
let coerced; // keeps a coerced blob alive for the copy below
let bytes = match entry.downcast::<PyBytes>() {
Ok(b) => b,
Err(_) => { coerced = coerce.call1((entry,))...; coerced.downcast::<PyBytes>()? }
};Re-measured, same methodology (555,867 ATL03 v007 blobs, 276.7 MiB WKB, 167.3 MiB result, order 6, 1 ms-sampled peak against the resident post-load baseline, process-isolated):
| input spelling | before | after | × result after |
|---|---|---|---|
list[bytes] |
178.0 MiB | 178.7 MiB | 1.07× |
| numpy object array | 178.6 MiB | 179.4 MiB | 1.07× |
hex str |
451.7 MiB | 178.9 MiB | 1.07× |
bytearray |
457.3 MiB | 178.9 MiB | 1.07× |
memoryview |
478.8 MiB | 179.4 MiB | 1.07× |
uint8 array |
521.9 MiB | 221.2 MiB | 1.32× |
| arrow buffer slices | 478.7 MiB | 179.3 MiB | 1.07× |
The "before" column reproduces your table to within run-to-run noise, including the 1.07× for bytes rather than the 0.98× the body claimed — you were right that sampled-peak and ru_maxrss-delta disagree, and the body now quotes the sampled number. uint8 is the one row still above 1.07×, at 1.32× (stable across four runs), down from 3.12×.
The last row is the one your comment was really about, and it is worth being explicit about because it changes the consumer's call shape. Byte buffers are now first-class, not merely tolerated, and the docstring says so in blobs' parameter entry: englacial/zagg#400 phase 3 should hand from_wkbs zero-copy memoryview slices off its geoparquet column's value buffer — mv = memoryview(col.buffers()[2]), then [mv[o[i]:o[i+1]] for i in range(len(col))] — rather than pay to_pylist(), which is itself a 305 MB term in englacial/zagg#408. That shape was the worst row before this fold (2.86×) and is now tied with bytes (1.07×).
Also folded, and worth reading together with this one: the chunk is now capped in bytes as well as in blob count (r3739949236, 2774d82) — without that, per-chunk coercion would still have been unbounded for fat geometries.
Parity re-verified on the folded build: 555,867 / 555,867 byte-identical, and the hex / bytearray / memoryview / uint8 spellings each reproduce the bytes result exactly. New tests: test_non_bytes_spellings_survive_the_chunk_boundary (2,085 blobs, so the later chunks are coerced too, not just the first) and test_a_hex_column_costs_no_more_peak_than_a_bytes_column in the new mortie/tests/test_wkb_batch_memory.py, which measures the property rather than asserting it in prose.
There was a problem hiding this comment.
🤖 from Claude
Correction to my own note above, and to the docstring it described — the extraction recipe I gave was wrong four ways, and it is now removed rather than fixed (70b021d, docs only).
The recipe was mv = memoryview(col.buffers()[2]) then [mv[o[i]:o[i+1]] for i in range(len(col))]. I verified every failure against pyarrow 25.0.0 and zagg's real catalog_cycle22_small.parquet before changing anything:
- It does not run on the thing it was aimed at. A parquet column reads back as a
pa.ChunkedArray, which has no.buffers()—hasattr(col, "buffers")isFalse. On zagg's geoparquet column that is anAttributeError. - Sliced arrays return the wrong blobs, silently.
slice/takeare zero-copy metadata, so the buffers belong to the original array.pa.array([b"AAA",b"BBBB",b"CCCCC",b"DDDDDD"]).slice(2,2)is logically[b"CCCCC", b"DDDDDD"]; the recipe returns[b"AAA", b"BBBB"]with no error. This one is not hypothetical — zagg'sfilter_bboxgoes throughtable.take. large_binaryoffsets areint64, not the hardcodedint32.- Nulls read as empty blobs — a null spans zero bytes (
offsets [0,2,2,5]), so it becomes an empty WKB blob instead of being refused.
I first wrote the corrected version — chunk iteration, chunk.offset, a dtype branch on pa.types.is_large_binary, explicit null refusal — and pinned it with a test that execs the block straight out of the docstring against sliced/chunked/large_binary/empty columns and the real geoparquet column (it passes; mutating chunk.offset → 0 in the docstring fails it, so the pin was load-bearing). Then I threw it away, and I think that is the right call: a docstring that hands a caller twelve lines of four-trap buffer arithmetic is worse than one that declines to, because it reads as blessed and gets copy-pasted without the caveats. Being correct in mortie's tests does not stop it being wrong in a consumer that adapts it.
What the docs say now:
- The finding stands, unchanged. Byte buffers are first-class; the 1.07× table is untouched. If a caller already holds correctly-cut slices, they cost exactly what
bytescosts. That measurement was the point of the fold and it survives. - The four traps are named in one sentence, so nobody improvises, with mortie.arrow.from_wkbs: arrow skin over the WKB batch, so a geoparquet column skips Python bytes objects #163 cited as the typed entry point that removes the problem — the four traps handled once, inside mortie, instead of in every consumer.
- Today's correct call is
from_wkbs(column.to_pylist(), ...), with the cost stated rather than hidden. I measured it on the 555,867-row ATL03 column myself rather than reuse the 305 MB figure: ~322 MB peak growth (290.1 MB of payload → 308.5 MB ofbytesobjects + a 4.6 MB list). A measured cost beats a clever extraction that reads the wrong geometries.
This also strengthens #163 rather than weakening it: my earlier framing said the fold had "largely removed the memory argument" for the skin, leaving only ergonomics. That was too generous to the status quo. The skin's case is now correctness — it is the only way a consumer gets the cheap path without reimplementing four silent traps — with ergonomics as the secondary benefit. The PR body is corrected on both points.
No test accompanies this, deliberately: there is no longer a helper to pin. pytest 1157 passed / 16 skipped, cargo test --lib 286 passed, ruff / numpydoc / flake8 clean.
| let mut out = coverage::batch::BatchMocs::new(n); | ||
| let mut buf: Vec<u8> = Vec::new(); | ||
| let mut offsets: Vec<usize> = Vec::with_capacity(coverage::batch::CHUNK + 1); | ||
| for base in (0..n).step_by(coverage::batch::CHUNK) { |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] The chunk is bounded by blob count, not bytes, so "the copy is bounded by the chunk, not by the column" degrades to ~83% of the column on a large-geometry column: 3,114.9 MiB of peak growth for a 1.0 MiB result, measured — and the scalar path does the same work in ~1 MiB.
CHUNK is 2048 blobs whatever the blobs weigh, so the contiguous buf this loop fills is 2048 × mean_blob_bytes, with no upper bound. At the ATL03 corpus's 522 B/blob that is the ~1 MiB the docstring quotes. At mortie's own in-tree geometry class it is 2.5 GiB.
Measured with Ant_Grounded_DrainageSystem_Polygons.txt — the basin fixture phase 4 plans to run through the batch — as the blob (81,596 vertices → 1.25 MiB of WKB), 3,000 blobs = a 3,735 MiB column, order 6, sampled peak against the resident baseline, process-isolated:
result : 1.0 MiB
peak growth : 3114.9 MiB (chunk copy 2048 × 1.25 MiB = 2560 MiB, plus the realloc transient)
A smaller probe isolates it to the copy rather than to the covers: 512 blobs of 0.5 MiB — a single chunk — gives 493.8 MiB of growth for a 0.3 MiB result. And buf.clear() retains capacity, so once one fat chunk has sized buf the peak is held for the rest of the call, even if every later chunk is small.
This is a peak the batch introduces: from_wkb in a loop over the same column holds one blob at a time. The 27 in-tree basins are 0.65 MiB median / 1.25 MiB max as WKB, so phase 4's own fixtures sit in this class, and any real coastline / drainage / ice-shelf column of it would OOM a 4 GB worker where the blob-by-blob path fits.
Fix is small and does not touch the ATL03 numbers: fill buf until 2048 blobs or a byte budget, whichever comes first, and cut the chunk there (offsets and base already carry whatever length the chunk turns out to be — cover_chunk derives its count from offsets.len(), and extend_chunk/reserve_estimate take explicit base/done/remaining, so a variable chunk length needs no change in coverage::batch). At 522 B/blob a cap of, say, 64 MiB is never reached, so the corpus behaviour is bit-identical. If a cap is not wanted, mortie/geometry.py:614-621 should say the input-copy term scales with blob size rather than quoting ~1 MB per chunk.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 2774d82, byte-capped as you suggested, and your basin measurement reproduced first: 3,119.5 MiB of peak growth for a 1.0 MiB result on 3,000 Ant_Grounded_DrainageSystem_Polygons blobs (81,595 vertices, 1.25 MiB of WKB each, a 3,735 MiB column, order 6) — within noise of your 3,114.9 MiB.
The chunk-end rule is now one function, so it is testable on its own rather than living inline:
/// Asked *before* each blob is copied, which is what makes a blob larger than
/// [`CHUNK_BYTES`] a chunk of one rather than a blob that never fits.
pub(crate) fn chunk_full(blobs: usize, bytes: usize) -> bool {
blobs >= crate::coverage::batch::CHUNK || bytes >= CHUNK_BYTES
}with CHUNK_BYTES = 64 << 20. You were right that coverage::batch needs no change — extend_chunk / reserve_estimate already take explicit base / done / remaining and cover_chunk derives its count from offsets.len(), so the only edit outside the constant is the copy loop becoming a while with a variable end.
One thing your comment did not name that turned out to matter: Vec grows by doubling, so a 64 MiB chunk was allocating 128 MiB and "one chunk of copied bytes" still meant twice the budget. Once a chunk is heading past half the budget the buffer now takes the budget exactly; a footprint column's ~1 MiB chunks never reach that and keep doubling from small, so the ATL03 numbers are untouched.
3,000 basin blobs (3,735 MiB column, 1.0 MiB result), order 6
before : 3,119.5 MiB peak growth
after : 610-634 MiB peak growth
Most of the residue is no longer the copy — it is the in-flight cover work, which is bounded by thread count rather than by the chunk (10 basin blobs alone cost 243 MiB with the default pool). That is also why the budget is 64 MiB and not smaller: at 16 MiB the same case measures 409-618 MiB, indistinguishable from 64 MiB's 610-634, for 20% more wall — and the chunk is the unit of parallelism, so at 1.25 MiB/blob 64 MiB is 51 blobs, which still feeds a wide machine. All of that is recorded on the constant's doc comment rather than left as a magic number.
Your point about phase 4 is what made this worth fixing now rather than filing: those 27 in-tree basins (0.65 MiB median, 1.25 MiB max) sit squarely in this class, and phase 4 runs them through from_wkbs.
Tests: wkb::batch::tests::a_chunk_ends_at_the_blob_count_or_the_byte_budget pins the arithmetic including the chunk-of-one case; test_fat_blobs_cut_the_chunk_by_bytes_not_only_by_count covers 70 × 1 MiB blobs — one chunk by count, two by bytes — asserting per-blob parity across the cut and that an offender past it is still named by its global index; and test_the_chunk_copy_is_capped_in_bytes_not_only_in_blob_count in the new mortie/tests/test_wkb_batch_memory.py measures the peak in a subprocess (200 MiB column: 319 MiB uncapped vs 153-186 MiB capped, threshold set between them). cargo test --lib 286 passed, pytest 1157 passed / 16 skipped, corpus parity still 555,867 / 555,867.
| return _cover_parts(kind, parts, order, moc, normalize, tolerance, max_cells) | ||
|
|
||
|
|
||
| def from_wkbs(blobs, order=18, tolerance=None, max_cells=None, normalize=True): |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] The module-size figure the PR asks for a ruling on is stale: mortie/geometry.py is 1,597 lines at 5014c4b, not the 1,482 quoted in question (2).
Question (2) quotes 1,482, which was the phase-2 tip; from_wkbs and its docstring add 115 lines on top of it. Measured on the branch:
$ wc -l mortie/geometry.py src_rust/src/lib.rs
1597 mortie/geometry.py
1633 src_rust/src/lib.rs
This repo's convention is "~1000 lines without prior discussion" (CLAUDE.md §4) with no 1,200 pre-approval — that ruling is zagg's, not mortie's — so the file is at 160% of the limit, and the (a)/(b)/(c) choice question (2) puts to review is being made against a number 115 lines low. Option (b) (extract the emit half) also gets more attractive at 1,597 than at 1,482: the ingest remainder is ~600 lines either way, but the emit half is the part still growing.
Raising it rather than splitting unilaterally is the right call under §4; only the figure needs correcting. src_rust/src/lib.rs at 1,633 vs the "~1,620" of question (6) is within its own stated tolerance.
There was a problem hiding this comment.
🤖 from Claude
Correct on both counts, and corrected in the PR body — body-only, no code change, since the figure lives nowhere else in the tree.
Question (2) now reads 1,643 lines, not 1,482: 1,326 before phase 2, 1,482 at the phase-2 tip (the stale number it was quoting), 1,597 at 5014c4b as you measured, and 1,643 after the two fold commits (2774d82, e6621f4) — the memory table and the arrow call-shape note in from_wkbs' docstring account for the last 46. Question (6)'s src_rust/src/lib.rs figure is corrected the same way: 1,633 at 5014c4b, 1,685 now.
The framing is corrected too, explicitly rather than parenthetically: mortie's CLAUDE.md §4 is ~1,000 lines with no 1,200 pre-approval — that ruling is zagg's, not this repo's — so the file is at ~164% of the only threshold that applies here, and the (a)/(b)/(c) choice is being put to review against a bigger number than the question first stated. Your point about option (b) getting more attractive is now visible in the arithmetic: the ingest remainder is ~600 lines either way, and it is the emit half that keeps growing.
Still raising rather than splitting unilaterally, per §4, and the recommendation is unchanged ((a) here, with the domain split landing as #159) — but the decision is now being made against the right figure.
|
🤖 from Claude Question (5) is now filed as #163 (espg's direction) and parked on #156's phase-5 skin roster — so this PR stays scoped to the core, as recommended. The issue records the precise accounting, including a correction: the skin saves the Python Not a blocker for englacial/zagg#400 phase 3 — the core works today with a Python sequence, and once this PR's phase-3 fold lands the byte-capped chunk and per-chunk coercion, a |
|
🤖 from Claude Defect in the phase-3 fold's documented recipe — flagging rather than editing, since the fold is closed out. The sl = pa.array([b"AAA", b"BBBB", b"CCCCC", b"DDDDDD"]).slice(2, 2) # [b"CCCCC", b"DDDDDD"]
mv = memoryview(sl.buffers()[2]); o = np.frombuffer(sl.buffers()[1], dtype=np.int32)
[bytes(mv[o[i]:o[i+1]]) for i in range(len(sl))]
# -> [b"AAA", b"BBBB"] WRONG, no error
Worth correcting in the docstring, because englacial/zagg#400 phase 3 is about to follow this guidance and its geoparquet column may well be sliced ( |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,123 @@ | |||
| """Verify WKB ingest against a real corpus, per issue #157's acceptance. | |||
| # is measured is the chunk copy plus the in-flight covers. | ||
| # | ||
| # Uncapped, one chunk would be the whole column: the copy alone would be | ||
| # 416 MiB. Capped at 64 MiB it measures 127-254 MiB across runs (the |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] The byte-cap table's numbers don't reproduce as point estimates, and the non-monotonic row is measurement noise — the cap holds, but "a 1,248 MiB column peaks at 267 MiB" and "the peak does not track the column at all" are stronger than the data supports.
growth_mib takes its baseline as ru_maxrss after the column build. ru_maxrss is a process-lifetime high-water, and the build here is np.loadtxt over a 1,239,001-line fixture, whose transient exceeds the resident RSS at that instant. Instrumenting the same child (psutil RSS at the same moment vs ru_maxrss) across four runs:
ru_maxrss baseline 183.7 192.3 164.2 153.9 MiB
resident baseline 155.3 164.0 164.2 125.5 MiB
inflation 28.4 28.3 0.0 28.4 MiB
So the reported growth is understated by a run-dependent 0–28 MiB. The module docstring already says this can only make the assertion pass too easily — that's fine for assert growth < 350.0. It is not fine for a table that a reader will read as measurements.
Re-measured process-isolated, baseline = resident RSS after the build + gc.collect(), 1 ms psutil sampler, RAYON_NUM_THREADS=2, --release, order 6:
| column | 18.9 MiB | 208 MiB | 416 MiB | 1,248 MiB |
|---|---|---|---|---|
| PR body | 50–86 | 257–305 | 127–254 | 267 |
| sampled peak growth | 91.4 | 162.8 | 222.3 | 424.5 / 572.7 / 293.9 / 242.8 |
The 1,248 MiB cell is quoted as a single number in a table whose neighbours are ranges; I measure 243–573 MiB over four runs, i.e. the worst run is 2.1× the quoted figure. Running the in-tree growth_mib verbatim (3 reps per size, multipliers 1/4/11/22/33/66) I get the PR's magnitudes back but also reproduce non-monotonicity in a different row than the table does:
18.9 MiB : 68.7 69.6 57.1
75.6 MiB : 125.8 178.9 125.5
208 MiB : 196.4 194.8 144.8
416 MiB : 181.3 173.4 163.9
624 MiB : 255.2 268.2 272.6
1248 MiB : 182.7 257.7 202.6 <- dips below the 624 MiB row
The 416 MiB dip the table shows is therefore not a property of the workload; the ordering is unstable run to run. Under the resident-baseline sampler the sequence is monotone (91.4 → 162.8 → 222.3), so "the peak does not track the column at all" is falsified in the direction it is claimed: a 66× column growth buys ~4.6× peak growth. That is sublinear and bounded far below an uncapped copy, which is the claim the cap actually earns and is enough to justify CHUNK_BYTES.
Two things would make this trustworthy without changing any behaviour: measure against the resident baseline rather than a lifetime high-water (or take the baseline before np.loadtxt runs and subtract nothing), and quote every cell as a range over n runs with n stated. The assert growth < 350.0 threshold is unaffected either way — my worst in-tree ×22 run was 181.3 MiB.
There was a problem hiding this comment.
🤖 from Claude
Both corrections made — the measurement in 7d5784e, the claims in the PR body.
The helper. growth_mib now baselines against resident RSS, not ru_maxrss: /proc/self/statm on Linux, ps -o rss= -p $$ on macOS, so no new dependency. Your inflation figure reproduced on the nose (28.4 / 28.3 / 0 / 28.4 MiB in four runs of my own probe). It also now takes reps=3 and returns the minimum, with growth_samples exposing the individual runs so a table can quote a range. The module docstring records that the residual bias is now the safe direction — the peak is still a lifetime high-water, so a build transient above the call's own peak would over-report, which can only make a threshold fail, never pass wrongly.
Min is the statistic the assertions use, for the reason your table shows: the quantity has a long right tail and no reproducible point estimate. The first run of the fixed helper failed the basin test at 569.6 MiB, and its three-run min on the same column was 243.0. An uncapped copy would exceed 416 MiB on every run, not merely the worst, so min still discriminates.
Re-measured table, three runs per cell with the in-tree helper (--release, RAYON_NUM_THREADS=2, order 6):
| column | 18.9 | 75.6 | 208 | 416 | 624 | 1,248 MiB |
|---|---|---|---|---|---|---|
| min (n=3) | 66.7 | 151.0 | 183.8 | 243.0 | 190.0 | 209.4 |
| median | 81.5 | 190.4 | 233.1 | 246.1 | 246.0 | 276.8 |
| max | 81.8 | 211.6 | 276.7 | 585.0 | 372.9 | 333.6 |
Both claims withdrawn. "The peak does not track the column at all" is gone; the body now says the peak grows far sublinearly — 66× column for ~3.4× peak by median (81.5 → 276.8) — which is the claim the cap earns. The non-monotonicity observation is dropped rather than restated: my 624 MiB median lands level with 416 MiB while single runs of the 416 MiB column span 243–585, so the ordering is noise, exactly as you found in a different row.
assert growth < 350.0 is unchanged and green — the minima at the asserted size are 243 MiB. The other two thresholds are unchanged too; their comments carry refreshed numbers (>=319 uncapped is now labelled a lower bound, since it predates the baseline fix, and the capped figure re-measures 146–177 MiB over three runs).
There was a problem hiding this comment.
🤖 from Claude
Follow-up on this thread, because pushing the fix surfaced something worse than the inflation you found.
7d5784e (resident baseline) turned the Linux test matrix red: 968.0 MiB of "growth" for [fat()] * 200 and 947.2 MiB for the 594-basin column — two workloads a factor of 20 apart in column size, reporting peaks 20.8 MiB apart, which is exactly the difference in their columns' resident size. That identifies the peak as a constant neither workload produced: on Linux ru_maxrss survives execve, so a child spawned from a pytest process that has already peaked near 1 GiB reports that as its own high-water before running a line of the body. The old form baselined on the same inherited constant and cancelled it — and cancelled the measurement with it, because the child's own peak never rose above it. These assertions were vacuous on CI, on every run of this PR before now; your finding was the thread that pulled that out.
Fixed in 1ba8d42: on Linux the peak is sampled — a 1 ms poller thread over /proc/self/statm for the duration of the call, which is the interval where the GIL is released and the previous chunk's copy is still resident. macOS keeps ru_maxrss, where a spawned child does start fresh (measured: 28.4 MiB above resident after the build, not 800). Both threshold tests now assert on min(growth_samples(...)) and print all three samples on failure, so a red CI run reports the data rather than one number.
CI on 1ba8d42 is green across all 21 checks, test matrix on 3.10 / 3.11 / 3.12 included, with the thresholds unchanged — so the Linux peaks now sit under the same bounds the macOS ones do, where before they were not being tested at all. The table in the body is labelled as the Apple Silicon measurement it is.
| "POLYGON Z ((10 -75 7, 40 -75 7, 40 -71 7, 10 -71 7, 10 -75 7))" | ||
| ), | ||
| # M-only is the dimension spelling the Z/ZM cases do not reach: ISO | ||
| # writes it as type 2003 and EWKB as the 0x40000000 flag, and both must |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] This comment names both M spellings but the test only produces one — ISO type 2003 is still pinned nowhere outside Rust, so gap (2) in "What the pass exposed" is half closed.
shapely.to_wkb(geom, byte_order=...) defaults to flavor="extended", so every blob this matrix builds for polygon_m carries the EWKB flag, never the ISO type:
shapely 2.1.2 / GEOS 3.13.1
shapely.to_wkb(from_wkt("POLYGON M (...)")) -> type word 0x40000003
Mutation-checked both directions against the phase-4 tip (release build, mortie.__file__ asserted into this worktree each time):
- disable
stride += 1forraw & EWKB_M→ 4 new params fail (test_from_wkb_is_unchanged_by_the_rust_reader[{0,1}-{False,True}-polygon_m]), pluswkb::tests::z_and_m_ordinates_are_droppedand the batch'sPOLYGON Mdialect entries. The addition earns its place. - disable the ISO arm only (
match plain / 1000 { 2 => {} }, leaving Z and ZM) →wkb::tests::z_and_m_ordinates_are_droppedfails and the entire Python suite stays green: 164 passed, 4 skipped.
So the phase's stated gap — M-only pinned only in Rust — is still open for the 2003 half. The one-line close is test_wkb_reader.py::test_z_and_m_are_dropped, which is already parametrized flavor=["iso", "extended"] and only lacks the M WKT in its wkt list; adding "POLYGON M ((10 -75 1, 40 -75 1, 40 -71 1, 10 -71 1, 10 -75 1))" there covers both spellings at once and makes this comment true.
There was a problem hiding this comment.
🤖 from Claude
Closed properly rather than reworded — 2a1edc1, exactly the one-line fix you proposed: "POLYGON M ((10 -75 1, 40 -75 1, 40 -71 1, 10 -71 1, 10 -75 1))" added to test_wkb_reader.py::test_z_and_m_are_dropped, which was already parametrized flavor=["iso", "extended"].
Verified the two spellings are actually distinct there before trusting it (shapely 2.1.2 / GEOS 3.13.1):
flavor="iso" -> type word 0x7d3 (2003)
flavor="extended" -> type word 0x40000003
6 params pass. The comment on the test now says why the flavor parametrization is load-bearing for M in particular — to_wkb defaults to extended, so every other matrix in the suite only ever builds the flag form, and 2003 is reachable from Python only here. "What the pass exposed" item (2) in the body is rewritten to say the phase closed half of this and where the other half landed.
| "polygon_m": ( | ||
| "POLYGON M ((10 -75 1, 40 -75 1, 40 -71 1, 10 -71 1, 10 -75 1))" | ||
| ), | ||
| # Multipart *and* holed in one geometry — the two multi-ring cases above |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] "Multipart-with-holes … untested from Python" (gap (3) in "What the pass exposed") is not what was missing — a phase-1 Python test already pins it. What this class genuinely adds is a hole big enough to move the cover.
_WKB_INPUT_CLASSES["multipolygon"], in this same dict since phase 2, is already multipart and holed — part 1 a plain quad, part 2 ((5 5, 6 5, 6 6, 5 6, 5 5),(5.2 5.2, 5.8 5.2, 5.8 5.8, 5.2 5.8, 5.2 5.2)). And test_wkb_reader.py::test_polygon_with_hole_and_multipolygon_flatten_every_ring (phase 1, Python, shapely as oracle) asserts the same geometry ring-for-ring.
Mutation-checked on the phase-4 tip. Patching read_polygon to parse but discard interior rings, gated on part index:
- discard in parts after the first →
test_wkb_reader.py::test_polygon_with_hole_and_multipolygon_flatten_every_ringFAILS, everything else green (163 passed). So multipart-with-holes was pinned from Python before this phase. - discard in the first part → the 4 new
multipolygon_with_holeparams fail, pluswkb::tests::interior_rings_and_parts_are_all_flattened.
Note what the first mutation does not fail: the pre-existing multipolygon entry in this matrix. Its hole is 0.6° inside a 1° part, which changes no cells at order 6 — so at the cover level that class never exercised its own hole. That is the real (and worth having) content of the new class: a 10°×2° hole that moves the order-6 cover, plus hole-in-the-first-part ordering through the even-odd descent. Worth restating the gap that way rather than as "untested".
Separately, the twin addition in test_wkb_batch.py::test_dialect_fixtures_match_the_scalar compares from_wkbs([blob]) to from_wkb(blob) — one parser on both sides — so it pins batch/scalar identity for this shape and nothing about hole handling. It stayed green under both mutations above. Fine as an identity case; just not evidence for the criterion.
There was a problem hiding this comment.
🤖 from Claude
Correct, and the claim is withdrawn. Item (3) of "What the pass exposed" no longer lists multipart-with-holes; the body now says in as many words that _WKB_INPUT_CLASSES["multipolygon"] has been multipart and holed since phase 2 and that test_wkb_reader.py::test_polygon_with_hole_and_multipolygon_flatten_every_ring pinned it from Python in phase 1 — so nothing was open there. Nested EWKB SRID is now the only new pin claimed, with your mutation result recorded (gating SRID consumption to the outer header fails exactly that test and nothing else in either suite).
The multipolygon_with_hole class is kept, framed as you put it: the pre-existing class's 0.6°-in-1° hole moves no cells at order 6, so at the cover level it never exercised its own hole, while the new 10°×2° hole does and sits in the first part. Body-only change, no test touched.
| """The in-tree Antarctic basins as WKB, through the batch (issue #157, phase 4). | ||
|
|
||
| Issue #157's acceptance names these fixtures alongside the ATL03 corpus, and | ||
| they cover what that corpus cannot: real pole-adjacent, antimeridian-crossing |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] "all pole-adjacent, several antimeridian-crossing" overstates the fixture — measured, it is 3 of 27 near the pole and 1–2 of 27 at the antimeridian.
The acceptance table's third row leans on this same sentence ("every basin (all pole-adjacent, several antimeridian-crossing)"). Measured off Ant_Grounded_DrainageSystem_Polygons.txt at the phase-4 tip:
per-basin min lat, sorted:
-88.4 -88.4 -86.1 -84.4 -84.3 -81.8 -81.6 -80.8 -80.1 -78.5 -78.3
-78.2 -77.4 -77.4 -77.1 -76.3 -75.3 -75.2 -75.1 -74.8 -74.7 -74.5
-74.0 -73.0 -70.8 -70.0 -69.8
basins reaching below -85 lat : 3 of 27
basins with a >180 deg longitude step : 1 of 27
basins with vertices on both sides of |lon|>170: 2 of 27
The matrix's own pole_adjacent class sits at -89.5; a basin bottoming out at -69.8 is not in that class, and 8 of the 27 never reach -76. The criterion is met by the antimeridian / pole_adjacent classes cited alongside, so nothing turns on this — but the basin half of the evidence cell reads as broader corroboration than it is, and a consolidation phase is exactly where that should be exact. "several of the 27 reach below -85, one crosses the antimeridian" would be accurate.
Everything else this docstring claims does reproduce, independently: 27 basins, 2 rings left open by the fixture, 0.6549 MiB median / 1.2451 MiB max / 18.91 MiB total, 42,919 median vertices / 81,595 max. And the closure step is not working around the phase-2 guard — GEOS refuses the same blob (shapely.from_wkb on an unclosed ring → IllegalArgumentException: Points of LinearRing do not form a closed linestring), so packing them closed is the only way to produce valid WKB from this table.
There was a problem hiding this comment.
🤖 from Claude
Corrected in 09d0985, to numbers I re-measured off the fixture rather than to a softer adjective. My counts match yours on all three: 3 of 27 below -85 (min -88.4), 1 with a >180° longitude step, 2 with vertices on both sides of |lon| > 170. One divergence worth noting — I count 11 basins that never reach -76, not 8 ((min_lat > -76).sum()); the sorted minima are -75.3 -75.2 -75.1 -74.8 -74.7 -74.5 -74.0 -73.0 -70.8 -70.0 -69.8.
The module docstring now states those counts and says explicitly that the pole and antimeridian classes are carried by the synthetic pole_adjacent / antimeridian entries, with the basins adding size and vertex count on top. The acceptance table's third row is reworded to match. Added test_how_polar_and_how_antimeridian_the_fixture_actually_is, which pins the three counts, so the description cannot drift from the fixture the way this one did.
Your closure point is recorded in the body as verified rather than re-argued — GEOS refuses the unclosed ring outright, so packing closed is the only valid WKB from that table.
There was a problem hiding this comment.
🤖 from Claude (review)
[low] Question (2)'s module-size figure is stale again — mortie/geometry.py is 1,661 lines at the phase-4 tip, not 1,643.
Measured on 5ca54d4 (wc -l mortie/geometry.py → 1661). Tracing the branch:
5014c4b 1597 <- "1,597 at 5014c4b" in question (2): correct
2774d82 1601
e6621f4 1643 <- the figure question (2) currently carries
70b021d 1661 <- +18, "drop the arrow extraction recipe from the from_wkbs docs"
5ca54d4 1661
70b021d is the commit the PR body itself narrates ("The recipe is removed rather than corrected", phase-3 fold), and question (2) was not re-counted after it. This is the same defect the phase-3 review already raised as [low] (r3739949511), recurring in the phase whose deliverable is the accuracy audit — which is the only reason it is worth saying twice.
It does not move the (a)/(b)/(c) decision: 166% of the ~1,000 aim rather than 164%. src_rust/src/lib.rs at 1,685 in question (6) is still correct.
There was a problem hiding this comment.
🤖 from Claude
Right on both counts — 1,661 at 5ca54d4, and this is the second time the figure has gone stale. Fixed in the body, and fixed in a way that should stop it recurring: question (2) now leads with wc -l mortie/geometry.py as the thing that matters, gives 1,661 at 5ca54d4 with the per-commit trace, and says to read the number off the file rather than off that line. It is re-counted at each phase push and nowhere else.
No code change; the (a)/(b)/(c) recommendation is unmoved at 166% of the aim, and src_rust/src/lib.rs at 1,685 in question (6) stays as it is.
| g = shapely.from_wkt(wkt) | ||
| for order in (1, 0): | ||
| for flavor in ("iso", "extended"): | ||
| blobs.append(shapely.to_wkb(g, byte_order=order, flavor=flavor)) |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] Every fixture stamps one dialect on the whole geometry, so the one axis the phase's headline finding is about — the part header — is never value-compared. Filling it in produces 7 value divergences against wkb 0.9.2 and 3 against geozero, on valid blobs, all in mortie's favour.
shapely.to_wkb(g, byte_order=order, flavor=flavor) writes a single byte order and a single dimensionality for the container and every part. The only hand-packed part-header case in the set is multipolygon/nested_ewkb_srid (SRID only). So of the 601,283 inputs I re-ran, 601,282 have parts whose header dialect matches their container's — while wkb.rs's own docstring calls the opposite out as a capability: "Nested geometries (the parts of a Multi*) carry their own header, so each part is read through this same function and may differ in byte order or dimensionality."
I packed 13 valid MultiPolygons varying only the part header and fed them to the unmodified binary this script builds:
blobs=13
wkb 0.9: agree_ok=5 oracle_stricter=1 VALUE_DIVERGENCE=7 (of 13)
geozero Ewkb: agree_ok=10 oracle_stricter=0 VALUE_DIVERGENCE=3 (of 13)
geozero Wkb: agree_ok=6 oracle_stricter=4 VALUE_DIVERGENCE=3 (of 13)
GEOS (shapely.force_2d(shapely.from_wkb(b))) parses all 13 and returns exactly what mortie returns — so mortie is the correct party in every one. Two concrete cases:
part2_iso_Z_valid— outer type 6 (2-D), part 2 declares ISO1003and carries real Z ordinates.- mortie / GEOS:
[(5,5),(6,5),(6,6),(5,6),(5,5)] wkb0.9.2 and geozero:[(5,5),(7,6),(5,7),(6,6),(7,5),(5,5)]— the Z ordinate read as the next vertex's x. Same for2003/3003, andwkbalone also fails the EWKB spellings0x80000003/0x40000003/0xC0000003.
- mortie / GEOS:
mixed_byte_order_parts— part 1 NDR, part 2 XDR. mortie, GEOS and geozero all return the right rings;wkb0.9.2 rejects it (Invalid buffer length for LinearRing: data would end at byte 1342177284, but buffer length is 84). That is also a counterexample to the body's unqualified "mortie is never the more permissive party" — here mortie is more permissive and right.
Why this matters for the report rather than for the code: the phase already found this behaviour, but only as an accident — one random mutation (blob 14971) landing an M flag on a part header, written up as a 1-to-5-blob curiosity under "Truncated". It is not a curiosity; it is a systematic difference reproducible on valid input, and it is the strongest evidence in the phase. As written, "zero value divergence across 601,283 inputs" reads as broad dialect coverage when the per-part axis contributes one blob to it.
Cost to close is small — fixture_blobs already hand-packs multipolygon/nested_ewkb_srid with struct; the same few lines with 1003 / 0x80000003 parts and an XDR part would move this from anecdote to a measured row.
There was a problem hiding this comment.
🤖 from Claude
Folded in 7ef59a6 — the axis is now a measured row, not an inference.
cross_part_blobs() hand-packs the 13 valid MultiPolygons (six per-part dimension spellings, mixed byte order, per-part SRID, SRID+Z, and the same-dialect controls), geos_rings() scores mortie against shapely.force_2d(shapely.from_wkb(b)) from Python, and main() runs it as its own cross_part set — kept separate from fixtures so the two grids stay distinguishable (what shapely.to_wkb can write vs what it cannot).
Your numbers reproduce exactly on my run:
blobs=13
wkb 0.9: agree_ok=5 oracle_stricter=1 VALUE_DIVERGENCE=7 (of 13)
geozero Ewkb: agree_ok=10 oracle_stricter=0 VALUE_DIVERGENCE=3 (of 13)
geozero Wkb: agree_ok=6 oracle_stricter=4 VALUE_DIVERGENCE=3 (of 13)
GEOS agrees with mortie on 13/13
Uncapped, the seven blobs any crate misreads are part2_{ewkb,iso}_{Z,M,ZM}_valid plus part2_srid_and_Z: wkb 0.9.2 misreads all seven, geozero Ewkb the three ISO spellings, geozero plain Wkb the three EWKB ones (and refuses four more). Decoded part2_iso_Z_valid independently — part 2's header is 01 eb 03 00 00 (type 0x3eb = 1003), 1 ring / 5 vertices, 120 bytes of ordinates = (5,5,7),(6,5,7),(6,6,7),(5,6,7),(5,5,7); read as 2-D pairs that is [(5,5),(7,6),(5,7),(6,6),(7,5)], which is what both crates return, against the authored square from mortie and GEOS.
The PR body now leads with this: the headline table's "zero value divergence" is scoped to the 601,283 same-dialect inputs, a new "per-part dialect axis" section carries the row and both concrete cases, and the verdict claims correctness on a class of valid input rather than one mutated blob. The unqualified "mortie is never the more permissive party" is narrowed to malformed input, since mixed_byte_order_parts is exactly the counterexample you name.
| for i in range(n_mutations): | ||
| src = bytearray(seeds[i % len(seeds)]) | ||
| for _ in range(int(rng.integers(1, 4))): | ||
| src[int(rng.integers(0, len(src)))] = int(rng.integers(0, 256)) |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] The [5, 6] / "dropping 110 declared vertices" figures belong to one blob, not to "the other two cases" — three of the four return [5, 5].
I enumerated every blob in this fuzz set where mortie's error starts with truncated and an oracle nonetheless accepts (all 45,300, uncapped rather than the 6-example cap):
[14971] mutate/14671 mortie Err(5 vertices need 120 bytes at offset 115, 80 remain)
wkb Ok([5,5]) geozero Ok([5,5])
[21769] mutate/21469 mortie Err(1075052544 vertices ... 72 remain)
wkb Err(...) geozero Ok([5,5])
[22297] mutate/21997 mortie Err(1075052544 vertices ... 72 remain)
wkb Err(...) geozero Ok([5,5])
[28309] mutate/28009 mortie Err(1075052544 vertices ... 72 remain)
wkb Err(...) geozero Ok([5,5])
[33637] mutate/33337 mortie Err(116 vertices need 2784 bytes at offset 119, 76 remain)
wkb Err(...) geozero Ok([5,6])
Five blobs, two distinct shapes — consistent with the truncated=1 / truncated=5 cause counts. But [5, 6] and "dropping 110 declared vertices" describe only blob 33637 (116 declared, 6 returned). The 1,075,052,544-vertex shape occurs three times and geozero returns [5, 5] for each — a larger fabrication (>1e9 vertices declared, 5 returned), not the one quoted.
The headline case checks out byte-for-byte, so this is presentation only. Blob 14971 is 195 bytes; part 2's header sits at offset 102, 01 03 00 00 40 = 0x40000003 = Polygon + EWKB M, i.e. the mutated byte at offset 106 is the flag; ring count and vertex count follow at 107 and 111; 80 bytes remain from offset 115 against the 120 that 5 XYM vertices need. Dumping the coordinates both crates return for that part gives [(5,5),(6,5),(6,6),(5,6),(5,5)] — the raw byte pairs — confirming both read it as 2-D and ignored the declared M. mortie's refusal is right, exactly as claimed.
There was a problem hiding this comment.
🤖 from Claude
Corrected in the PR body (presentation only, no code change — the enumeration is already reachable with DIFF_EXAMPLES=).
Divergence item (5) now prints all five blobs and names both shapes: [5, 6] and "116 declared, 6 returned — 110 dropped" are attributed to blob 33637 alone, and the 1,075,052,544-vertex shape is called out as occurring three times with geozero returning [5, 5] for each — the larger fabrication, which is now described as such instead of being folded under the quoted numbers. Blob 14971 keeps the byte-for-byte decode you confirmed (header 01 03 00 00 40 at offset 102, 80 bytes remaining against 120 needed), and it is also now cross-referenced to the new per-part dialect section, since it is the accidental instance of the systematic finding folded from r3740351968.
| "[{i}] {name}: VALUES differ ({} vs {} rings)", a.len(), b.len())); | ||
| } | ||
| (Err(_), Err(o)) if o.starts_with("unsupported geo type") => { | ||
| self.agree_unsupported_type += 1 |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] The report's "agree" column folds agree_unsupported_type back into agreement — the exact inflation this bucket exists to prevent — and the malformed row then doesn't sum to its blob count.
This arm is reached when the crate parsed the blob successfully and only the harness has no ring form for the result (Point / MultiPoint / GeometryCollection), while mortie refused the blob. Directionally that is a mortie-stricter outcome, not an agreement; the module docstring and the report both say so, and then the table counts it as agree.
My re-run, per corpus:
| corpus | oracle | agree_ok | agree_err | agree_unsupported_type | mortie_stricter | oracle_stricter | divergence |
|---|---|---|---|---|---|---|---|
| fixtures (116) | wkb 0.9 |
86 | 0 | 15 | 15 | 0 | 0 |
| malformed (45,300) | wkb 0.9 |
12,453 | 12,816 | 151 | 19,880 | 0 | 0 |
| corpus (555,867) | wkb 0.9 |
555,867 | 0 | 0 | 0 | 0 | 0 |
The buckets are exhaustive and sum to the blob count on every corpus, which is the important property — but:
- the fixtures row's published
agree 101is86 + 15, and those 15 are precisely thepoint/multipoint/geomcollectionfixtures (3 types × 5 spellings) that mortie refuses by name and every crate parses; - the malformed row's published
12,453 ok + ~12.8k err+~19.9kmortie-stricter is45,149, 151 short of45,300— the missing 151 is this bucket, which the row omits entirely.
Printing the bucket and then dropping it from the table costs the report the one number that shows how much of "agree" is a genuine comparison. Suggest carrying it as its own column (or adding it to mortie_stricter, which is where it belongs directionally) rather than into agree.
There was a problem hiding this comment.
🤖 from Claude
Fixed in the PR body — the harness was always right (it prints the buckets separately), the table was not, so the table now matches what the harness prints.
The headline table carries every bucket as its own column, for all three oracles on all four corpora, and every row sums to its blob count. agree_unsupported_type is no longer inside agree, and the text says what the arm is (crate parsed, harness has no ring form, mortie refused — directionally mortie-stricter) rather than leaving it to be inferred. My re-run matches yours, and adds the two geozero oracles:
| corpus | oracle | agree_ok | agree_err | agree_unsupported_type | mortie_stricter | oracle_stricter | divergence |
|---|---|---|---|---|---|---|---|
| fixtures (116) | wkb 0.9 |
86 | 0 | 15 | 15 | 0 | 0 |
| fixtures (116) | geozero Ewkb |
86 | 0 | 15 | 15 | 0 | 0 |
| fixtures (116) | geozero Wkb |
68 | 6 | 12 | 12 | 18 | 0 |
| malformed (45,300) | wkb 0.9 |
12,453 | 12,816 | 151 | 19,880 | 0 | 0 |
| malformed (45,300) | geozero Ewkb |
12,453 | 12,529 | 21 | 20,297 | 0 | 0 |
| malformed (45,300) | geozero Wkb |
12,453 | 13,649 | 8 | 19,190 | 0 | 0 |
| corpus (555,867) | all three | 555,867 | 0 | 0 | 0 | 0 | 0 |
So the fixtures row is 86, not 101, and the malformed rows sum to 45,300 with the 151 (and 21, and 8) visible rather than dropped. Kept as its own column rather than merged into mortie_stricter, so the size of the genuine-comparison bucket stays readable — that being the number you point out the old table cost the report.
|
|
||
|
|
||
| def _poly(pts, le=True, extra=(), typ=3, srid=None): | ||
| """Hand-pack one Polygon, header included, in either byte order. |
|
|
||
|
|
||
| def cross_part_blobs(): | ||
| """Build the per-part dialect set: the part header varies, the container's does not. |
There was a problem hiding this comment.
|
🤖 from Claude All five phases complete, all five review folds landed, zero unanswered inline threads. CI: 16 success, 0 failing; only the ~27-minute CodSpeed Rust benchmarks job outstanding (it runs that long on What this PR ended up provingThe issue asked for a backend-free WKB ingest. It delivered that — Against two independently-written crate implementations, over 601,283 inputs: zero value divergence, and Both crates silently misread per-part dialect flags on VALID input. On 13 hand-packed valid MultiPolygons whose parts carry differing dialects, GEOS agrees with mortie 13/13, The cost of that edge is ~1.35× on parse, which is ≤2.3% of end-to-end (148.5 ms of ~6,600 ms for the full What the review cycles caught that the phases did notWorth recording, since it is the case for the process: a heap OOB that aborted the interpreter where the scalar raised catchably; a Standing for espgQuestions (1)–(4) in the body, of which the live one is (2): Merging this unblocks two things at once: the release (which #160's |
1 participant
Closes #157
What
A Rust WKB reader (
src_rust/src/wkb.rs), so mortie's geometry ingest stops going through a Python geometry backend. Before this,from_wkb→geometry_from_wkb(backend decode) →from_geometry→decompose→_require_shapely— which meant WKB ingest hard-required shapely, and_require_backend's own message ("mortie's runtime is numpy-only, so the backend is an optional extra") was untrue for anyone who touched WKB. The reader parses the bytes directly and hands the coverage kernels the rings they already expect;from_wkbis rewired onto it, and the plural batchfrom_wkbsis built on the same parser.Scope is the ingest direction only.
_geometry_from_wkbkeeps its backend — it returns a backend geometry object, which is its entire purpose — andfrom_geometry/decomposeare untouched, so existing callers see no behaviour change.Approach
decomposedocuments (mortie/geometry.py:272-330): the exterior and interior rings of every part, flattened into one ragged array in arrow list layout. mortie's even-odd descent then unions disjoint outers and carves holes in the one pass, as it does today. Vertices are kept as authored — winding untouched, the closing vertex not stripped, and an empty part still contributes its empty ring.(x, y) = (lon, lat); mortie's kernels take(lats, lons). The swap happens once, inread_points, and is pinned by an asymmetric fixture — a footprint with latitudes in[-75, -71]and longitudes in[10, 40], ranges that cannot be confused — asserted in both directions (every lat lands in the lat range, every lon in the lon range) in Rust, and against shapely's own decomposition of the same geometry in Python._strip_ewkt_sridon the WKT side; Z/M dropped, read from either spelling — ISO's+1000/+2000/+3000type offsets or EWKB's0x8000_0000/0x4000_0000flag bits.ValueErrorrather than an arbitrary allocation. Truncation at every prefix of a valid blob is an error, never a panic (asserted exhaustively, in both Rust and Python). Point / MultiPoint / GeometryCollection are refused by name; an empty geometry raises withdecompose's exact wording (empty geometry has no coverage).Dependency decision
Hand-rolled, no crate (
wkb/geozero/geo-types) — espg's ruling, recorded on #157: no crate enters mortie's shipped dependency list. The parser is ~380 lines against a small, stable, well-specified format, and nothing new appears incargo tree.The crates still get used — as oracles, in phase 5: differential-test the hand-rolled reader against a mature crate over the real 555,867-granule corpus, the edge fixtures, and fuzzed/malformed blobs (comparing error behaviour there), plus a parse-throughput comparison. That comparison must leave no trace in the shipped crate — an off-by-default feature under
[dev-dependencies], or a scratch crate outside the repo, withcargo treefor a default build confirmed unchanged.Phases
src_rust/src/wkb.rs+ therust_wkb_ringsbinding, ring contract and coordinate-order swap pinned (45276df)from_wkbgoes backend-free. Routed through the reader instead of_geometry_from_wkb+decompose;_geometry_from_wkb/from_geometry/decomposeunchanged (8140908), plus the codec quartet privatized (03c265a)from_wkbs. The plural batch: many blobs → ragged MOCs in one call, chunked copy-then-release, lowest-index fail-fast, with the corpus parity and bench numbers below (5014c4b)Phase 2 — what changed
from_wkbno longer decodes through a backend. The routing tail it shared withfrom_geometryis extracted into_cover_parts, so the two entry points differ only in how they reach(kind, parts)— a backend geometry viadecompose, or bytes via the Rust reader — and cannot drift apart:The headline test of the issue lands here (
mortie/tests/test_wkb_no_backend.py): asys.meta_pathfinder makesshapelyandspherelyunimportable, already-imported copies are pulled out ofsys.modules, and the cached_BACKENDis cleared — thenfrom_wkbcovers polygons, holes and linestrings, in flat andmoc=Trueform, withtolerance/max_cells, and raises itsValueErrors, all with no geometry library reachable. Every blob in that module is packed by hand withstruct, so the test itself has no geometry-library dependency either. A guard test asserts the block is genuinely in force, so the headline cannot pass for the wrong reason. Pinned alongside it:from_wktandto_geometrystill raiseImportErrorunder the block — that is #157's scope line, not an oversight.Behaviour is pinned as unchanged across the swap: 8 input classes (polygon, hole, multipolygon, antimeridian-crossing, pole-adjacent, linestring, multilinestring, Z) × both byte orders × flat/
mocare asserted equal tofrom_geometry(shapely.from_wkb(blob))— the path this replaces, whose tail is untouched — plus EWKB/SRID and the refusal classes. The input contract is pinned separately, because it is not pure parity — see the phase-2 fold below: a hex string is parity (the backend path took one), a byte buffer is a deliberate widening, and an iterable of ints is refused.Privatizing the codec quartet (espg ruling, 2026-08-07):
geometry_from_wkb/geometry_from_wkt/geometry_to_wkb/geometry_to_wkt→_-prefixed. They are two-line pass-throughs to the backend's own codec with backend dispatch and no mortie logic of their own, and they were already absent frommortie/__init__.py's exports, with zero external callers across zagg and moczarr. Re-exporting another library's codec under a mortie name buys nothing — a caller who wants a shapely object callsshapely.from_wkb. Blast radius confirmed internal-only by grep before and after:mortie/geometry.py(9 references),mortie/tests/test_geometry.py(13),mortie/tests/test_wkb_no_backend.py(1), and nothing inmortie/__init__.py,docs/,USAGE.md, or any notebook.Review fold (phase 1)
Adversarial-review findings folded, one commit each. Both were leniency edges where the reader was more permissive than the path it replaces — which matters precisely because phase 2 makes it the default:
a6574d9— unclosed polygon rings rejected (r3739676202). GEOS enforces ring closure at parse time, so these raise today; verified the cover is identical either way, so rejecting costs no capability. Polygonal only (lines are open by nature), the ring named by its flattened index, and a NaN endpoint reads as unclosed — which is what GEOS reports for it too.9adee4e— an empty MultiPolygon part keeps its empty ring (r3739676235).decomposesees an empty Polygon part as a zero-length exterior ring, which is what trips the downstream 3-vertex refusal; dropping it would have turned aValueErrorinto a silent cover. It also makes the polygonal path agree with the linear one, which already emitted the empty component.The review found no other divergence: 555,867 / 555,867 ATL03 v007 blobs byte-identical to the shapely oracle, plus 20,000 grammar-generated dialect blobs, 21,000 randomized geometries, and 400,000 fuzzed blobs with zero panics.
Review fold (phase 2)
Both findings were on the input contract of
from_wkb, not the parser — the review's independent re-run found no other divergence in the phase (555,867/555,867 ATL03 v007 blobs identical at ring level and end-to-end, orders 4/6/9/12,normalize=False,moc=Trueat o6/o9/o13/o16,tolerance=/max_cells=, the full dialect matrix, and 200,000 mutation-fuzzed blobs withwidened=0/diff_cells=0).9b05e06— explicit input contract, replacing the barebytes(data)(r3739755714). That one line had moved the contract in both directions. New_wkb_bytesdispatches by type:bytesthrough; a hexstrviabytes.fromhex(restored —shapely.from_wkbdocuments "the WKB byte object or hexadecimal string", sofrom_wkb(blob.hex())worked before phase 2 and had started raising CPython'sTypeError: string argument without an encoding); any one-byte-item buffer (bytearray/memoryview/ auint8array) viamemoryview.tobytes(); everything else refused by name with mortie's ownTypeError— which closes the dangerous half,list(blob)/tuple(blob)decoding to a plausible-looking cover becausebytes()assembles a blob out of any iterable of ints. A buffer whose items are wider than a byte (e.g. afloat64array) is refused for the same reason. Malformed hex is aValueError, matching the reader's other parse failures.1615d63— the widening is labelled as a widening in the tests (r3739755855).test_from_wkb_accepts_a_bytearraysat under "unchanged for every input" while pinning a new capability. Replaced bytest_from_wkb_accepts_byte_buffers_a_deliberate_widening, which covers all three buffer spellings and asserts in the same test that the backend path raisesTypeErrorfor each — the change is now visible in the test itself. Added alongside:test_from_wkb_accepts_a_hex_string_as_the_backend_path_did(parity againstfrom_geometry(shapely.from_wkb(hex)), both cases, plus the non-hexValueError) andtest_from_wkb_refuses_non_byte_input_by_name(list / tuple /float64array /int/None).test_the_input_contract_is_backend_free_toopins hex, buffer and refusal under the no-backend block, since the coercion must not reach for a geometry library either.Contract chosen, and why: the widening is kept, deliberately and documented rather than held to the old bytes-or-str line. A byte buffer is a genuine WKB blob by any reading, zagg's arrow-backed consumer (issue #157's phase-3 consumer, englacial/zagg#400) hands over buffers at its boundary, and refusing them would buy nothing a caller cannot get with
bytes(...). What is not kept is the accident that came with it: an iterable of ints is not a blob, and silently covering one is a wrong-column bug that used to be aTypeError.from_wkb's docstring now states all three arms.Phase 3 —
from_wkbsResult
iis byte-identical tofrom_wkb(blobs[i], order=order, moc=True). The plural marks many→many — one MOC per blob — against the many→one unionfrom_wkbperforms over the rings inside one blob; unlikepolygons_to_morton_mocs, an entry may therefore be multipart and may carry holes.Input shape: a sequence of blobs, with the scalar's accept list per item, unnarrowed. Each entry goes through the
_wkb_bytescontract landed in the phase-2 fold —bytes, a hexstr, or any itemsize-1 buffer — so a list ofbytes, a numpy object array (whatpandas/pyarrowgive for a binary column),bytearray,memoryviewandnp.uint8arrays all work as they are, and are asserted equal to each other in the tests. Rejected alternatives: a flat buffer + offsets pair would be zero-copy from a pyarrowBinaryArray, but it would be a second, different input grammar for the same operation and would force every non-arrow caller to assemble it by hand; and it is not needed to hit the memory target, because the copy is already bounded by the chunk. A pyarrow-typed entry point belongs inmortie.arrowbesidemortie.arrow.polygons_to_morton_mocs, which is where #154 put the arrow face — flagged as question (5) rather than smuggled in here.Chunked copy-then-release, because
parseneeds&[u8]and a&[u8]borrowed from a Pythonbytesis GIL-bound — it cannot crosspy.allow_threads. So per chunk of 2048: copy the blobs into one contiguous buffer with the GIL held, release the GIL,par_iterparse+cover the chunk, assemble into the ragged output, reuse the buffers, repeat.Py<PyBytes>handles were rejected — they would force re-acquiring the GIL for every blob inside the parallel region.Memory posture, measured, not asserted (the omission #162 is about). The true model is result + one chunk of copied input bytes + one chunk of in-flight covers, and the docstring says exactly that including the mandatory input copy. On the real corpus — 555,867 ATL03 blobs, 290.1 MB of WKB, mean 522 B/blob:
The column's 290 MB of bytes are never copied wholesale: the input-copy term is one chunk, ~1 MiB at 522 B/blob. (That figure is the re-measured one — a 1 ms-sampled peak against the resident post-load baseline, the methodology the phase-3 review used. The
ru_maxrss-delta this section originally quoted, 164.0 MiB / 0.98×, understates it. The full per-spelling table is in the fold below.)Lowest-index fail-fast, by the #154 mechanism: per-blob results are materialized in index order and scanned in that order before any is allowed to fail the call, and chunks run in index order, so rayon's schedule cannot change which error surfaces.
basecarries the caller's global numbering into later chunks, so an offender in chunk 2 is stillblob 4217, notblob 121. Two ordered gates, stated in the docstring: the input contract is screened by a serial pre-pass over the whole sequence, then blobs are parsed and covered — each gate reporting its own lowest-index offender.Polygonal only, per question (3), now resolved: linear geometry is refused by index (
blob 3: linear geometry (LineString / MultiLineString) has no ragged MOC form...), because a LineString cover is one array per line and has no single-MOC-per-blob spelling.from_wkbstill covers them.Ring validation mirrors
mortie.coverage._prep_ringsword for word (the 3-vertex minimum, the finiteness check), so a blob refused by the batch is refused with the same message the scalar uses for the same geometry.Acceptance (phase 3)
Parity — the full corpus, per blob:
Every one of the 555,867 MOCs is byte-identical to
from_wkb(blob, moc=True)on the same blob, and the ragged contract holds exactly (offsets[0] == 0,offsets[-1] == len(values),len(offsets) == n + 1).Bench, the issue's ≥100k acceptance workload (
python benchmarks/measure_batch_wkb.py 100000 8, Apple Silicon, 10 cores):from_wkbsThe per-call fixed term is what disappears (103.2 → 5.5 µs/blob); the real-corpus 12.41× is lower than the synthetic 18.83× because ATL03 footprints are ~5.6× larger than the synthetic quads, so a greater share of the work is covering rather than crossing the boundary — which is the point.
Determinism: offenders placed at indices 0, 1, 2047, 2048, 2049, 4103 and 6143 (either side of every chunk boundary) are each named by their global index; with three offenders spread across three chunks the lowest wins on 25 consecutive runs, and with two adjacent offenders in one chunk the lowest wins on 25 more. Seven failure classes (truncated, unclosed, short ring, NaN, linear, empty, unsupported type) each name their blob, and the two input-contract violations name theirs.
Backend-free: the headline test is extended to the batch — 2,200 blobs (crossing a chunk boundary, so the chunked loop runs, not just its first iteration) covered identically with
shapelyandspherelyboth unimportable, refusals included.Phase 4 — the basins, and the acceptance pass
The Antarctic basins through
from_wkbsThe in-tree fixture (
mortie/tests/Ant_Grounded_DrainageSystem_Polygons.txt) is a lat/lon/basin-id table, sotest_wkb_basins.pypacks it into WKB and runs it through the batch. It is the fat blob class the phase-3 byte-cap was added for — measured, not assumed: 27 basins, 0.65 MiB median, 1.25 MiB max, 18.9 MiB total, 42,919 vertices median / 81,595 max. (Two of the 27 rings are left open by the fixture and are closed when packed — a WKB ring is closed by definition, and the reader enforces that, matching GEOS.) A test pins those sizes, so the numbers quoted against this fixture cannot go stale silently.from_wkb, and 27/27 identical to the shapely-backed path (from_geometry(shapely.from_wkb(blob))) — the criterion is parity with the path the reader replaced, not with itself.blob 100.The byte cap's real-fixture number (
test_the_byte_cap_holds_on_the_real_antarctic_basins,@slow, subprocess,RAYON_NUM_THREADS=2). 594 basins = a 416 MiB column:And the peak grows far sublinearly with the column — the point of the cap. Every cell is three runs of the in-tree helper on Apple Silicon, min / median / max,
--release,RAYON_NUM_THREADS=2, order 6:A 66× column buys ~3.4× peak growth (81.5 → 276.8 MiB by median), and a 1,248 MiB column never went above 334 MiB in any run. What is left is the in-flight cover work, which is bounded by thread count rather than by the chunk — exactly what
CHUNK_BYTES' doc comment says, now with a real fixture behind it rather than only the synthetic 3,000-blob case.These numbers replace the ones this section first carried, and two of its claims are withdrawn (phase-4 review fold, r3740223445; helper fix in
7d5784e). The old figures baselined againstru_maxrssafter the column build — a process-lifetime high-water, which on this fixture (np.loadtxtover 1,239,001 lines) sits a run-dependent 0–28.4 MiB above what is actually resident when the call starts, so every published growth was understated by an unstable amount.growth_mibnow baselines against resident RSS and returns the minimum of three runs, withgrowth_samplesexposing the individual runs for a table like the one above. Withdrawn with the old numbers: (1) "the peak does not track the column at all" is false — it tracks it, just far sublinearly, which is the claim the cap actually earns; (2) the 416 MiB dip was read as a property of that column size, and it is not — the review reproduced non-monotonicity in a different row, and in the table above the 624 MiB median sits level with 416 MiB while single runs of the same column span 243–585 MiB. The ordering across the middle rows is run-to-run noise. Theassert growth < 350.0threshold is unaffected: the minima above are 243 MiB at the asserted size, against 416 MiB that an uncapped copy could not avoid.Consolidated acceptance pass against issue #157
benchmarks/verify_wkb_corpus_parity.py: 555,867/555,867 reader-vs-shapely and 555,867/555,867 batch-vs-scalar, 290.1 MB payload, order 6test_geometry.py::test_from_wkb_is_unchanged_by_the_rust_reader(10 classes × 2 byte orders × flat/moc),…_ewkb_and_srid_are_unchanged,test_wkb_reader.py::test_z_and_m_are_droppedantimeridian/pole_adjacentclasses in the same matrix carry the criterion; the basin fixture corroborates it at fat sizes, measured rather than asserted — of the 27, 3 reach below -85° (min -88.4°) and one crosses the antimeridian, now pinned bytest_wkb_basins.py::test_how_polar_and_how_antimeridian_the_fixture_actually_is(09d0985)test_wkb_basins.py, 27/27 against both oraclestest_wkb_no_backend.py(9 tests):shapelyandspherelyblocked viasys.meta_path, scalar + batch + refusals, with a guard test proving the block is in forceValueErrornaming the problemtest_wkb_reader.py(truncation at every prefix, bad byte-order flag, unsupported types, three empty spellings, lying ring/part counts)test_wkb_batch.py: 7 failure classes each named by index, offenders at 0/1/2047/2048/2049/4103/6143, lowest-of-several wins on 25 consecutive runsfrom_wkbsbenchmarked vs the per-blob loop at ≥100k blobsbenchmarks/measure_batch_wkb.py: 18.83× at 100k synthetic, 12.41× on the real 555,867-blob corpusfrom_geometrygeometry_from_wkb_geometry_from_wkb(03c265a, espg ruling, zero external callers verified across zagg and moczarr). Flagging rather than ticking silently — see question (1)What the pass exposed
Two gaps, both now closed, and one item that was never a gap — corrected below after the phase-4 review, which is the kind of thing an accuracy pass exists to catch. Each passed on first run; the defect was that nothing pinned them, not that anything was broken:
benchmarks/verify_wkb_corpus_parity.py, which checks both claims (reader-vs-shapely and batch-vs-scalar), takes any parquet with a WKB column, and has a--sample Nmode. Re-running it is how the top row of that table gets re-verified after any change. This is the gap I would most want closed, because it is the criterion the whole issue turns on.ZandZMbut notM, so neither of M's two spellings was reachable from Python. What the phase added to the scalar matrix and the batch dialect list is EWKB's flag0x40000000only, becauseshapely.to_wkbdefaults toflavor="extended"; ISO type 2003 stayed pinned nowhere outsidewkb::tests. Closed properly in2a1edc1by addingPOLYGON Mtotest_wkb_reader.py::test_z_and_m_are_dropped, which is already parametrized over both flavors — verified to emit type word0x7d3(2003) underflavor="iso"and0x40000003under"extended"(r3740223783).Not a gap, and the list said it was (r3740223804): multipart-with-holes was already pinned from Python.
_WKB_INPUT_CLASSES["multipolygon"]has been multipart and holed since phase 2, andtest_wkb_reader.py::test_polygon_with_hole_and_multipolygon_flatten_every_ring— phase 1, Python, shapely as oracle — asserts that geometry ring for ring. The newmultipolygon_with_holeclass is kept because it does add coverage the old one lacked: the old hole is 0.6° inside a 1° part and changes no cells at order 6, so at the cover level that class never exercised its own hole, while the new 10°×2° hole moves the cover and sits in the first part. That is what it adds; it closed nothing that was open.Phase 5 — differential validation against the crates
espg's direction was to build it first and then compare. So the crates are used as oracles, not candidates: a mature, widely used WKB implementation is a far better source of truth than hand-written expectations.
benchmarks/compare_wkb_crates.pygenerates the harness into a temp directory outside the repo, copiessrc_rust/src/wkb.rsverbatim (minus itspub mod batch;line, which pulls in rayon) so the parser under test is the real one and cannot drift, and runs three oracles:wkb0.9.2 (georust) — the focused reader, viageo-traits→geo-types.geozero0.15.1 in itsEwkbdialect (falling back toWkb), the closest equivalent to mortie's single entry point.geozeroin plainWkb, kept separate so the dialect finding stays measurable.Comparison is bit-exact (
f64::to_bitson every coordinate), so an axis swap or a lost ulp would surface. Four blob sets, kept separate because they measure different things: the real corpus (555,867 ATL03 v007 footprints), the edge fixtures (the dialect gridshapely.to_wkbcan write — one dialect stamped on the whole geometry), the per-part dialect set (cross_part, the grid shapely cannot write — hand-packed parts whose header differs from their container's, added in the phase-5 fold below), and the fuzz corpus (truncations, mutations, noise).Correctness — the headline
Every bucket, per corpus and per oracle. The buckets are exhaustive: each row sums to its blob count, so nothing is hidden in a rounded "agree".
agree_unsupported_typeis carried as its own column and not folded into agreement — it is the arm where the crate parsed the blob and only this harness has no ring form for the result (Point / MultiPoint / GeometryCollection) while mortie refused the blob, i.e. directionally mortie-stricter.wkb 0.9geozero Ewkbgeozero Wkbwkb 0.9geozero Ewkbgeozero Wkbwkb 0.9geozero Ewkbgeozero Wkbwkb 0.9geozero Ewkbgeozero WkbZero value divergence on all 601,283 inputs whose parts carry their container's dialect — every real footprint, every fixture, and every malformed/fuzzed blob: whenever mortie and an oracle both accept, the rings are bit-identical. And
oracle_stricteris 0 on the fuzz corpus against all three oracles: there is no malformed input any crate rejects that mortie accepts.Every value divergence that exists is on the per-part axis, and mortie is the correct party in all of them — see below.
The per-part dialect axis — where the crates and mortie actually differ
shapely.to_wkb(g, byte_order=…, flavor=…)writes one byte order and one dimensionality for the container and every part, so the fixture grid above cannot reach the axiswkb.rsdocuments as a capability: "Nested geometries (the parts of a Multi*) carry their own header, so each part is read through this same function and may differ in byte order or dimensionality." Thirteen valid MultiPolygons, hand-packed to vary exactly that (cross_part_blobs()), through the same unmodified harness:GEOS is the third opinion, because with the two crates disagreeing with mortie the direction needs a party outside the harness:
shapely.force_2d(shapely.from_wkb(b))parses all 13 and returns exactly what mortie returns on every one — so mortie is right in every divergence. Seven distinct blobs are misread by at least one crate: the six per-part dimension spellings (ISO1003/2003/3003, EWKB0x80000003/0x40000003/0xC0000003) andpart2_srid_and_Z.wkb0.9.2 misreads all seven; geozeroEwkbmisreads the three ISO spellings; geozero plainWkbmisreads the three EWKB ones and refuses four more. Two concrete cases:part2_iso_Z_valid— container type 6 (2-D), part 2 declares ISO1003and carries real Z ordinates. Part 2's header is01 eb 03 00 00= type0x3eb= 1003, 1 ring, 5 vertices, and 120 bytes of ordinates follow — 15 doubles,(5,5,7), (6,5,7), (6,6,7), (5,6,7), (5,5,7).[(5,5),(6,5),(6,6),(5,6),(5,5)]— the authored square.wkb0.9.2 and geozero: the same 15 doubles read as 2-D pairs,[(5,5),(7,6),(5,7),(6,6),(7,5),…]— each Z consumed as the next vertex's x. Same for ISO2003/3003;wkbalone also misreads the EWKB spellings0x80000003/0x40000003/0xC0000003, which geozero handles.mixed_byte_order_parts— part 1 NDR, part 2 XDR. mortie, GEOS and geozero all return the right rings;wkb0.9.2 rejects it (Invalid buffer length for LinearRing: data would end at byte 1342177284, but buffer length is 84). That is the oneoracle_stricterin the table, and it is a case where mortie is more permissive and right — so the earlier unqualified "mortie is never the more permissive party" is narrowed to what was actually measured: never on malformed input.This is the phase's strongest result, and it was found by the phase itself only as an accident — one fuzz mutation landing an M flag on a part header, written up as a "truncated" curiosity. It is not a curiosity: it is systematic, on valid input, and reproducible as a measured row (r3740351968).
Every mortie-stricter divergence, and its direction
All on malformed or empty input, all mortie stricter:
Unclosed rings (17,644–17,660, the dominant cause). The crates accept them; GEOS does not, and mortie deliberately matches GEOS (the r3739676202 fold). Direction: mortie stricter, by an explicit decision already recorded.
Unrecognized type codes (603–1,672) and wrong part types (213–217) — e.g. a MultiPolygon whose part header declares a LineString. mortie refuses; the crates accept some of them.
Bad byte-order flags (200–711). mortie requires 0 or 1, as the OGC spec does. geozero accepts other values; the
wkbcrate mostly agrees with mortie here.Empty geometries (15 fixtures, 5 fuzz).
POLYGON EMPTYand friends parse fine for the crates; mortie refuses withempty geometry has no coverage, because a cover of an empty geometry is meaningless. This isdecompose's pre-existing wording, unchanged by this PR."Truncated" (1 blob against
wkb, 5 against geozero). The one category worth chasing, so every blob in it was decoded by hand — uncapped, not the 6-example default (r3740351987). Five blobs, two distinct shapes:Blob 14971 is the per-part case, verified byte-for-byte: 195 bytes, one mutated byte at offset 106, part 2's header at offset 102 reads
01 03 00 00 40=0x40000003= Polygon + EWKB M; 80 bytes remain from offset 115 against the 120 that 5 XYM vertices need, and both crates return that part's raw byte pairs, i.e. they read it as 2-D and ignore the declared M. mortie's refusal is right — and the systematic form of it is the per-part section above.The other four are geozero fabricating geometry out of a lying vertex count.
[5, 6]and "dropping 110 declared vertices" describe only blob 33637 (116 declared, 6 returned); the1,075,052,544-vertex shape occurs three times and geozero returns[5, 5]for each — a larger fabrication (>1e9 declared, 5 returned), not the one previously quoted for the category.Separately, and not a mortie divergence: geozero requires the caller to pick the dialect up front. Plain
geozero::wkb::Wkbrejects all 18 SRID-prefixed fixtures outright (geometry format), and 4 of the 13 per-part blobs. That is correct behaviour for a dialect the caller chose, not a handicapped oracle — but it is the cost of the design: mortie's single entry point accepts ISO and EWKB, with or without SRID, including a nested SRID on a MultiPolygon's part header.Performance
Parse-and-materialize into the same ring structure, 7 repeats, min/median/max:
wkb0.9.2Run-to-run spread is tight (corpus min–max 196.6–203.0 ms for mortie, 145.9–148.8 ms for
wkb) and the numbers reproduce across sessions within ~4% (an independent re-run measured mortie 197.4 ms,wkb144.5, geozero 494.4), so the ordering is real, not noise: thewkbcrate parses ~1.35× faster than mortie; geozero is 2.5–3.9× slower.Is the harness fair to the crates? It materializes every result into
Vec<Vec<(f64,f64)>>, which neither side needs internally, so the shared overhead could in principle flatter one implementation. Measured with the materialization split out: mortie 148.5 → 193.0 ms,wkb112.1 → 144.4 ms — ratios 0.755 parse-only vs 0.748 materialized. The overhead is proportional and does not distort the comparison.Is ~1.35× a serious gap? No, and the end-to-end number settles it. Parsing the entire corpus single-threaded is ~197 ms (148.5 ms parse-only) against ~6,600 ms for the whole
from_wkbscall over that corpus. Parsing is therefore ≤3% of end-to-end wall — the parse-only deficit is ≤2.3% — and closing the full gap would buy ≤1% overall, against acquiring a dependency, its transitive tree, and a second geometry model in the ingest path. Against espg's bar ("a modest performance difference is not a serious gap; a wrong answer on any real or edge input is"), this is squarely the modest side.cargo treeverificationThe comparison acquired no dependency. Three checks, all in the report because a single one would be weak:
Direct dependencies are unchanged:
arrow,healpix,numpy,pyo3,rayon,smallvec, pluscodspeed-criterion-compatunder dev. All three checks were re-run independently in review and hold.Methodology note, since this session has had several numbers undone by baseline choice. My first attempt compared
cargo treeagainst a fresh clone ofmainand produced a large diff — which was entirely registry drift (Locking 177 packages to latest compatible versionsre-resolvedlibc,serde,regex, … to newer versions).Cargo.lockis untracked, so a clone-based tree diff measures the day's crates.io state, not the branch. The three checks above are immune to that.Verdict
The hand-rolled parser should stand. It is bit-exact against two independent implementations on 555,867 real footprints and every same-dialect fixture, it is never more permissive than either crate on 45,300 fuzzed inputs, it is more correct than both on a whole class of valid input — 13/13 agreement with GEOS where
wkbdiverges on 7 and geozero on 3 — it handles a dialect (EWKB with SRID, including nested) that geozero requires a separate entry point for, and its ~1.35× parse deficit against thewkbcrate is ≤2.3% of end-to-end wall. Adopting a crate would trade a measured correctness edge for an unmeasurable speed-up and a new dependency.Testing
Phase 2 (cumulative):
cargo test --lib: 278 passed / 1 ignored (266 onmain; 12 inwkb::tests).pytest: 1114 passed, 16 skipped (1046 onmain; 68 new — 18 reader tests, 8 no-backend tests, 42 added totest_geometry.py).cargo fmt --check,cargo clippy --lib --benchesbaseline-differenced against unmodifiedmain(no new warnings; the pre-existing ones incoverage/tests.rs,prefix_trie.rs,geo2mort.rsare untouched),flake8 mortie --select=E9,F63,F7,F82,flake8 --max-line-length=88andnumpydoc linton every touched module — all clean.Phase 1:
geometry.decomposeon the same geometry — shapely as an oracle, never as a dependency of the path under test.Local note, pre-existing: plain
cargo testfails to link on macOS on unmodifiedmaintoo (extension-module; unresolved_PyBaseObject_Type/_Py_IncReffrom the doctest/bench targets) — verified by stashing this branch and rerunning.cargo test --libis unaffected and is what the counts above come from; CI on Linux is unaffected.Review fold (phase 3)
Both code findings were about the peak, not correctness — the review's independent re-run found the phase clean everywhere it looked: parity
555867/555867byte-identical (measuring 12.51× against the 12.41× claimed above, so this PR is the conservative number), 84 dialect combos plus EWKB-SRID-on-a-nested-part,tolerance=/max_cells=/normalize=Falsewith zero divergences, 660 determinism runs across mixed failure classes with gate ordering confirmed in both directions, the ragged contract including the degenerate zero-area ring keeping its slot, the linear refusal consistent with the scalar, backend-free proven non-vacuous, no leak over 300 calls, and thepub(crate)visibility change verified safe (four tokens, strictly less exposed than the already-pubtype they sit in).2774d82— the chunk copy is capped in bytes, not blob count (r3739949236).CHUNKis 2048 blobs whatever they weigh, so the contiguous copy was2048 × mean_blob_byteswith no upper bound: on 3,000 of this repo's ownAnt_Grounded_DrainageSystem_Polygonsbasins (1.25 MiB of WKB each, a 3.7 GiB column) that measured 3,119.5 MiB of peak growth for a 1.0 MiB result — worse than the scalar loop, which holds one blob at a time, and exactly what chunking exists to prevent. A chunk now ends atCHUNKblobs orwkb::batch::CHUNK_BYTES(64 MiB), whichever comes first; the rule is one function,chunk_full(blobs, bytes), asked before each blob is copied so a blob larger than the whole budget still forms a chunk of one.coverage::batchneeded no change, as the review said.bufalso stops growing by doubling once a chunk is heading for the budget — otherwise a 64 MiB chunk allocates 128 MiB and "one chunk of copied bytes" means twice what the budget says.Most of what is left is the in-flight cover work, not the copy: it is bounded by thread count, and a 16 MiB budget measures 409-618 MiB — indistinguishable — for 20% more wall, which is why the budget is 64 MiB and not smaller (the chunk is also the unit of parallelism; at 1.25 MiB/blob 64 MiB is 51 blobs). At footprint sizes the budget is never reached, so the ATL03 column chunks exactly as before.
e6621f4— non-bytesblobs are coerced per chunk, not in a pre-pass over the whole column (r3739948998). The finding reproduced exactly:_wkb_bytesreturns a newbytesfor everystr/bytearray/memoryview/uint8entry, and the pre-pass held all of them live for the whole call, so the documented "one chunk of copied input bytes" was true only forbytesinput — and false for precisely the arrow-backed buffer caller the phase-2 widening was added for.Fixed as the review proposed. The pre-pass is now
_wkb_bytes(blob, materialize=False): the identical accept list and the identical errors, but no blob-sized object retained — a hexstris still decoded (that is the only way to know it is hex, so an invalid hex string is still caught in the earlier gate, ahead of any parse error), and a buffer is screened onitemsizealone, sincetobytes()cannot fail once that holds. The byte-producing coercion moved into the Rust chunk loop, where thebytesit makes dies at the end of the iteration. So the documented gate ordering is unchanged: type errors and hex errors before parse errors, lowest-index-first in both.555,867 ATL03 v007 blobs (276.7 MiB of WKB, 167.3 MiB result, order 6), 1 ms-sampled peak against the resident post-load baseline, process-isolated:
list[bytes]strbytearraymemoryviewuint8array(The "before" column reproduces the review's table to within run-to-run noise.
uint8is the one spelling still above 1.07× — down from 3.12×, and stable across four runs.)Byte buffers are first-class, not merely tolerated — a buffer column costs the same peak a
bytescolumn does (the "arrow buffer slices" row, 1.07×, tied withbytes, where before this fold it was the worst row at 2.86×). That is the substantive finding and it stands.What does not follow is a call-site recipe. An earlier version of this note (and of the
blobsdocstring) taught a one-liner for cutting those slices out of a pyarrow column —mv = memoryview(col.buffers()[2])and[mv[o[i]:o[i+1]] ...]. It was wrong four ways, each failing silently or with wrong data rather than loudly, verified against pyarrow 25.0.0 and zagg's realcatalog_cycle22_small.parquet: (1) a parquet column reads back as aChunkedArray, which has no.buffers()at all —AttributeErroron the very column it was aimed at; (2)slice/takeare zero-copy metadata, so a chunk's buffers are the original array's —pa.array([b"AAA",b"BBBB",b"CCCCC",b"DDDDDD"]).slice(2,2)is logically[b"CCCCC", b"DDDDDD"]and the recipe returns[b"AAA", b"BBBB"]with no error, which matters because zagg'sfilter_bboxgoes throughtable.take; (3)large_binaryoffsets areint64, not the hardcodedint32; (4) a null entry spans zero bytes, so it becomes an empty WKB blob instead of being refused.The recipe is removed rather than corrected. A correct version needs chunk iteration,
chunk.offset, an int32/int64 branch and explicit null refusal — and a docstring that hands a caller twelve lines of that is worse than one that declines to, because it reads as blessed and gets copy-pasted without the caveats. The docs now name the four traps in one sentence (so nobody improvises), point at mortie.arrow.from_wkbs: arrow skin over the WKB batch, so a geoparquet column skips Python bytes objects #163 as the typed entry point that removes the problem, and tell callers who need it today to usefrom_wkbs(column.to_pylist(), ...)— correct on every case, at a cost that is measured rather than hidden: ~322 MB peak on the 555,867-row ATL03 column (290.1 MB of payload; 308.5 MB ofbytesobjects plus a 4.6 MB list). A measured cost beats a clever extraction that reads the wrong geometries.[low] the module-size figure (r3739949511) — correct, and body-only: no code change. Question (2) and question (6) above are updated, and the framing is now explicit that mortie's §4 aim is ~1,000 with no 1,200 raise (that is zagg's ruling, not this repo's).
Left standing deliberately, both raised and dismissed during the review rather than skipped silently:
reserve_estimatemis-sizing under skew reserves address space, not pages (RSS stays bounded at 2.36× and it never aborts — the doc comment is accurate); andorder=300raisingOverflowErrorrather thanValueErroris identical pre-existing behaviour inpolygons_to_morton_mocs, i.e. the house pattern, not a phase-3 divergence — changing it here would be a silent cross-cutting API change.Gates after the fold:
cargo fmt --checkclean;cargo clippy --lib --benches --all-targetsno new warnings (the six pre-existing ones are all incoverage/tests.rs,geo2mort.rs,prefix_trie.rs, unchanged);cargo test --lib286 passed / 1 ignored (+1, the chunk-rule unit test);pytest1157 passed, 16 skipped (+4: two batch tests, two memory tests);flake8 mortie --select=E9,F63,F7,F82clean,flake8 --max-line-length=88clean on every touched file,numpydoc lintclean. Full-corpus parity re-run on the folded build: 555,867 / 555,867 byte-identical, 21,375,174 cells, and the hex /bytearray/memoryview/uint8spellings each reproduce thebytesresult exactly. The ≥100k bench is unmoved — 17.4-18.9× across four runs against the 18.83× above, i.e. run-to-run noise, not a regression.Review fold (phase 4)
Phase 4's deliverable is an accuracy audit, so its review is mostly about whether the phase's own claims hold. Three corrections landed as commits and two are body-only; the verifications the review reproduced are recorded below, because on a consolidation phase they are the value.
7d5784e— the peak-memory helper baselines on resident RSS, and quotes repeats (r3740223445). The finding reproduced exactly:ru_maxrssafter the column build sits 28.4 / 28.3 / 0 / 28.4 MiB above resident RSS at the same instant, so it was subtracting anp.loadtxttransient from the growth.growth_mibnow baselines against resident RSS (/proc/self/statmon Linux,ps -o rss=on macOS — no new dependency) and returns the minimum of three runs;growth_samplesexposes the individual runs. The byte-cap table above is re-measured with it, and the two claims that did not survive re-measurement are withdrawn there, in place. The tail is real and worth stating — the same 416 MiB column measured 243, 246 and 585 MiB in three consecutive runs.1ba8d42— the peak is sampled on Linux, becauseru_maxrssis not a per-process figure there — and it turns out these assertions had been vacuous on CI all along. Pushing7d5784eturned the Linux test matrix red at 968.0 and 947.2 MiB of "growth" for two workloads a factor of 20 apart in column size. The 20.8 MiB between them is exactly the difference in their columns' resident size, which identifies the peak as a constant neither workload produced: on Linuxru_maxrsssurvivesexecve, so a child spawned from a pytest process that has already peaked at ~1 GiB reports that 1 GiB as its own high-water before running a line of the body. Baselining onru_maxrsscancelled it — and with it cancelled the measurement, because the child's own peak never rose above the inherited one. On Linux the peak is therefore sampled: a 1 ms poller thread over/proc/self/statmfor the duration of the call, which is the interval in which the GIL is released and the previous chunk's copy is still resident. macOS keepsru_maxrss, where a spawned child does start fresh (measured: high-water 28.4 MiB above resident after the build, not 800). The matrix is green on 3.10 / 3.11 / 3.12 with the sampler, and the thresholds are unchanged — so the Linux numbers now sit under the same bounds the macOS ones do, where before they were not being tested at all.2a1edc1— ISO type 2003 pinned from Python (r3740223783). Detail in "What the pass exposed" item (2) above. Cheap to close and the phase had claimed it closed, so it is closed rather than reworded.09d0985— the basin fixture described in measured counts (r3740223821). "All pole-adjacent, several antimeridian-crossing" is not this fixture. Measured over the 27 basins: 3 reach below -85° (min -88.4°, max -63.2°), 11 never reach -76°, exactly 1 has a >180° longitude step, and 2 have vertices on both sides of |lon| > 170°. The module docstring now says that, the acceptance table's third row is corrected to match, and a new test pins the three counts so the description cannot drift from the fixture again.What the review verified sound, independently reproduced — the phase's actual value, so it is recorded rather than assumed:
benchmarks/verify_wkb_corpus_parity.pyover the full 555,867-row column:batch vs scalar 555867/555867,reader vs shapely 555867/555867, 21,375,174 cells — this PR's figure to the cell.--samplespreads withnp.linspacerather than taking the first N;--columnworks on a differently-named column.IllegalArgumentException: Points of LinearRing do not form a closed linestring), so packing the fixture's two open rings closed is the only way to get valid WKB out of that table.geometry_from_wkbacceptance row is honest, not an understatement. Never exported, absent fromdocs/api/geometry.md'smembers:list, zero hits across zagg and moczarr; the only real break isfrom mortie.geometry import geometry_from_wkb, which is what the row says.5ca54d4are success, "Python benchmarks" included — the earlier red was transient infrastructure.benchmarks/measure_batch_wkb.py 100000 8re-measures 18.76× against the 18.83× quoted above.One loose word, recorded rather than fixed: the
from_geometryacceptance row's "unchanged" is about behaviour, not text — its body was refactored in phase 2, when the routing tail moved into_cover_parts(8140908). Phase 2 discloses that and the parity matrix pins the behaviour, but the word reads as textual and is imprecise.Gates after the fold:
cargo fmt --checkclean;cargo clippy --lib --benchesno new warnings (the same six pre-existing ones incoverage/tests.rs,geo2mort.rs,prefix_trie.rs);cargo test --lib286 passed / 1 ignored (unchanged — the fold is Python-side);pytest1178 passed, 16 skipped (+3: twoPOLYGON Mparams, one basin-geography test);flake8 mortie --select=E9,F63,F7,F82clean,flake8 --max-line-length=88clean on every touched file,numpydoc lintclean. CI on1ba8d42is green across the board, including the test matrix on 3.10 / 3.11 / 3.12 — which is now actually exercising the memory assertions on Linux rather than passing them vacuously.Review fold (phase 5)
Phase 5's deliverable is evidence, so its review is about whether the evidence holds and whether it covers what it claims. One finding was a missing axis and landed as a commit; two were reporting errors in this body and are corrected above. All three, and every verification the review reproduced, are recorded here — on a validation phase the verifications are the value.
7ef59a6— the cross-part dialect axis is now measured, not inferred (r3740351968).shapely.to_wkbstamps one dialect on the container and every part, so the fixture grid never varied the part header — the exact axis the headline finding is about, contributed to the old "601,283 inputs" by a single hand-packed SRID blob.cross_part_blobs()adds 13 valid hand-packed MultiPolygons varying only the part header, andgeos_rings()scores mortie against GEOS on them from Python. The result reproduces the review's numbers exactly and is in the body above:wkbdiverges on 7 and rejects 1, geozero on 3, GEOS agrees with mortie 13/13. This moves the phase's strongest claim from "more correct on one mutated blob" to "more correct on a class of valid input".[5, 6]attribution (r3740351987).[5, 6]and "110 dropped vertices" belong to blob 33637 alone; three of the other four are the1,075,052,544-vertex shape, for which geozero returns[5, 5]. Divergence item (5) above now names each blob and both shapes. The headline case (14971) checks out byte-for-byte, so this was presentation only.agreeaccounting (r3740352000). The old table'sagreecolumn foldedagree_unsupported_typeback in — the inflation that bucket exists to prevent — so the fixtures row published101 = 86 + 15, and the malformed row summed to 45,149 rather than 45,300. The harness itself was always right (it prints the buckets separately); the table was not. It now carries every bucket as its own column, for all three oracles, and every row sums to its blob count.What the review verified independently, and reproduced — recorded so it is not re-derived later:
agree_okto exactly 0 (86/86 fixtures, 12,453/12,453 malformed caught); ring-swap, ring-drop and vertex-reversal are caught wherever structurally applicable. Buckets still sum to n and nothing leaked intoagree_erroragree_unsupported_type— so bit-exactness, ring order, ring count and vertex order are all genuinely compared, not vacuously.wkb::reader::read_wkb,geozero::wkb::Ewkb/Wkb→to_geo), so this is not mis-feeding.Vec<Vec<(f64,f64)>>materialization was suspected of flattering one side and was measured out — ratios 0.755 parse-only vs 0.748 materialized.cargo treeverification holds on all three checks, withCargo.lockconfirmed ignored at.gitignore:44and the[dev-dependencies]subtree confirmed present in the very tree that returns 0 hits.pub mod batch;from the copied parser excludes no parsing logic —src_rust/src/wkb/batch.rs:107calls the sameparse(bytes)the harness calls.Wkbrejecting the 18 SRID fixtures is correct geozero behaviour for a dialect the caller chose, not a handicapped oracle.Gates after the fold:
cargo fmt --checkclean;cargo clippy --lib --benchesno new warnings (the same pre-existing ones incoverage/tests.rs,geo2mort.rs,prefix_trie.rs);cargo test --lib286 passed / 1 ignored (unchanged — the fold is Python-side, inbenchmarks/);pytest1178 passed, 16 skipped (unchanged);flake8 mortie benchmarks/compare_wkb_crates.py --select=E9,F63,F7,F82clean,flake8 --max-line-length=88clean on the touched file,numpydoc lint mortie/*.pyclean.Left standing deliberately:
same()compares ring counts, ring order, vertex order and every coordinate bit, but notRings.kind(Polygonal vs Linear). That is a real gap in principle — and unreachable in practice, because a type code that would flipkindalso changes the coordinate layout, so any such blob diverges on values or lengths first. Recorded here so it is not rediscovered as a hole.Questions for review
_geometry_from_wkbnow has no non-test caller at all — the Rust parser replaced its only one — while_geometry_from_wktsurvives, because there is no Rust WKT parser. That asymmetry is worth a decision rather than a drift: does WKT eventually get the same treatment (a Rust WKT parser, making ingest backend-free in both spellings), or is WKT ingest permanently a backend-requiring convenience? Not mine to settle; flagging it while the shape is fresh. The scope line as it stands is pinned bytest_wkt_and_emit_still_require_a_backend.mortie/geometry.pyis well past this repo's ~1,000-line aim —wc -l mortie/geometry.pyis the figure that matters and 1,661 at5ca54d4is what it reads today (1,326 before phase 2; 1,482 at the phase-2 tip; 1,597 at5014c4b; 1,661 after70b021d). Baking the number into this question has now gone stale twice (r3739949511, r3740224359), so read it off the file rather than off this line; it is re-counted at each phase push and nowhere else — mortie's CLAUDE.md §4 has no 1,200 pre-approval; that ruling is zagg's, not this repo's, so the file is at ~164% of the only threshold that applies here and the (a)/(b)/(c) choice is being made against a bigger number than the question first stated. Phase 2 added_rings_from_wkb,_cover_partsand_wkb_bytes; phase 3 adds thefrom_wkbswrapper, and the ruled convention from mocs_to_orders: ragged batch moc_to_order — plus an API sweep for bulk-by-default operators #156 is a domain split with each plural twin beside its scalar, which putsfrom_wkbsnext tofrom_wkbingeometry.py. Flagging rather than splitting unilaterally. The overage is temporary by plan rather than unowned: Split tools.py and geometry.py into domain modules mirroring the Rust tree #159 (the domain split oftools.py/geometry.pymirroring the Rust tree,implement+blocked) is where the emit-half extraction now lives, and it is blocked on this PR and mocs_to_orders: ragged batch moc_to_order — plus an API sweep for bulk-by-default operators #156 — so the seam is scheduled, just not here. Options: (a) land phase 3 here and take the overage until Split tools.py and geometry.py into domain modules mirroring the Rust tree #159; (b) extract the emit half (to_geometry/to_wkb/to_wktand the dissolve helpers, roughlygeometry.py:644-1643) intomortie/emit.pyin this PR, leaving ingest ~600 lines — the natural domain seam, since emit is precisely the direction that keeps a backend; (c) something else. Recommendation: (a) here, with the split landing as Split tools.py and geometry.py into domain modules mirroring the Rust tree #159 so this PR stays reviewable.from_wkbs' scope for linear geometry.mortie.arrow.from_wkbsskin — RESOLVED: follow-up, filed as mortie.arrow.from_wkbs: arrow skin over the WKB batch, so a geoparquet column skips Python bytes objects #163 (espg's direction). Two corrections to how this was originally framed. (a) An arrow face would not be "zero-copy" and would not drop the one-chunk input-copy term: releasing the GIL still requires owned bytes on the Rust side, so the chunked copy stays regardless of how the blobs arrive. (b) The phase-3 fold largely removed the memory argument for it — arrow buffer slices now measure 1.07x, tied withbytes, so if a caller already holds correctly-cut slices, they cost no more thanbytes. But cutting them correctly out of a pyarrow column is the four-trap problem above, which is why the recipe was removed from the docs rather than corrected — so today's honest advice isto_pylist(), and mortie.arrow.from_wkbs: arrow skin over the WKB batch, so a geoparquet column skips Python bytes objects #163 is what makes the cheap path safely reachable. What the skin still buys is correctness (the four traps handled once, inside mortie, instead of in every consumer) plus ergonomics and avoiding N Pythonmemoryviewobjects — a stronger case than the memory one it replaces. mortie.arrow.from_wkbs: arrow skin over the WKB batch, so a geoparquet column skips Python bytes objects #163 is rescoped accordingly.src_rust/src/lib.rsis at 1,685 lines (1,633 at5014c4b, which is what the "~1,620" here rounded; 1,685 after the fold) and grew by ~120 across phase 3 (the batch pyfunction). It is a binding registry rather than logic — every phase that adds an entry point grows it, and Batch polygon coverage: polygons_to_morton_mocs (issue #153) #154 added its pyfunction there too — but it is worth saying out loud alongside question (2) rather than letting it drift. No proposal; flagging.