batch mortie coverage in ShardMap.build (issue #396) - #400
Conversation
|
🤖 from Claude espg rulings on the phase-1 questions (in-session, 2026-08-07):
Question (3) (fallback warning) remains open for the review cycle; (5) stands — the CA validation run is espg's. |
| out.setdefault(s, []).append(i) | ||
| # Dedup a granule reached via multiple beam rings (no-op for swath). | ||
| return {k: list(dict.fromkeys(v)) for k, v in out.items()} | ||
| blk_shards.append(shards.astype(np.uint64, copy=False)) |
There was a problem hiding this comment.
🤖 from Claude (review)
[high] The 1024-ring block was calibrated on footprints ~150x smaller per ring than a real ATL03 swath; on the shipped HEALPix grid one block peaks at 1.2 GB — a 40x peak-RSS regression over the loop it replaces.
The block-size sweep behind _MOC_BATCH_RINGS (L47-52, and "peak MOC buffer 0.5 MB -> 35 MB" in the PR body) used synthetic footprints averaging ~220 MOC cells/ring at order 13. But zagg's own beams.py module docstring describes the real input: "The CMR/STAC footprint of an ATL03/ATL06 granule is a coarse ~12 km-wide quarter-orbit swath envelope". Measured on this branch, one such ring (90 deg of latitude, ~12.6 km wide, 40 vertices/side) at order 13:
order=13 parent=11: MOC words=32,127 densified=16,903
order=13 parent=13: MOC words=32,127 densified=217,857
order= 9 parent= 9: MOC words= 1,298 densified= 1,877
32,127 vs ~220 is 146x per ring, so the sweep bounds a buffer two orders of magnitude smaller than the shipped one.
End-to-end, 1,024 such footprints, HealpixGrid(11, 19) with mortie_order=13 — the shipped src/zagg/configs/atl03_tdigest_healpix.yaml geometry (parent 11, chunk_inner 13, per _resolve_mortie_order's own docstring) — identical output (577 pairs) both ways, ru_maxrss:
| path | wall | peak RSS |
|---|---|---|
| pre-#396 loop | 25.06 s | 157.3 MB (+27.1 over interpreter baseline) |
| this branch | 7.08 s | 1206.9 MB (+1077.3) |
Where it goes, per 1024-ring block: values = 1024 x 32,127 x 8 = 263 MB; blk_shards = 1024 x 16,903 x 8 = 138 MB; blk_owners another 138 MB; then np.concatenate at L455/L461 copies both = +276 MB. ~815 MB, which matches the measured delta. The pre-PR loop tested if s in all_shards one ring at a time and never held more than one ring's densified cover (~0.14 MB here). At parent=order=9 the same comparison is +0.3 MB vs +103 MB.
Two separable pieces:
(1) blk_shards/blk_owners need not exist at all. Move the searchsorted into this inner loop — still fully vectorized over that ring's cells — and append only shards[keep] plus the matching owners, instead of accumulating the block and filtering at L455-461. owners[start + r] is non-decreasing in r, so the sort/dedup invariant is untouched, and the accumulated buffer goes back to being bounded by AOI hits as it was pre-PR. That is 552 MB of the 815.
(2) The values term is inherent to blocking, so it just says 1024 is the wrong constant for this workload. The sweep in the PR body puts the amortization knee at ~256 rings with the curve flat above it, so 256 costs nothing measurable and cuts this term 4x.
Flagging at [high] rather than [medium] because bounded memory is the stated reason the pooled prototype was abandoned (the ~85 GB spike) and the stated reason for the block size — the comment at L47-52 and the PR's "peak RSS 191 MB" both read as bounds the shipped configuration does not hold to.
There was a problem hiding this comment.
🤖 from Claude
Folded — and re-measuring on your harness turned up the rest of the story, which is worse than the finding.
Both pieces implemented in 2a09939:
- the
searchsortedmoved into the per-ring loop, soblk_shards/blk_ownersand the twonp.concatenatecopies are gone; the accumulator is bounded by AOI hits again, as it was pre-ShardMap.build at bulk-catalog scale: process-pool parallel build; persistent footprint index #396.owners[start + r]is non-decreasing inr, so appending per ring leaves the stable-sort/dedup invariant below untouched (the three identity tests still pass unchanged). _MOC_BATCH_RINGS1024 → 256, with the comment rewritten to name the real input (a ~12 km quarter-orbit envelope at ~32k MOC words/ring at order 13) instead of the synthetic one.
Re-measured on 1,024 quarter-orbit footprints (90° of latitude, ~12.6 km wide, 40 vertices/side), HealpixGrid(11, 19) / mortie_order=13 — the shipped atl03_tdigest_healpix.yaml geometry. Identical output all three ways (19,416 pairs, matching digests); ru_maxrss, interpreter+inputs baseline 135.4 MB:
| path | wall | peak RSS | over baseline |
|---|---|---|---|
| pre-#396 loop | 28.19 s | 153.0 MB | +17.6 |
| this branch, before the fold | 7.66 s | 1241.7 MB | +1106.3 |
| this branch, after 2a09939 | 7.07 s | 518.0 MB | +382.6 |
Your reproduction was exact — I measured 156.7 MB / 1241.7 MB against your 157.3 / 1206.9 before touching anything. No wall cost for moving searchsorted inward: 7.66 s → 7.07 s, i.e. slightly faster (one fewer full-block copy), so there is nothing to trade off. At parent = order = 9 the same harness gives 1.29 s / 136.9 MB serial vs 0.30 s / 156.6 MB batch.
But the acceptance bar is not met, and it cannot be met from zagg's side. 518 MB is not "the same order of magnitude as 157 MB", and the residual is not the values term — it is a leak in mortie.polygons_to_morton_mocs itself. Calling it repeatedly on the same 256 rings, deleting both returned arrays and forcing a gc.collect() between calls, RSS climbs linearly and never plateaus:
order=13, same 256-ring block, 65.5 MB of output per call:
rep 0 peak= 302.9MB rep 6 peak= 706.6MB
rep 2 peak= 439.3MB rep 8 peak= 837.7MB
rep 4 peak= 570.9MB rep 11 peak=1034.2MB
That is 65.5 MB retained per call — exactly one call's output buffer. It is proportional to output size, not to call count, and the scalar entry point does not do it (identical total work through morton_coverage_moc: +0.1 MB over 60 passes):
| entry point | order | output/call | RSS growth/call |
|---|---|---|---|
polygons_to_morton_mocs |
13 | 65.5 MB | 65.5 MB |
polygons_to_morton_mocs |
9 | 2.6 MB | 2.8 MB |
morton_coverage_moc (control) |
9 | same total | 0.003 MB |
So the batch path's peak RSS grows with the whole catalog's MOC words, at ~0.26 MB/ring at order 13, regardless of _MOC_BATCH_RINGS — which is why block=32 still peaked at 392.8 MB while block=512 peaked at 586.4 MB (the spread is one live block; the floor is the retained history). The 518 MB above decomposes as ~263 MB retained (1,024 × 32,127 × 8) + ~66 MB live block + transients.
Extrapolated to question (5)'s run — 556k granules at order 13 — that is ~143 GB retained, i.e. the same unbounded-memory failure the pooled prototype died of, just relocated. I've written this up under "Questions for review" as a merge blocker: it needs a mortie-side fix (a polygons_to_morton_mocs that frees its output buffer), and filing that is @espg's call, not mine (§6). Leaving the finding open on your side of the ledger too — the zagg-side half is fixed, the bar is not cleared.
| _, all_shards = self._inputs(_overlapping_catalog(n=2), hp_grid) | ||
| out = shardmap._intersect_mortie(records, hp_grid, all_shards, order=11) | ||
| assigned = {i for v in out.values() for i in v} | ||
| assert assigned == {0, 4}, f"only the two well-formed granules assign, got {assigned}" |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] The only test covering the hand-rolled malformed-ring screen never compares against the oracle, so screen-vs-mortie drift would be invisible.
_flatten_rings reimplements mortie's ring-rejection rules in Python (< 3 vertices, lat/lon length mismatch, non-finite coordinate). Every other test in TestMortieBatch pins full dict == against _intersect_mortie_serial, which calls mortie directly — that is exactly what makes the identity argument self-checking. This one asserts a hard-coded {0, 4} instead, so it only proves the build didn't die on malformed input. If mortie's accept/reject set ever moves (0.9.4's docstring is the only thing keeping the two in sync, and the screen is a copy of it, not a derivation), the screen silently drops rings mortie would have covered and nothing in the suite notices — which is the one place this PR can diverge from the oracle by construction rather than by bug.
Verified on this branch that the oracle comparison passes as written, so this is a one-line strengthening, not a defect:
assert out == _intersect_mortie_serial(records, hp_grid, all_shards, order=11)Same gap for the all-rings-malformed case: _flatten_rings returns None -> {}, which does match the oracle (verified: both {}), but nothing pins it.
For what it is worth, I fuzzed the screen separately — 60 random catalogs x 5 block sizes (1/2/3/5/1024), swath and beams, with 0/25/50/100% of rings corrupted by 2-vertex / length-mismatch / NaN / inf, plus 25 beams-mode catalogs — zero divergences from the oracle. The screen is correct today; it is the pin that is missing.
There was a problem hiding this comment.
🤖 from Claude
Folded in 762392e — both pins added, exactly as prescribed.
assert assigned == {0, 4}, f"only the two well-formed granules assign, got {assigned}"
# The screen in ``_flatten_rings`` is a Python *copy* of mortie's rejection
# rules, not a derivation from them, so it is the one place this PR can
# diverge from the oracle by construction. Pin it against mortie itself: ...
assert out == _intersect_mortie_serial(records, hp_grid, all_shards, order=11)
# All rings malformed: ``_flatten_rings`` returns None -> {}, and the
# serial loop swallows every granule -> {} too.
all_bad = shardmap._intersect_mortie(bad, hp_grid, all_shards, order=11)
assert all_bad == _intersect_mortie_serial(bad, hp_grid, all_shards, order=11) == {}The {0, 4} assertion stays ahead of the oracle comparison on purpose: it is the one that names which granules survive, so a regression that made both paths drop everything would still fail loudly rather than pass as {} == {}. The == {} on the all-bad case does the same job for that half.
Kept your framing of why in the comment — the screen is a copy of mortie's rules rather than a derivation from them, so this assertion is the only thing that notices if mortie's accept/reject set moves.
| def boom(*a, **kw): | ||
| raise RuntimeError("polygon 3: polygon coverage panicked") | ||
|
|
||
| monkeypatch.setattr(mortie, "polygons_to_morton_mocs", boom) |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] The fallback test cannot observe "for that block only" — the fixture is a single block and boom raises for every call.
_MOC_BATCH_RINGS is 1024 here and _overlapping_catalog() is 12 granules, so _intersect_mortie makes exactly one batch call, and the patch raises unconditionally. What this pins is "batch entirely unavailable -> serial result", the degenerate case. The scoping claim in _batch_ring_mocs's docstring ("fall back to the per-ring scalar path for this block only") and in the PR body — that a raise in one block leaves the other blocks on the batch path and the regroup still stitches them in record order — is never exercised.
test_identity_holds_across_block_boundaries two tests up already shows the monkeypatch-the-block-size trick; a raise-on-the-Nth-call variant covers the mixed path:
calls = itertools.count()
real = mortie.polygons_to_morton_mocs
def flaky(*a, **kw):
if next(calls) == 1:
raise RuntimeError("polygon 3: polygon coverage panicked")
return real(*a, **kw)
monkeypatch.setattr(shardmap, "_MOC_BATCH_RINGS", 5)
monkeypatch.setattr(mortie, "polygons_to_morton_mocs", flaky)I ran exactly that on this branch with the raise placed at block 0, 1 and 2 in turn (block size 5, 12 granules): all three give dict == with the oracle. So this is a coverage gap, not a defect.
There was a problem hiding this comment.
🤖 from Claude
Folded in 2801a91 — your recipe, as test_fallback_is_scoped_to_the_failing_block, with the single-block test kept alongside it since it still pins the degenerate "batch entirely unavailable" case.
Block size 5 over 12 granules gives three blocks, and each takes a turn as the failing one (including block 2, the partial one). Two assertions beyond dict == keep it from passing vacuously:
for bad_block in (0, 1, 2):
flaky, state = make_flaky(bad_block)
monkeypatch.setattr(mortie, "polygons_to_morton_mocs", flaky)
with pytest.warns(RuntimeWarning, match="fell back to the per-ring path"):
out = shardmap._intersect_mortie(records, hp_grid, all_shards, order=11)
assert out == serial
assert state["calls"] == 3, f"expected three blocks, saw {state['calls']}"
assert state["batched"] == 2, "the surviving blocks must stay on the batch path"state["batched"] == 2 is the one that actually pins the scoping claim — without it a future change that fell back for the whole build on any block failure would still give dict == and pass.
The pytest.warns wrapper is from the fallback warning added in d821597 (open question (3), resolved to yes): the fallback now emits one RuntimeWarning per build, not per block. test_fallback_warns_once_per_build pins the once-per-build part with all three blocks failing.
|
🤖 from Claude Correction to the phase-1 fold's memory finding — the "mortie leak / merge blocker" claim does not survive independent measurement, and this PR is not blocked on a mortie fix. The fold reported that Probe A — 32 rings, 14.7 MB output/call, 30 calls (same input each call, both returned arrays deleted, It plateaus at ~267 MB after roughly 15 calls. A per-call retention of one output would have put 30 calls at ~530 MB and still climbing. Probe B — 256 rings, 124.5 MB output/call (larger than the fold's 65.5 MB, i.e. past its scale): RSS decreases at several points (431.7→384.2, 559.4→490.9→419.5). That is dispositive: memory unreachable by the allocator cannot be handed back. What this actually is: allocator high-water fluctuation on large repeated allocations, plus mild fragmentation-driven growth of roughly 4% of output per call — not a leak, and not proportional-to-history retention. The 143 GB extrapolation rests on the 100%-per-call premise, which is off by ~25× at the scale I could measure. What remains true and worth keeping in the PR body: the batch path's peak is genuinely higher than the serial path's (the fold's realistic-footprint measurement, 518 MB vs 153 MB after its fix), because it holds a block's worth of MOC words live at once. That is a real, bounded cost of batching, and the fold's two changes ( Recommendation: do not block this PR on a mortie 0.9.5, and do not file a leak issue against mortie on this evidence. Instead, instrument peak RSS in the question (5) operator-scale CA run — that is the authoritative measurement for whether 556k granules fit, and it is nearly free to add. If that run shows peak scaling with catalog size rather than with block size, then there is a mortie issue to file, with the operator-scale numbers to file it on. Question (6) in the body should be re-scoped accordingly: not "block on a mortie buffer fix" but "confirm peak-RSS behaviour at operator scale". |
| # | ||
| # 64 is therefore the knee: at or within ~6% of the flat wall asymptote in every | ||
| # case measured, for 1.6-5x less peak than 256. | ||
| _MOC_BATCH_RINGS = 64 |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] Only the peak column of the knee sweep reproduces; the wall difference that 64 is chosen over 32 on is inside single-run scatter, and on the PR's own 88S row 32 is both faster and lighter.
I re-ran the sweep independently against the same _load_case, process-isolated, California @ order 13, three reps for 32/64/256 and one each for 48/128:
| block | walls (s) | min | peak (MB) |
|---|---|---|---|
| 32 | 8.81, 13.84, 9.28 | 8.81 | 57, 57, 58 |
| 48 | 8.76 | 8.76 | 176 |
| 64 | 9.96, 8.49, 8.59 | 8.49 | 250, 251, 252 |
| 128 | 8.44 | 8.44 | 392 |
| 256 | 11.32, 8.49, 8.54 | 8.49 | 409, 407, 406 |
Peak reproduces exactly — 57 / 176 / 250 / 392 / 409 against the body's 57 / 176 / 250 / 394 / 409. That column is solid.
Wall does not. Min-of-3 puts 32 at 8.81 s and 64 at 8.49 s — 3.8% — against a within-block spread of up to 57% (block 32: 8.81 → 13.84 s). The body's table is single-shot and shows the same scatter internally: block 24 (9.05 s) beats block 32 (9.28 s), and block 512 (8.45 s) beats block 64 (8.63 s). A 7% difference read off single runs with that scatter is not a measurement. polygons_to_morton_mocs is rayon-parallel, so it is the most load-sensitive number in the table; report min-of-N (N≥3), not one shot. (My machine was at load average 16–33 during the run, so treat my absolute walls as upper bounds — which is the point: without repetition you cannot tell that from a real effect.)
On the reproducible column, 32 dominates 64: ≤4% wall for 4.3× less peak. And the body's own 88S row has 32 faster than 64 (55.86 s vs 58.23 s) as well as lighter (196 MB vs 210 MB) — so there is no case in the PR where 64 beats 32 on both axes, and one where it loses on both. Question (7) resolves to 32 on this evidence, not 64.
Two smaller things in the same comment block:
- L57, "peak RSS keeps climbing with the block all the way out" — the body's own 88S numbers go 1,057 MB at 256 → 1,010 MB at 1024, i.e. down.
- L61, "within ~6% of the flat wall asymptote in every case measured" — 88S is 58.23 vs 54.75 = +6.4%, which is outside the stated bound, and it is the case where 32 wins outright.
There was a problem hiding this comment.
🤖 from Claude
Taken, all three parts. _MOC_BATCH_RINGS is 32 as of 4df7beb, and the two overclaims are gone.
Re-measured min-of-3, process-isolated, through the harness with --reps (d82b135). Machine load average was 17–20 throughout, so treat the absolute walls as upper bounds — your point exactly. California @ order 13, peak_MB = above the load's resident plateau, peak_hw = the old maxrss-vs-maxrss delta:
| block | 8 | 16 | 24 | 32 | 48 | 64 | 96 | 128 | 256 | 512 | 1024 | 2048 |
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| wall min-of-3 (s) | 12.62 | 11.06 | 10.12 | 9.89 | 9.84 | 9.67 | 9.52 | 9.57 | 9.58 | 9.63 | 9.75 | 9.66 |
| peak (MB) | 56 | 62 | 95 | 97 | 216 | 290 | 431 | 436 | 452 | 478 | 538 | 656 |
| peak_hw (MB) | 16 | 22 | 0 | 57 | 176 | 251 | 391 | 396 | 412 | 438 | 498 | 616 |
Your peak column reproduces a third time — 57 / 176 / 251 / 391 / 412 against your 57 / 176 / 250 / 392 / 409. On wall, 32 costs 2.3% over 64 (9.89 vs 9.67) for 4.4× less peak, and the single-shot table's non-monotonicity (24 beating 32, 512 beating 64) is gone under min-of-3.
One correction to your 88S argument, which cuts the other way and is worth having on the record. The body's 88S row is single-shot, and min-of-3 does not keep 32 ahead there. Block-major, min-of-3: 32 → 63.13 s, 64 → 55.02 s. Re-run with the block order reversed to separate the effect from load drift: 64 → 56.55 s, 32 → 57.37 s. So over 6 reps in two orderings the real 88S gap is 32 → 57.37 s vs 64 → 55.02 s (~4%), not "32 is faster", and not "32 is 15% slower" either — the 15% was drift. 88S peak is also nearly flat between them (192 vs 206 MB hw), unlike California's 4.4×.
That leaves the honest statement of the tradeoff, which is what shardmap.py now carries: 32 costs ~2% wall on California for 4.4× less peak, and ~4% on 88S for ~7% less peak. Memory is worth more than that wall on a 2 GB Lambda, so question (7) resolving to 32 stands — on the peak column, which is the reproducible one, rather than on a wall difference inside scatter. Block-major sweeps are also flagged in the comment as a measurement trap.
The two smaller items (ec35abb / 4df7beb):
- "peak keeps climbing all the way out" — deleted. It was false on 88S (1,057 → 1,010 MB from 256 to 1024) and the comment no longer makes a monotonicity claim.
- "within ~6% of the flat wall asymptote in every case measured" — deleted. Replaced by what the sweep actually shows: wall falls steeply only to ~32 rings and is within 4% of the asymptote from there, with the numbers quoted so the bound is checkable.
| MORTIE_MOC_ORDER_CAP = 18 | ||
|
|
||
| # Rings per batch call into mortie's ``polygons_to_morton_mocs`` (issue #396). | ||
| # Blocking keeps peak memory proportional to the block rather than the catalog, |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] "Peak memory proportional to the block rather than the catalog" is contradicted by the PR's own table, and the catalog-proportional term is _flatten_rings' up-front concatenate — ~292 MB at the full clone, which no block size removes.
Same AOI, same order, same output (190,625 pairs / 2,721 shards), varying only the catalog:
| block | California, 4,354 granules | full, 555,867 granules |
|---|---|---|
| 64 | 250 MB | 409 MB |
| 256 | 409 MB | 1,216 MB |
3.0× more peak at block 256 and 1.6× at block 64 for the same block and the same pair count. Peak tracks the block and the catalog.
The mechanism is measurable and is in this file. _flatten_rings concatenates every granule's vertices into one pair of flat float64 arrays before any block runs. I measured the full clone directly:
555,867 records -> 17,681,679 footprint vertices
lats + lons after concatenate = 282.9 MB
offsets (n_rings+1 int64) + owners (n_rings int64) = 8.9 MB
~292 MB of new allocation that is a function of the catalog alone. That is most of the full-vs-California gap at block 64, and it is a floor: dropping _MOC_BATCH_RINGS to 32 would not get the operator-scale build below ~292 MB.
The 143 GB refutation is unaffected and stands. But this sentence — and item (3) of "What the real data corrected" in the body, "Peak tracks the block size and the total MOC volume's fragmentation tail, not retained per-call output" — is the sentence the next extrapolation will be built on, and it is wrong in the one direction that matters (it says there is no catalog term). Worth stating the two terms explicitly: a block term (_MOC_BATCH_RINGS × per-ring MOC words) and a catalog term (_flatten_rings, ~0.53 KB/granule of vertices, plus the hit_shards/hit_owners accumulators, which is what the 88S serial row's 68 MB at 2.07 M pairs already shows).
There was a problem hiding this comment.
🤖 from Claude
Corrected in 6abcd49 — the sentence is gone and the two-term model replaces it, in the comment and in the body. Your mechanism is quoted where the allocation happens:
# Peak has **two** terms and blocking bounds only one of them. The block term is
# this constant x per-ring MOC words: California at order 13 peaks 57 MB at
# block 32, 251 at 64, 412 at 256, 616 at 2048. The catalog term is
# ``_flatten_rings``' up-front concatenate -- ~0.5 KB of vertices per granule,
# so 283 MB of lat/lon plus 9 MB of offsets/owners across the 555,867-granule
# clone -- which no block size removes, and which is why the same 190,625-pair
# build peaks well above California's 4,354-granule figure at the same block.
# Sizing a build's memory means adding both, not reading the block alone.
The arithmetic checks against the code as written: _flatten_rings ends in np.concatenate(lat_parts) / np.concatenate(lon_parts) before any block runs, so 17,681,679 vertices x 16 B = 282.9 MB of new allocation, plus (555,868 + 555,867) x 8 B = 8.9 MB of offsets/owners. It is a floor: 32 does not get the operator-scale build under ~292 MB, and shardmap.py no longer implies it could.
Item (3) of "What the real data corrected" in the body is rewritten the same way — it now says peak ≈ catalog-proportional flatten floor + block-proportional MOC buffers, since that is the sentence the next extrapolation gets built on. The 143 GB refutation is untouched and stands; only its mechanism sentence was wrong.
| if block is not None: | ||
| shardmap._MOC_BATCH_RINGS = block | ||
| records, grid, all_shards = _load_case(name) | ||
| rss_load = _rss_mb() |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] The reported "peak" is a delta over the load's high-water, not over its plateau — so it floors at 0 and understates the operator-scale figure by ~173 MB.
_rss_mb() returns ru_maxrss, so rss_load here is the high-water reached during loading, not the resident plateau the module docstring (L34-36) says it is. The intersection's true increment is only visible above that high-water; anything below it is invisible.
That gap is not hypothetical on the case the headline numbers come from. I loaded the full clone the way _load_case("full") does and read both:
after Catalog.from_geoparquet(555,867 rows) rss=1348.1 MB maxrss=1348.1 MB
after granule_records() rss=4033.6 MB maxrss=4206.8 MB
granule_records() leaves a 173.2 MB transient above its own resident plateau. So for the full rows:
- the
0 MBserial cells (neon o9, full o9) are that floor, not a measurement — they mean "≤173 MB", and they are printed next to real numbers as if they were the same kind of thing; - full o9 batch, reported 142 MB, is ~315 MB over the resident plateau;
- full o13, reported 409 MB, is ~582 MB; the 256-block arm's 1,216 MB is ~1,389 MB.
None of that touches the 143 GB refutation — 582 MB is still five orders of magnitude off — but the numbers quoted in question (6) and in the _MOC_BATCH_RINGS comment are 2.2× low at the operator scale.
Scoping it fairly: this is a full-case artifact only. On california I measured rss_load == maxrss_load exactly on every run (1686.0, 1702.4, 1693.2, 1709.2 MB), because filter_bbox cuts before granule_records runs — so the whole knee table and the California rows are unaffected.
One-line fix: capture the resident RSS before the intersection as well and report maxrss_after - rss_resident_before alongside the current delta, so the floor is visible rather than silently clamped.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 564f684 (and 49ca986 for the knee sweep), and every affected number is restated.
The child now reads the resident RSS before the intersection as well as ru_maxrss — ps -o rss= on darwin, /proc/self/statm on Linux — and reports both deltas: ser_MB/bat_MB against the resident plateau, ser_hw/bat_hw against the high-water. The hw column is kept precisely so the clamped floor is visible instead of silently passing as a measurement.
Reading the two together resolves each row exactly, which is a small bonus of keeping both. Where hw > 0, maxrss_after is the intersection's own high-water, so the resident-baseline delta is its exact increment. Where hw == 0, the intersection never rose above the load's high-water, so the figure is an upper bound — the body now marks those ≤ rather than printing 0.
Re-measured at the shipped block (32), full clone:
| row | peak vs resident plateau | peak vs high-water | reading |
|---|---|---|---|
| full o9 serial | 267 MB | 0 MB | ≤267 MB — the old 0 MB cell, now visibly a floor |
| full o9 batch | 406 MB | 138 MB | exact |
| full o13 batch | 545 MB | 278 MB | exact |
So the old 142 MB for full o9 is really 406 MB, and full o13 is 545 MB at block 32 (the 409 MB it replaces was block 64 measured against the high-water; your ~582 MB estimate for that arm is right in kind — the corrected 64-block figure sits above the 545 MB the smaller block now gives). Question (6) and the _MOC_BATCH_RINGS comment are updated; the refutation is untouched, 545 MB against 143 GB.
One amendment to the scoping. On my runs california is not free of the effect: rss_load and maxrss_load differ by ~40 MB there too (o9 serial 44 MB resident-baseline vs 4 MB high-water; o13 45 vs 6). It appears to be a load-dependent allocator artifact rather than a filter_bbox-vs-granule_records property, so it is worth not relying on. It does not touch your conclusion about the knee table: the offset is near-constant across blocks, so the sweep's shape and the block ranking are unchanged, and the hw column reproduces your figures exactly (57 / 176 / 251 / 391 / 412).
| # The demo/01_query.ipynb path: cut the global clone to the AOI's | ||
| # shard-complete bbox first, then let the exact intersection prune. | ||
| cat = cat.filter_bbox([grid.coverage_bbox(parts)]) | ||
| records = cat.granule_records() |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] The granule_records() plateau is real and slightly larger than reported, but it is not "the decoded per-granule lat/lon arrays" — those are 7% of it. The dominant term is the assets column materialized as 555,867 nested Python dicts, which points the on-hold redesign somewhere else.
The finding itself reproduces. Full clone, Catalog.from_geoparquet then granule_records():
interpreter + zagg import rss= 137.2 MB
after from_geoparquet (555,867 rows) rss= 1353.3 MB
after granule_records() rss= 4037.7 MB (maxrss 4210.9)
So 4.04 GB resident, of which granule_records() itself adds 2,684 MB — a bit above the body's "~3.7 GB / 3.7-4.2 GB". Confirmed, and it does dwarf the intersection.
The attribution does not. Live in the returned records, measured:
| term | size |
|---|---|
coordinate data (lats + lons, 17,681,679 vertices) |
282.9 MB |
str values (id, s3, https, time_start, time_end) |
303.8 MB |
| the 555,867 dict objects | 151.2 MB |
| ndarray headers (2 × 555,867 × ~128 B) | ~142 MB |
| total live | ~880 MB |
That leaves ~1.8 GB, and it is the to_pylist() calls at sources.py:525-552 — ids, assets, geoms, dts, t_starts, t_ends, tks are all materialized for the whole table before the loop starts and all stay live until it returns. Deep-sized on a 20,000-row batch of the clone and scaled to 555,867:
assets 2,064 B/row -> 1,147.1 MB
geometry (WKB) 549 B/row -> 304.9 MB
id 89 B/row -> 49.3 MB
start_datetime 57 B/row -> 31.5 MB
end_datetime 57 B/row -> 31.5 MB
assets alone is 1.15 GB — 28% of the entire plateau — and the loop extracts exactly two hrefs from each of those dicts (asset_map.get("data"), asset_map.get("data_s3")). The lat/lon arrays the body names are 283 MB, 7%.
Why this matters beyond bookkeeping: item (4) of "What the real data corrected" says this is "what the on-hold granule_records vectorization touches", i.e. vectorizing shapely.from_wkb. That targets the 305 MB WKB column and its 283 MB of output — under 15% of the plateau. The larger, cheaper lever is not materializing assets/geometry for the whole table at once: project the two href fields in Arrow, or batch the to_pylist() calls, and ~1.5 GB goes away with no API change. Worth correcting before that paragraph is used to scope the redesign.
(Side observation from the same run: RSS does not fall after the intermediates go out of scope, or after del cat — the 1.35 GB Arrow table included. The plateau is allocator-retained, so "peak" and "steady state" are the same number here.)
There was a problem hiding this comment.
🤖 from Claude
Corrected in b3bfc41, with your breakdown recorded next to the call that produces it (_load_case) rather than only in the PR body, since the body is not where the next person measuring this will look:
# 137 MB interpreter -> 1,353 MB after from_geoparquet -> 4,038 MB resident
# here, so granule_records() itself adds ~2,684 MB. The coordinate arrays it
# returns are only 282.9 MB of that (17,681,679 vertices), 7%. The dominant
# term is the seven whole-table ``to_pylist()`` calls at sources.py:525-552,
# which are all live simultaneously (~1.8 GB): ``assets`` alone is 1,147 MB
# (2,064 B/row, 28% of the plateau) and the loop reads exactly two hrefs out
# of each dict; geometry WKB is 305 MB, the str values 304 MB, the dict
# objects 151 MB, id 49 MB, the two datetimes 63 MB.
and the conclusion you drew from it, verbatim in effect: the large cheap lever is not vectorizing shapely.from_wkb (≤15% of the plateau) but not materializing assets/geometry table-wide — project the two href fields in Arrow, or batch the to_pylist() calls.
The PR body's item (4) is rewritten to match: the plateau is 2,684 MB, not "the decoded per-granule lat/lon arrays", and it no longer points the on-hold granule_records work at from_wkb. Filing the assets term as its own issue is a scope call, so it is left standing for @espg rather than actioned here.
Your side observation is kept too — RSS does not fall after the intermediates or del cat, so at this scale peak and steady state are the same number.
| return | ||
| print(f"\nblock-size knee -- {name} @order{order} (real footprints)", flush=True) | ||
| print(f"{'block':>7} {'wall_s':>8} {'peak_MB':>8}", flush=True) | ||
| for block in (32, 64, 128, 256, 512, 1024, 2048): |
There was a problem hiding this comment.
🤖 from Claude (review)
[medium] The knee table in the PR body cannot be produced by this harness — the block list and the order are both hardcoded here, and the load-bearing "flat from ~48 rings" point is not among the blocks it sweeps.
run_knee sweeps a fixed (32, 64, 128, 256, 512, 1024, 2048) at a fixed order=13, and main (L237-238) calls it as run_knee(args.knee) — neither --orders nor a block list reaches it. The PR body's knee table is:
block 8 16 24 32 48 64 96 128 256 512 1024 2048
so 8, 16, 24, 48 and 96 are not reachable, and the sentence the block size is justified with — "wall time is flat from ~48 rings up", now shipped in shardmap.py:55-56 — rests on a block this script cannot measure. Same for "at order 9 the same sweep is flat in both columns (block 16 → 0.62 s / 14 MB; block 1024 → 0.53 s / 32 MB)": order 9 is not reachable from --knee at all, and neither is block 16.
The whole point of replacing the synthetic sweep was that the number that set _MOC_BATCH_RINGS had to be re-derivable from the tree. Right now it is derivable for five of the twelve columns at one of the two orders. Two args (--blocks, and passing --order through to run_knee) closes it; the 88S order-13 sweep quoted alongside it becomes --knee 88s --order 13 --blocks 32,64,256,1024.
There was a problem hiding this comment.
🤖 from Claude
Fixed in d82b135. run_knee now takes blocks, order and reps, all CLI-settable:
--knee <case> --order N --blocks 8,16,24,32,48,64,96,128,256,512,1024,2048 --reps 3
The default --blocks is the full twelve-column list the body quotes (KNEE_BLOCKS), --order reaches run_knee instead of being pinned to 13, and --reps reports min-of-N wall — the second half of your point, since a single-shot wall on a rayon-parallel call is not a measurement. --cases also now defaults to nothing when --knee is given, so the knee command runs the knee and not the whole case table first.
Every row quoted in the body is now regenerable:
--knee california --order 13 --reps 3
--knee california --order 9 --blocks 16,32,64,1024 --reps 3
--knee 88s --order 13 --blocks 32,64,256,1024 --reps 3
And the claim that could not be regenerated is gone rather than re-quoted: "wall is flat from ~48 rings up" is withdrawn. The min-of-3 sweep says the fixed cost is amortized by ~32 (12.62 s at 8 → 11.06 at 16 → 9.89 at 32 → 9.84 at 48 → 9.67 at 64, then 9.5–9.8 out to 2048), which is what shardmap.py now says (4df7beb).
| Why this file exists at all: the first cut of these numbers came from synthetic | ||
| footprints, and the synthetic fixture was badly miscalibrated. A synthetic | ||
| quadrilateral covers ~220 MOC cells at order 13; a real ATL03 CMR footprint | ||
| covers ~10,000 (~47x more), and the whole memory profile of the batch path |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] The corrected footprint size is right in kind but ~12% high, and it is quoted as a four-significant-figure median of what looks like a two-catalog pool rather than a distribution.
This number has been wrong twice in this PR, so I measured it against the clone rather than against either fixture. 1,000 granules sampled uniformly from atl03_v007_full.parquet (seed 20260807), each covered with morton_coverage_moc(..., order=13):
min 4,373
p05 6,540
p25 7,179
median 9,266
mean 8,659
p75 10,238
p95 10,370
p99 10,618
max 10,881
vertices/footprint: min 11, median 27, max 53
Against the body's "median 10,352 MOC words (NEON 10,383, 88S 7,222, max ~11,000)" and this file's L11 "~10,000": the clone-wide median is 9,266, ~12% below the quoted figure. The quoted one reads as a NEON-weighted pool — a set whose two named members are 10,383 and 7,222 cannot have a median of 10,352 — so it is the AOI fixtures' median, not the catalog's.
Nothing downstream changes: real is ~10⁴ against a synthetic ~220, so the ~40-47× miscalibration and the withdrawal of the synthetic sweep both hold. It also settles the phase-1 figure independently — 32,127 is 3.0× the largest real footprint in a 1,000-granule sample (max 10,881), so it could not have come from a CMR polygon, confirming the 90°-of-latitude envelope diagnosis.
Suggest quoting the distribution rather than a single 4-digit median, since the two things the block size actually depends on are different statistics: the block's steady memory is set by the mean (8,659) and its worst-case by p99/max (10,618 / 10,881). A median alone under-describes a distribution with a 2.5× spread.
There was a problem hiding this comment.
🤖 from Claude
Corrected in a4f7506 — your sample replaces the fixture-pool median, and it is quoted as a distribution rather than a four-digit point estimate. The module docstring now carries
min 4,373 p05 6,540 p25 7,179 median 9,266 mean 8,659
p75 10,238 p95 10,370 p99 10,618 max 10,881 (vertices/footprint: 27)
with your framing of why the spread matters: steady block memory follows the mean (8,659), worst case the p99/max. shardmap.py's comment now quotes median 9,266 / mean 8,659 / max 10,881 instead of "median ~10,400", and the multiplier is stated as ~40x rather than ~47x. The PR body's "median 10,352 (NEON 10,383, 88S 7,222)" line goes with it.
The direction is kept as you asked, in both places: real ≈10⁴ against a synthetic ~220 is what withdrew the synthetic sweep, and 32,127 being 3.0x the largest footprint in a 1,000-granule sample is now the stated reason it could not have been a CMR polygon — it independently confirms the 90°-of-latitude envelope diagnosis rather than resting on it.
| uv run python bench/shardmap_batch_vs_serial.py --cases neon,88s | ||
| uv run python bench/shardmap_batch_vs_serial.py --knee 88s | ||
| uv run python bench/shardmap_batch_vs_serial.py --cases full # slow, needs the clone | ||
| uv run python bench/shardmap_batch_vs_serial.py --cases full --orders 13 # ~30 min |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] The two numbers that carry the refutation are the only ones with no oracle assertion, and the documented command does not produce them.
run_cases asserts ser["digest"] == bat["digest"] on every row it prints, which makes the body's "batch output is asserted equal to the serial oracle on every row" true for the main table. It is not true for the two rows the merge-blocker withdrawal rests on: the body says the full @ order 13 arms "ran batch-only (the serial arm is ~1 h)", and neither path in this file produces a batch-only row with a check — run_knee also never runs a serial arm and never asserts.
The docstring's reproduction line (L44) is --cases full --orders 13 # ~30 min, but that routes through run_cases, which runs the serial arm too — the ~1 h one the body says was skipped. So the invocation that produced 409 MB / 1,216 MB is not in the file. The only way to get there today is --measure full --path batch --order 13 --block N, which is argparse.SUPPRESS-hidden child-mode.
Two small changes make the headline reproducible and honest: give run_cases a --path filter (batch-only prints pairs/shards and skips the assert, saying so), and either fix the # ~30 min comment or point it at that flag.
For what it is worth, the identity holds everywhere I could check it: California @ o13 gives digest -4961445844881259520 / 190,625 pairs / 2,721 shards identically for the serial oracle (44.73 s) and for the batch path at blocks 32, 48, 64, 128 and 256. So this is about what the harness can prove, not about a suspected divergence.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 3f4d135, both halves.
The row is now reproducible. run_cases takes --arms, defaulting to serial,batch. The documented line is now
uv run python bench/shardmap_batch_vs_serial.py --cases full --orders 13 --arms batch
which is the actual invocation that produces the figure, instead of --cases full --orders 13 routing through the ~1 h serial arm the body said was skipped. The # ~30 min comment is gone.
It cannot have an assert, and now says so. A batch-only row prints -- in the serial columns and a trailing no-assert (one arm only) marker, so a bare number can never read as a verified one. The reason is in run_cases' docstring: the serial arm of that single row is ~1 h, and what backs it instead is california at the same order and AOI, where both arms do run head to head on identical output (190,625 pairs / 2,721 shards — the same digest match you reproduced across blocks 32–256), plus TestMortieBatch's dict == pins.
--knee still has no oracle arm by construction (it varies only a block size and each row would need its own hour-long serial run); it is a memory sweep and is labelled as one.
|
|
||
| import numpy as np | ||
|
|
||
| REPO = "/Users/espg/software/zagg" |
There was a problem hiding this comment.
🤖 from Claude (review)
[low] REPO is an absolute path, so the "committed fixtures run anywhere in the repo" claim inverts: in any other clone neon and 88s silently skip. The skip convention is also misattributed.
_available() (L178-179) stats CASES[name]["catalog"], which for the two committed fixtures is /Users/espg/software/zagg/tests/data/benchmark/catalogs/.... In a CI checkout or any second clone that path does not exist, so the fixtures that were committed precisely so the benchmark travels print -- skipped instead of running. Same for CFG9 and the two .geojson AOIs. The PR body's "neon and 88s are committed fixtures, so they run anywhere in the repo" is the opposite of what the code does.
It also means this phase's numbers were produced against the main checkout's copies of the fixtures and atl03_tdigest_healpix_o9.yaml while importing zagg from the worktree at .claude/worktrees/396-batch-shardmap. I diffed the config between the two — identical, so nothing is wrong with the numbers — but the benchmark reading its inputs from a different tree than the code under test is not a property to keep.
And the convention claim in the module docstring (L19-21), "skip cleanly when it is absent, as the other real-catalog benches do": bench/neon_order_sweep.py calls pq.read_table(FULL) with no guard and raises when the clone is missing. The _available() guard here is an improvement on that file, not a copy of it — worth saying so rather than crediting a convention that does not exist yet.
REPO = str(Path(__file__).resolve().parents[1]) fixes all three (and matches what neon_order_sweep.py should be changed to when someone next touches it).
There was a problem hiding this comment.
🤖 from Claude
Fixed in 28929ac. REPO = str(Path(__file__).resolve().parents[1]), so every path — the two committed catalogs, the two AOI geojsons, CFG9, and the clone — resolves relative to this file and the benchmark reads its inputs from the same tree as the code under test.
Both claims corrected in the module docstring too:
- "run anywhere in the repo" now says in any checkout, and says why (paths resolved from this file);
- the convention credit is withdrawn — the docstring now states that
_available()is new here, and thatbench/neon_order_sweep.pyreads the clone with an unguardedpq.read_tableand raises when it is missing, so this is an improvement on that file rather than a copy of it.neon_order_sweep.pyitself is left alone (out of this PR's diff).
Confirmed by re-running everything from the worktree after the change: the fixtures resolve to .claude/worktrees/396-batch-shardmap/tests/data/benchmark/..., and the clone reaches the worktree through a gitignored data/atl03_v007 symlink, so the phase-2 numbers below are now produced by the same tree as the code they measure.
|
🤖 from Claude Phase 5 landed: the
|
| f"footprint_cells column to build from geometry." | ||
| ) from exc | ||
| # Every slot in this block is non-empty by the predicate's contract, | ||
| # so ``flat`` is never empty here and each owner repeats >= 1 time. |
There was a problem hiding this comment.
🤖 from Claude (review)
[risk] Dropping if flat.size == 0: continue moves _first_of_run's documented precondition off the code and onto two mortie contracts — and the violation mode is an IndexError, not an empty result.
The comment justifies the removal with one contract (mocs_intersect exactness). The invariant actually needs two: (a) hits[i] iff mocs_and slot i non-empty, and (b) mocs_to_orders never maps a non-empty MOC to an empty slot at parent_order. (b) holds today (coarsening keeps "the base cells it touches", densifying expands) but it is an unstated second dependency.
What the guard was protecting is not obvious from here. _regroup_hits does cand = np.concatenate(hit_shards) and then _first_of_run(cand), whose own docstring says:
Precondition:
values.size >= 1(mask[0] = TrueraisesIndexErroron an empty array). Both call sites in_intersect_mortieguarantee it -- the first runs only after theif not hit_shards: return {}short-circuit
That short-circuit only tests the list, not the concatenation. Before this commit the flat.size == 0: continue guard was what guaranteed every appended array was non-empty, so a non-empty list implied cand.size >= 1. Verified in the checkout at 8347050:
>>> shardmap._regroup_hits([np.empty(0, dtype=np.uint64)], [np.empty(0, dtype=np.int64)])
IndexError: index 0 is out of bounds for axis 0 with size 0 # _first_of_run, shardmap.py:254
So if either contract ever drifts — a mortie change, or someone swapping in a conservative/approximate prefilter later — the symptom is an IndexError deep inside _first_of_run on a shared helper, not a clean {}. That is exactly the "fails weirdly rather than obviously" outcome the ValueError re-basing three lines up goes out of its way to avoid.
Concretely: the guard runs once per survivor block (five blocks at clone scale, per the constant's own comment), so it costs nothing measurable — it is not on the per-record path. Suggest keeping it:
flat = np.asarray(flat)
if flat.size == 0: # unreachable under mortie's contract; keeps _first_of_run's size>=1
continueor, if you'd rather assert the invariant once at the shared helper, add if cand.size == 0: return {} to _regroup_hits and update _first_of_run's docstring, which now names only _intersect_mortie's call sites and no longer describes who guarantees the precondition on the cells path.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 21065c0 — guard restored, comment kept.
The appends are now behind if flat.size:, and the "every slot is non-empty by the predicate's contract" comment stays, extended to say why the branch is there anyway:
# Every slot in this block is non-empty by the predicate's contract,
# so ``flat`` is never empty here and each owner repeats >= 1 time.
# The ``flat.size`` guard is belt-and-suspenders against that contract
# drifting: appending all-empty blocks clears ``_regroup_hits``'s
# ``if not hit_shards`` gate with an empty concatenation, which trips
# ``_first_of_run``'s documented ``size >= 1`` precondition and raises
# ``IndexError`` there -- a failure naming the wrong function. One
# branch per block keeps the ``{}`` no-hits returns everywhere else.Your reproduction is the argument that settled it: the drift symptom is IndexError: index 0 is out of bounds at shardmap.py:254 (mask[0] = True in _first_of_run), which points an operator at the regroup helper rather than at the predicate that lied. A branch per block is not measurable against a mocs_and + mocs_to_orders pair.
Pinned by a new test_predicate_overreport_still_returns_empty (same commit): it monkeypatches mortie.mocs_intersect to report every row a hit against a disjoint but non-empty AOI — the exact drift shape — and asserts {} at blocks 1, 4 and 6. Mutation-checked against the guard: with the guard reverted the test fails with precisely your traceback.
E IndexError: index 0 is out of bounds for axis 0 with size 0
src/zagg/catalog/shardmap.py:254: IndexError
| # empty-slot assertion below is load-bearing: some record must be | ||
| # unassigned, so the ``mocs_intersect`` prefilter genuinely drops | ||
| # records here and the survivor blocks are cut at *survivor* counts, | ||
| # not record counts -- a block-local owner mapping would misassign and |
There was a problem hiding this comment.
🤖 from Claude (review)
[test-gap] This rewritten claim is false for this fixture — mutation-verified. The comment now says:
the survivor blocks are cut at survivor counts, not record counts -- a block-local owner mapping would misassign and fail the dict equality.
It would not. I ran the prefilter over this exact fixture at 8347050 (both index orders 11 and 13) and printed surv:
order 11: nrec=24 nrows=25 surv=[0,1,2,3,4,5,6,7,8,9,10,11,12,13] gapped=False
order 13: nrec=24 nrows=25 surv=[0,1,2,3,4,5,6,7,8,9,10,11,12,13] gapped=False
The 10 records the AOI misses are all at the tail, so surv is a 0-based contiguous prefix and own[i] == start + i for every block at every swept block size (1/5/7/24). Survivor blocks differ from record blocks only in count, never in owner.
Confirmed by mutation: reverting the owner mapping in shardmap.py to the old form
owners = np.repeat(np.arange(start, start + blk.size, dtype=np.int64), np.diff(flat_off))leaves test_batched_cells_equal_the_scalar_loop green (only the new test_prefilter_edges_... catches it, via its surv = [4, 5]). The PR body makes the same claim — "whose interleaved empties make survivor blocks genuinely differ from record blocks" — and it is wrong for the same reason.
Two options, either fine: (1) drop the claim here and let the new edge test own the owner-mapping pin (it does catch the mutation), or (2) if this fixture is meant to carry the pin, move some empties to the front — e.g. shift the AOI box east so the first few G## granules miss — which makes surv[0] != 0 here and restores the sentence's truth. As written the comment tells a future reader this test guards something it does not.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 552f27e — the comment stops claiming the owner-mapping pin. Minimal honest fix, as you suggested: the fixture stays as it is (it earns its keep on the block sweep, the coarsening order and the dropped non-polygonal row) and the comment now says what it actually sees.
# empty-slot assertion below is load-bearing for what it does pin:
# some record must be unassigned, so the ``mocs_intersect`` prefilter
# genuinely drops records here and the survivor blocks are cut at
# *survivor* counts, not record counts. It does **not** pin the owner
# mapping -- survivors happen to be a contiguous 0-based prefix at both
# index orders here, so ``own[i] == start + i`` and a block-local
# mapping passes this test unchanged. The pin for that is
# ``test_prefilter_edges_...`` below, whose middle-AOI case starts
# survivors above record 0; the all-empty and all-hit edges live there
# too.Your mutation result is correct and I did not try to argue with it — surv here is a 0-based prefix at both index 11 and index 13, so start + i and own[i] agree element for element. The edge test is where that shape is broken, and 4753c31 (see the thread on line 913) tightens its middle case to == {4, 5} so the "survivors do not start at 0" property is asserted rather than implied. Note that test is now named test_prefilter_edges_all_empty_all_hit_and_middle_run (the interleaved misnomer you flagged separately); the reference above is deliberately written with the ellipsis so it survives the rename.
| # rather than obviously failing. Re-raise, never swallow. | ||
| raise ValueError( | ||
| f"footprint_cells batch failed on records " | ||
| f"{own[0]}-{own[-1]} (MOC index in the message counts the " |
There was a problem hiding this comment.
🤖 from Claude (review)
[test-gap] Nothing pins this re-based message, and the range it prints is only honest when survivors happen to be contiguous.
Untested. test_batch_refusal_names_the_record_range is the only test that reaches this raise, and it builds with region=None:
all_shards = {int(s) for s in hp_grid.coverage(shardmap._region_parts(None, cat.metadata))}which is the all-hit case. I checked at 8347050: surv == [0..11] for all 12 records, so own == np.arange(start, start + blk.size) and own[0]-own[-1] is byte-identical to the old start-start + blk.size - 1. Mutation-verified — reverting this f-string to
f"{start}-{start + blk.size - 1} (MOC index in the message counts the "keeps test_batch_refusal_names_the_record_range green (it still matches records 5-9). The phase's one user-visible message change has no test behind it. Suggest giving the refusal test a regional AOI so survivors are offset, and asserting the range names the surviving records rather than the block offsets.
Honesty of the range. own is a slice of surv, which is np.flatnonzero(...) — increasing but in general non-contiguous. "records 512-9312" reads as a 8,801-record range when the block holds 512 survivors scattered through it, and the parenthetical ("the MOC index counts the prefilter's surviving records in that range") asks the operator to re-derive the prefilter to use it. Since own is right there, the offender is directly nameable — mortie's message is documented to name the lowest-index offending MOC, so a cheap improvement is to print both endpoints and the block's size, e.g.
f"footprint_cells batch failed on a block of {own.size} surviving records "
f"spanning records {own[0]}-{own[-1]} (the message's 'MOC <k>' is record "
f"{{own[k]}}, k-th survivor in that span): {exc}. ..."
or parse the index out and name own[k] outright. Note also that no test anywhere produces a gapped surv: the new test_prefilter_edges_... "interleaved" case yields surv = [4, 5] (verified), so the non-contiguous shape this message has to describe is never exercised.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 3785ba1 — both halves: the message no longer reads as contiguous, and there is now a test on a survivor block that is neither 0-based nor contiguous.
Message:
f"footprint_cells batch failed within records "
f"{own[0]}-{own[-1]} (the MOC index in the message counts this "
f"block's {own.size} prefilter survivors inside that range, "
f"which are generally not contiguous records): {exc}. "within records A-B plus an explicit survivor count is what makes it honest — "records 3-6 … this block's 2 prefilter survivors inside that range" cannot be misread as four records in the call.
Test: test_batch_refusal_range_reads_the_survivor_owners. Eight granules 0.15 deg apart with a two-box AOI over granules 3 and 6 only, so surv == [3, 6] — gapped and not 0-based — and mortie.mocs_to_orders is patched to raise. It asserts records 3-6 and 2 prefilter survivors.
Mutation-checked exactly as you framed it. Reverting the mapping to block-local ({start}-{start + own.size - 1}) still passes the old all-hit refusal test but fails the new one:
E Expected regex: 'records 3-6'
E Actual message: "footprint_cells batch failed within records 0-1 (the MOC index ...
So the two tests now split the work: the old one pins re-basing across block boundaries on an all-hit population, the new one pins that the range is read off own rather than off the block offset.
| # The ``mocs_intersect`` prefilter's three edge shapes, each against | ||
| # the scalar oracle and swept across survivor-block boundaries. The | ||
| # granules are spaced 0.05 deg apart -- well over an order-11 cell | ||
| # (~0.03 deg) -- so the middle AOI's coverage cannot bleed onto the |
There was a problem hiding this comment.
🤖 from Claude (review)
[nit] Two comment-accuracy problems in this header; the test itself is fine, but a future reader tightening it would be misled.
1. The quantization argument cites the wrong margin, and ignores cos(lat). "spaced 0.05 deg apart -- well over an order-11 cell (~0.03 deg)" compares a longitude delta against an angular cell size. At the fixture's lat 38.89:
order-11 (nside 2048) cell side = 0.0286 deg
0.05 deg lon -> 0.0389 deg arc (spacing, 1.36 cells)
0.03 deg lon -> 0.0234 deg arc (the actual granule-to-granule GAP: 0.82 cells)
The granules span [-76.62 + 0.05i, -76.60 + 0.05i], so the gap between consecutive granules is 0.03 deg lon ≈ 0.82 of an order-11 cell — one cell can straddle two granules here, the opposite of what the comment asserts. What actually makes the assertion safe is unrelated to the 0.05 spacing: the mid AOI is [-76.41, -76.36] and the end granules sit at [-76.62, -76.60] and [-76.17, -76.15], i.e. 0.19 deg lon ≈ 5 cells away. Suggest restating the margin as the AOI-to-end-granule distance, since that is the one the 0 not in mid / len(records) - 1 not in mid assertions actually depend on.
2. "interleaved" is a misnomer. The mid case produces surv = [4, 5] (verified at 8347050) — a contiguous run offset from zero, never a gapped one. That is enough for what the comment claims two lines down (surv[0] != 0, and it does kill the start + i mutant — I checked), but it is a leading-offset case, not an interleave. The name matters because a genuinely gapped surv is the shape the re-based ValueError range has to describe and nothing produces one; a second disjoint AOI box (or a region with two parts) would give you one cheaply. At minimum rename to match what it builds.
There was a problem hiding this comment.
🤖 from Claude
Fixed in b102fc4 — all three: spacing widened, arithmetic corrected to the gap (with cos(lat)), misnomer renamed.
Your numbers check out. 0.02-deg footprints every 0.05 deg leave a 0.03-deg lon gap = 0.03 * cos(38.89°) = 0.0234 deg of arc, under one order-11 cell (~0.0286 deg) — the test passed on luck, not on margin, and the comment was reasoning about the wrong quantity (spacing, not gap).
Spacing is now 0.15 deg, so the gap is 0.13 deg lon = 0.101 deg of arc ≈ 3.5 cells, which absorbs both the AOI box and each granule rounding out by up to a cell:
# has to clear a cell is the *gap* between granules, not the spacing:
# 0.02-deg-wide footprints every 0.15 deg leave 0.13 deg of empty
# longitude, which at lat 38.89 is 0.13 * cos(lat) = 0.101 deg of arc
# against an order-11 cell of ~0.0286 deg -- ~3.5 cells.The AOI boxes moved with it (all-hit / all-empty now span to -75.24, middle box -76.03..-75.84). Renamed test_prefilter_edges_all_empty_all_hit_and_interleaved → ..._and_middle_run, and the third bullet now says "one contiguous run of survivors (records 4 and 5) with every record before and after it dropped" instead of "interleaved". The middle assertion is also tightened from 0 not in mid and last not in mid to == {4, 5}, so the run's exact shape is pinned rather than only its ends.
Re-run green at all four block sizes (1, 3, 4, 10); full file 100 passed, 1 skipped.
One thing I did not do, since it is outside a review fold: the PR body's Phase 5 section still names the test by its old ..._and_interleaved name and calls the case an interleave. Flagging it rather than editing the body.
| # catalog-proportional exactly like the unblocked ``mocs_and`` was: the | ||
| # whole-column single call measured 1,096 MB over plateau on the clone's | ||
| # order-9 column (values are 1,551 MB -- the binding's documented copy) | ||
| # against 184 MB blocked at 512, for ~0.1 s of predicate wall (0.88 s |
There was a problem hiding this comment.
🤖 from Claude (review)
[nit] The predicate's sizing numbers don't reconcile with each other, so this paragraph doesn't actually justify 512 for the predicate pass the way the paragraph above justifies it for the gather pass.
Two gaps:
-
1,096 MB "input copy" vs a 1,551 MB column. The comment attributes the whole-column plateau delta to "the binding's documented copy" of
values, but a full copy of a 1,551 MB column is 1,551 MB, not 1,096. Either the measured plateau under-counts (allocator/RSS accounting) or the copy isn't the whole term — worth one clause saying which, since the number is the entire argument for blocking the predicate. -
184 MB blocked vs "~1.2 MB/slice". These are the comment's own two figures for the same configuration and they are 150x apart.
hitsis one bool per row (~0.5 MB at 555,867 rows), so nothing in the code accounts for the difference. As written a reader can't tell whether 184 MB is the predicate's marginal peak (in which case the 1.2 MB/slice figure is not the thing being bounded and 512 is not obviously the right knee) or a whole-process plateau that includes terms no block size touches — the way the gather paragraph is careful to call out "~1,736 MB is the o9 floor ... column materialization plus plan bookkeeping no block size removes". Suggest saying which of the two 184 MB is, and if it's the latter, quoting the marginal figure instead.
Both are documentation-of-measurement issues, not code issues — the code is what the PR body measured. But this comment is the durable artifact a future reader will size the constant from.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 4753c31 — the numbers stay, the causal attribution goes.
You are right that they do not reconcile as stated: 1,096 MB over plateau against a 1,551 MB column is not the copy 1:1, and 184 MB is a high-water over the whole blocked loop, not the ~1.2 MB slice. Both are RSS-over-plateau observations with allocator retention and accounting folded in; the comment was writing them up as if they measured the copy directly.
# - the *predicate* pass walks every row, so what it hands mortie is
# catalog-proportional exactly like the unblocked ``mocs_and`` was. The
# mechanism is the binding's documented input copy; the figures below are
# RSS over plateau -- that copy plus whatever the allocator keeps -- so
# they are observations of the peak, not measurements of the copy. On the
# clone's order-9 column (1,551 MB of values) the whole-column single call
# measured 1,096 MB over plateau in-process against 184 MB blocked at 512,
# the latter a high-water over the whole loop rather than one slice.
# Neither reconciles 1:1 with the bytes handed over, and the decision only
# needs the ordering they agree on: whole-column spikes ~1.1 GB, blocked
# stays near the floor, for ~0.1 s of predicate wall (0.88 s whole-column
# vs 0.98 s blocked, min-of-3 in-process). Same bytes-not-records argument
# as above: 512 rows is ~1.2 MB of column per slice at order 9 and ~30 MB
# at order 13.
The decision logic is untouched — whole-column spikes ~1.1 GB, blocked stays near the floor, ~0.1 s of predicate wall buys it — which is the part that never depended on the attribution being exact.
The same overdrawn phrasing appeared twice more downstream and is corrected in the same commit: the _intersect_footprint_cells docstring ("the binding's input copy alone measured ~1.1 GB" → "the whole-column clone call measured ~1.1 GB of RSS over plateau against ~184 MB blocked") and the inline comment at the predicate loop, which now points here for what the numbers do and do not say.
|
🤖 from Claude CI note for the record: the first two runs on today's phase-5 commits showed one 3.13-only failure in |
|
🤖 from Claude Follow-ups from the open review questions are now tracked, so nothing is lost at merge: #427 (Q2 — batch |
Closes #396.
ShardMap.buildspends its wall clock in per-granule Python→Rust call overhead, not in geometry: the o5-vs-o9 datapoint on the issue (comment) shrank cell work ~16× but wall only 3.2×, so ~2/3 of a full-catalog build is fixed per-call cost. This PR eliminates that cost rather than amortizing it, per the premise correction: the original phase-1 "process pool" is not implemented and is not planned here. A pooled prototype was tried and abandoned — it spiked ~85 GB and took the machine down, which is exactly what a batch call into a GIL-releasing, rayon-parallel Rust core makes unnecessary.Approach — batch, don't parallelize
mortie 0.9.4 ships
polygons_to_morton_mocs(lats, lons, offsets, order=, normalize=, tolerance=, max_cells=)(espg/mortie#153): a ragged many→many coverage entry point, strict arrow offsets, GIL released for the whole batch, rayon across polygons, chunked internally so the ragged result assembles without a 2.5× concat peak._intersect_mortie's HEALPix branch now:_flatten_rings) into mortie's ragged layout plus anownersarray mapping ring → record index;_batch_ring_mocs) instead of onemorton_coverage_moccall per granule;searchsortedover the sorted shard array — the vectorized shape the non-HEALPix branch below it already used — replacing the scalarif s in all_shardstest that ran once per MOC cell;The ring → granule map is the load-bearing piece
_granule_footprintsyields one ring per granule inswathmode but three per granule inbeamsmode (issue #65), while mortie's batch is one-ring-per-entry by design. Flattening without a back-map would silently turn "a granule's shards = the union over its rings" into "each ring is its own granule". So_flatten_ringsreturnsowners, and the shard-side regroup dedups by owner:That consecutive-run dedup is complete, not approximate: records are visited in order and a granule's beam rings are adjacent, so
ownersis non-decreasing globally; a stable sort by shard therefore leaves each shard's owners non-decreasing, and every repeat of a granule within a shard is adjacent. This is the same set the olddict.fromkeys(v)produced, in the same order.Blocking, and the block size
The batch is blocked rather than one giant call because the MOC words are the dominant allocation.
_MOC_BATCH_RINGSis 32 — set from the min-of-3 real-catalog sweep in Measured, not from a synthetic fixture. Its two prior values (1024, then 256) were both chosen against synthetic footprints and were both too large; see What the real data corrected.Peak memory has two terms and blocking bounds only one of them, so quoting it as "proportional to the block" (as an earlier revision of this body did) is wrong in the one direction that matters:
_MOC_BATCH_RINGS× per-ring MOC words. California at order 13 peaks 57 MB at block 32, 251 at 64, 412 at 256, 616 at 2048;_flatten_ringsconcatenates every granule's vertices into one pair of flat arrays before any block runs. On the 555,867-granule clone that is 17,681,679 vertices × 16 B = 282.9 MB, plus 8.9 MB of offsets/owners: a ~292 MB floor no block size removes. Plus thehit_shards/hit_ownersaccumulators, which scale with pairs (the 88S serial row's 85 MB at 2.07 M pairs is that term alone).So a build's peak ≈ catalog floor + block buffers, and the operator-scale figure cannot be read off the block alone.
The AOI filter also runs per ring, as each ring's cover lands, not once per block. Accumulating the block's densified cells first built a buffer that dwarfed the AOI hits it existed to produce, plus two
np.concatenatecopies of it. Filtering per ring is just as vectorized and keeps the accumulator bounded by hits, as the pre-#396 loop was;owners[start + r]is non-decreasing inr, so the sort/dedup invariant is untouched.Failure semantics preserved
The serial path wrapped each granule in
except Exception: continue, so one malformed footprint cost one granule. The batch call is fail-fast for the whole block (it raises naming the lowest-index offender). Two things restore the old contract:_flatten_ringsscreens the documented rejection causes up front — fewer than 3 vertices, mismatched lat/lon lengths, a non-finite coordinate — dropping those rings quietly. Verified against mortie: scalar and batch reject exactly the same inputs and accept the same odd-but-legal ones (degenerate, collinear, self-intersecting, out-of-range latitude, unclosed rings all cover identically in both), and the malformed-ring test pins that equivalence against the serial oracle rather than a hard-coded expectation.RuntimeWarningper build (not per block) naming the mortie exception — resolving question (3) below.moc_to_orderstays per-ring and keeps itstry/except: its cell budget can refuse a huge expansion, and that dropped the ring before this PR too.Dependency floor bump (existing dependency, not a new one)
mortie>=0.9.3→mortie>=0.9.6inpyproject.toml, in three steps: 0.9.4 for phase 1'spolygons_to_morton_mocs, 0.9.5 for phase 3'smortie.arrow.from_wkbs, and 0.9.6 for phase 4's MOC batch twins. This is a floor bump on an existing core dependency — no new package, no new transitive deps, no footprint change.polygons_to_morton_mocs(Batch polygon coverage: polygons_to_morton_moc over ragged (offsets) arrays — one call, rayon across polygons espg/mortie#153).mortie.arrow.from_wkbs— the pyarrow skin (mortie.arrow.from_wkbs: arrow skin over the WKB batch, so a geoparquet column skips Python bytes objects espg/mortie#163) over the backend-free Rust WKB reader (Parse WKB in Rust: backend-free geometry ingest, plus the plural from_wkbs batch espg/mortie#157).arrow.pyexists in 0.9.3 and 0.9.4 butfrom_wkbsis in neither, so phase 3 is not implementable below 0.9.5. Verified against the installed wheels: 0.9.4'smortie.arrowexportsEXTENSION_NAME, export_c_array, export_c_schema, from_morton_index, import_c_array, morton_index_type, to_morton_index; 0.9.5 addsfrom_wkbsandpolygons_to_morton_mocs.mocs_and— the 1×N broadcast intersection (Batch MOC set operations: mocs_and / mocs_intersect and the 1xN broadcast family espg/mortie#173, landed in Batch MOC set ops: mocs_and / mocs_intersect + scalar moc_intersects (issue #173) espg/mortie#174) that phase 4's stored-index swap pairs withmocs_to_orders(itself a 0.9.5 arrival, mocs_to_orders: ragged batch moc_to_order — plus an API sweep for bulk-by-default operators espg/mortie#156). 0.9.5 has no batchmoc_andof any shape, so phase 4 is not implementable below 0.9.6. Verified against the installed 0.9.6 wheel from PyPI.Safety note: 0.9.5 deletes
mortie/tools.py(espg/mortie#159's domain split) — the modulemort2polygon/mort2geoused to live in. The bump is safe because #411 landed first:git grep 'mortie\.tools' origin/mainreturns nothing, and the six sites that existed (data/build_aoi_shardmap.py,data/conus/build_conus_shardmap.py,demo/05_california_read.ipynb,notebooks/aoi_mask.ipynb,src/zagg/grids/healpix.py×2) came off this branch in theorigin/mainmerge below. Confirmed against the installed 0.9.5 wheel: notools.pyon disk, andimport mortie.toolsraisesModuleNotFoundError.pyproject.tomlis the only version spec that needs to move (question (1), now resolved):deployment/aws/build_layer.sh:92-95derivesMORTIE_SPECfrom[project.dependencies]viatomllib(issue #322), so the Lambda layer picks the floor up with no infra edit — which is why nothing underdeployment/aws/is touched here.uv.lockis gitignored and no CI job runs--frozen/--locked.CLAUDE.md§7 carried a stale>=0.7.2in its prose description of the dependency; cd78699 moved it to>=0.9.4per the "across all of zagg" ruling, 9005c3a carried it to>=0.9.5, and 5b74d70/6988e9e6 then removed the restatement fromCLAUDE.mdentirely (its §7 now points atpyproject.tomlas the single source), so the 0.9.6 sweep (fbc67b2) has exactly one floor site to move. A fresh grep formortie>=/mortie >=/mortie ≥/MORTIE_SPEC/MIN_MORTIE_VERSIONacross the tree (excludingarchive/) confirms it: the only floor spec ispyproject.toml's, now>=0.9.6; every other hit states when an API arrived (below). No CI workflow pins a mortie version.Left alone deliberately: the
mortie >= 0.8.3/>= 0.8.4/>= 0.9.3mentions indocs/aoi_mask.md,docs/ragged_layout.md,src/zagg/grids/aoi.py(MIN_MORTIE_VERSION) andsrc/zagg/grids/morton.py. Those state when a particular API arrived, not the package floor, and they stay true; rewriting them to 0.9.6 would erase provenance.src/zagg/catalog/shardmap.py'smortie >= 0.9.4note onpolygons_to_morton_mocsandsrc/zagg/catalog/sources.py'smortie >= 0.9.5note onfrom_wkbsare the same kind and stay. Flagging in case the ruling was meant to cover them too.Coordination note: a concurrent local edit to
pyproject.tomlin the primary checkout (a comment block documenting thelambdaextra's numpy/pandas pins) was inspected read-only before this sweep; it does not touch the mortie hunk, so the two changes cannot conflict and nothing from it is replicated here.Merged
origin/mainThe branch was 92 commits behind, brought current by merging
origin/mainin (3d46aa0) rather than rebasing — §1 forbids force-pushing, so a rebase was not available. The merge was clean: no textual conflicts and no semantic ones either, becausemaintouched nothing undersrc/zagg/catalog/in those 92 commits (git diff --stat ad8aa30 origin/main -- src/zagg/catalog/ tests/test_shardmap.py bench/ pyproject.tomlis empty; the only overlap indocs/is the newdocs/hive_layout.md). What the merge did bring is #411's removal of the last sixmortie.toolsimports — which is what makes the 0.9.5 floor safe (above).Measured — real catalogs (phase 2)
Every number below comes from
bench/shardmap_batch_vs_serial.pyagainst real CMR catalogs in the tree. The previous synthetic figures are withdrawn — see What the real data corrected.Grid is the shipped production one (
tests/data/benchmark/configs/atl03_tdigest_healpix_o9.yaml:parent_order 9,child_order 19,chunk_inner 13), so a default build resolves the MOC order to 13 (#92); 9 is also swept because it is the legal floor the issue's baselines used. Every measurement runs in its own process, sinceru_maxrssis a monotone high-water mark and serial and batch cannot share one. Batch output is asserted equal to the serial oracle on every row except the one row that has only a batch arm, which is labelledno-assertby the harness (see below).Peak is measured against the load's resident plateau, not its high-water. These are not the same number: on the
fullcasegranule_records()leaves a transient above the plateau it settles at, and a maxrss-vs-maxrss delta silently absorbs the intersection's first few hundred MB and floors at 0 — which is why an earlier revision of this table reported0 MBcells as if they were measurements. The harness now reports both (--columnsbat_MB/ser_MBagainst the resident plateau,bat_hw/ser_hwagainst the high-water). Where thehwdelta is > 0 the resident-baseline figure is the intersection's exact increment; wherehwis 0 the intersection never rose above the load's high-water, so the figure is an upper bound and is marked≤.neonand88sare committed fixtures (tests/data/benchmark/catalogs/) and run in any checkout — every path resolves relative to the script (previously they were absolute paths into one working copy, so they would have skipped anywhere else).californiaandfullneed the 305 MB ATL03 clone (data/atl03_v007/, not committed) and skip cleanly when it is absent via_available(); that guard is new here rather than a convention inherited frombench/neon_order_sweep.py, which reads the clone unguarded.All numbers below were re-measured at the shipped block of 32 after the block change, on a machine at load average 6–20 (walls are therefore upper bounds; the block-size decision is made on min-of-3, below).
The
fullrow is the issue's headline baseline reproduced: 279.3 s against the issue's 310.7 s for the same single-pass mortie build, and it lands on the same 190,625 pairs / 2,721 shards as the bbox-cutcaliforniarow — an independent check that the cut is not dropping assignments.Speedup is 2.9–4.6× across every real fixture and both orders, with the operator-scale case at 4.4×.
The
fullrow is at order 9 to line up with the issue's baseline. The order-13 arm of the same case — the order a default build actually resolves to — is run batch-only, because its serial arm is ~1 h. It is the one row in this PR with no oracle assert and the harness labels itno-assert (one arm only); what backs it instead is thecaliforniao13 row above, same AOI and order with both arms on identical output, plusTestMortieBatch'sdict ==pins. Reproduce it with--cases full --orders 13 --arms batch:_MOC_BATCH_RINGS = 32(shipped)_MOC_BATCH_RINGS = 256(previous)Same 190,625 pairs / 2,721 shards both ways. Walls (1,065 s and 929 s) are not quoted as a comparison — they ran at load average 18–29 against other work on the machine.
This is the measurement question (6) asked for, at the exact scale the withdrawn 143 GB was extrapolated to: 1.5 GB at the old block, 545 MB at the new one. Note that the old high-water-baseline figure for the 256 arm reproduces exactly (1,217 MB against the 1,216 MB previously reported); it is the baseline, not the run, that changed.
Block-size knee, re-validated on real footprints
Same harness,
californiaat order 13, each row its own process, wall reported min-of-3 (--knee california --order 13 --reps 3). Min-of-N is not optional here:polygons_to_morton_mocsis rayon-parallel, and single-shot walls on this machine scattered up to 57% within a block — wider than the block-to-block effect being measured. The earlier single-shot version of this table was internally inconsistent for that reason (block 24 beat 32; block 512 beat 64).At order 9 the sweep is flat in both columns (16 → 0.73 s / 15 MB; 32 → 0.66 s / 15 MB; 64 → 0.64 s / 16 MB; 1024 → 0.58 s / 32 MB). At 88S order 13 the picture is different from California's and worth stating exactly, because it is the case that most constrains the choice: min-of-3 gives 32 → 63.13 s / 200 MB, 64 → 55.02 s / 210 MB, 256 → 53.74 s / 840 MB — but re-running 32 and 64 with the block order reversed gives 64 → 56.55 s, 32 → 57.37 s. So 88S's 32-vs-64 wall gap is ~4% (min of 6 across both orderings), not the 15% the block-major sweep suggested and not the "32 is faster" the previous single-shot row claimed. Block-major sweeps correlate block size with drifting machine load; that is now noted in
shardmap.py.Verdict: the knee did not hold, and
_MOC_BATCH_RINGSmoves 256 → 32 (question (7), resolved to 32 rather than 64). The decision is made on the peak column, which reproduces across three independent runs, rather than on wall differences that sit inside single-run scatter:Memory is worth more than 2–4% of wall on a 2 GB Lambda, so 32 ships. The claim that peak "keeps climbing with the block all the way out" is withdrawn — 88S goes 1,057 → 1,010 MB from 256 to 1024 — as is "within ~6% of the asymptote in every case", which 88S violated at +6.4%.
What the real data corrected
fullbuild — 555,867 granules, the exact case the 143 GB was extrapolated to — peaks at 406 MB over its resident load plateau at order 9. The mechanism sentence that stood here before ("peak tracks the block size … not the catalog") was wrong and is replaced by the two-term model in Blocking, and the block size: a catalog-proportional_flatten_ringsfloor (~292 MB at this scale) plus block-proportional MOC buffers. The refutation is unaffected — 406 MB against 143 GB is five orders of magnitude — but the model the next extrapolation gets built on is now the right one.granule_records()is the real memory story at operator scale, not the intersection — but not for the reason stated here before. The plateau is real and slightly larger than reported: 137 MB interpreter → 1,353 MB afterfrom_geoparquet→ 4,038 MB resident aftergranule_records(), i.e. +2,684 MB. The coordinate arrays are only 282.9 MB of that, 7%. The dominant term is the seven whole-tableto_pylist()calls atsources.py:525-552, all live simultaneously (~1.8 GB), of whichassetsalone is 1,147 MB — 2,064 B/row, 28% of the plateau — while the loop reads exactly two hrefs out of each dict; geometry WKB is 305 MB, thestrvalues 304 MB, the dict objects 151 MB,id49 MB, the two datetimes 63 MB. So the big lever is not vectorizingshapely.from_wkb(≤15% of the plateau, which is what the on-holdgranule_recordswork targets) but not materializingassets/geometrytable-wide — project the two href fields in Arrow, or batch theto_pylist()calls, and ~1.5 GB goes away with no API change. Scoping that is a separate call; flagging it rather than doing it here. (RSS also does not fall after those intermediates go out of scope or afterdel cat— allocator-retained, so peak and steady state are the same number at this scale.)Phases
_intersect_mortie(HEALPix branch): flatten rings + ring→granule map, blockedpolygons_to_morton_mocs, vectorizedsearchsortedmembership, stable-sort regroup; mortie floor to 0.9.4; identity tests.bench/shardmap_batch_vs_serial.pyreplaces the synthetic numbers with the table above: four real catalogs from 2,089 to 555,867 granules, wall and peak RSS, serial vs batch, both orders, process-isolated. Block size re-validated on real footprints, min-of-N, with the blocks/order/reps all CLI-settable so every quoted row is regenerable. (The original phase 2 — vectorizingshapely.from_wkbingranule_records— is on hold pending an architecture decision and is not implemented here; item (4) below is why it is probably aimed at the wrong term.)footprint_cellscatalog column (issue phase 2): per-granule morton MOC at a fixed order in the catalog geoparquet via the mortie arrow skin,buildfast path = AOI MOC ∩ per-granule MOC set ops with no geometry work; refuse loudly when the shard order is finer than the column order; docs note it as a zagg-clone convention column whose staleness is structurally impossible (it rides in the same file as the geometry it was computed from). Section below.moc_and/moc_to_orderloop for onemocs_and+mocs_to_ordersper block of records (Batch MOC set operations: mocs_and / mocs_intersect and the 1xN broadcast family espg/mortie#173), blocked at_CELLS_BATCH_RECORDS = 512on the measured clone-scale memory sweep; mortie floor to 0.9.6 across the tree per the question (1) ruling; permanent scalar-parity test; bench harness grows--knee-arm cellsand the measured tables are regenerated. Section Phase 4 below.mocs_intersectprefilter on the stored-index path (8347050): the phase-4 section's flagged future work, measured and landed — a blocked range-walk predicate over the row-aligned column gates the materializingmocs_andpass to surviving records only (~0.4% at clone scale). First production consumer of Batch MOC set operations: mocs_and / mocs_intersect and the 1xN broadcast family espg/mortie#173'smocs_intersect, closing the loop on that op's consumer-evidence rationale. Clone-scale wall 2.53 s → 1.70 s min-of-3 at an unchanged peak; the input-copy term verified at clone scale and blocked; parity pinned by the harness digest on every row plus new edge tests (review fold 21065c0…4753c317). Section Phase 5 below.Phase 3 — the
footprint_cellscolumnThe per-granule footprint→cells cover is identical for every build against the same catalog, so the catalog carries it.
Catalog.index_footprints(order)covers the wholegeometrycolumn with onemortie.arrow.from_wkbscall and appends a raggedlarge_listof morton MOC words, typed with mortie'smorton_indexextension (the same conventionShardMap.to_parquetanddocs/morton_arrow.mduse, and it survivesstac_geoparquet.to_parquet— pinned bytest_column_is_morton_typed_on_disk). The order lives in the catalog's own metadata asfootprint_cells_order.python -m zagg.catalog --index-footprints ORDERdoes it while fetching, so a saved clone ships pre-indexed.ShardMap.buildthen does no geometry at all (phase 4 shape — two batch calls per block of records):Intersecting with the AOI first is what makes the
searchsortedmembership filter of phase 1 unnecessary:aoi_mocis a union of wholeparent_ordercells, so every cellmocs_to_ordersyields is inall_shardsby construction. The regroup is phase 1's, now shared as_regroup_hitsso the two paths cannot drift.Phase 4 — the per-granule loop is gone: mortie 0.9.6's batch twins
An earlier revision of this section recorded that mortie 0.9.5 had "no
mocs_intersectpredicate and no N-way variadic fold" and that the fast path therefore loops in Python calling scalarmoc_andper granule. That is no longer true and the loop is no longer here. mortie 0.9.6 shipsmocs_and— the 1×N broadcast ofmoc_and, one shared operand against a ragged column, GIL released, empty results kept as zero-width slots (espg/mortie#173/#174) — and phase 4 swaps the scalar loop for it, chained into the raggedmocs_to_orders(0.9.5, espg/mortie#156). Two Python→Rust crossings per block of records replace two calls per granule, and the AOI operand's BMOC is built once per block instead of once per granule.mocs_intersectwas deliberately not used as the per-slot answer: this call site needs the intersection's cells (they are the shards aftermocs_to_orders), never a bare emptiness test — empty slots ride through both batch calls for free. Phase 5 later adds it in the one role compatible with that, a prefilter that gates which records reach the materializing calls at all (section Phase 5 below).The swap was measured before it was adopted — the phase-4 measurement comment carries the full method, profiles and parity assertion; its numbers against the reproduced scalar baseline on this machine:
_intersect_footprint_cellsalone 0.225 → 0.052 s, boundary crossings 4,354 + 2,356 → 1 + 1 per block. Themoc_andterm itself is 11× — honest agreement with Batch MOC set ops: mocs_and / mocs_intersect + scalar moc_intersects (issue #173) espg/mortie#174's ~15× tiny-operand floor, not a shortfall against its 121–169× at-scale row.Blocked, because the memory posture was measured too. The measurement's whole-catalog single call peaked at ~5.2 GB over the load plateau at clone scale — a record-aligned gather copy of the column plus mortie's documented input copy (
mocs_andcopiesvalues/offsetsbefore releasing the GIL), both catalog-proportional — against the scalar loop's ~1.7 GB. So the shipped swap wraps the batch calls in blocks of granules,_CELLS_BATCH_RECORDS = 512, the same shape of constant as phase 1's_MOC_BATCH_RINGS. The clone sweep (--knee full --knee-arm cells, each row its own process, min-of-3 at the finalists):Blocking is at worst free on wall — unblocked runs measured 3.83 s (this session, single shot) and 3.04/3.57/4.16 s (the measurement session, n=3), against 2.49–2.55 s at every block from 512 to 16384 — and ~1,736 MB is the o9 floor, the sweep's own minimum (1,736.5 MB at 1024, 1,736.9 at 512): column materialization plus plan bookkeeping no block size removes. So 512 puts the batch path's peak at the scalar loop's own ~1.7 GB while keeping the 7.4–7.5× above. 512 over 2048 is a bytes argument, not a records one: per-record MOC size grows ~40× from an order-9 column to an order-13 one, and on the anti-recommended o13-indexed configurations the smaller block is what bounds the damage (88S o13: 2,627 MB at 512 vs 4,063 at 2048, walls 4.62/4.59 s; California o13: 295 vs 1,261 MB). The per-block cost of rebuilding the shared AOI operand's BMOC is measured, not assumed, negligible: 104 µs/call at the real 2,721-cell California operand — ~113 ms across the clone's 1,086 blocks, ~4% of the arm.
One semantic change rides along, stated rather than hidden. The scalar loop wrapped
moc_to_orderinexcept Exception: continue, so a cell-budget refusal silently dropped that one granule.mocs_to_ordersapplies the same per-MOC budget but refuses the whole call (ValueErrornaming the lowest-index offender), so a refusal now fails the build loudly — and since phase 4's review fold (290bfcc) the re-raise names the block's record range so the offender is identifiable. Reaching it requires a single granule spanning ~10⁶ shard cells inside the AOI: the expansion is bounded by the AOI-clipped footprint densified atparent_order, not zero — compressed operands meanmocs_to_orderscan genuinely expand, so "unreachable" would be too strong (the review thread on 669a09b carries the exact bound). Loud-over-silent is the intended trade, recorded in the_intersect_footprint_cellscomment; the geometry path's per-ring refusal still drops just the ring, a deliberate divergence also recorded there. (A malformed stored word still panics in Rust and aborts the build in both versions, unchanged.)Parity is a permanent test now, not just the measurement's assertion:
test_batched_cells_equal_the_scalar_loopkeeps the pre-phase-4 scalar loop verbatim in the test file as_intersect_cells_serial(the same one-implementation-ships pattern as_intersect_mortie_serial) and asserts exact dict equality — same keys, same granule lists, same order — over both a matched-order column and a coarsening one (index 13 against parent 11, the fold's 52e2125, which is what exercises dropping the scalar loop'snp.unique), at blocks 1, 5, 7 and 24 over a fixture with zero-width slots, a dropped non-polygonal row (so the block gather walks non-contiguous column spans) and ragged-tail block boundaries. The harness digest assert is the same check at scale: identical digests across arms on every asserted row below, including batch-vs-cells on the full 555,867-granule clone (-8201871529712101455, the measurement comment's digest reproduced).Phase 5 — the
mocs_intersectprefilter (the flagged future work, measured)Phase 4 flagged it; this phase lands it. At clone scale the batch calls scanned all 555,867 stored MOCs against the AOI with ~99% of slots coming back empty, paying a record-aligned gather (plus mortie's input copy) for slots that materialize nothing.
_intersect_footprint_cellsnow runs mortie 0.9.6'smocs_intersect— espg/mortie#173's range-walk predicate, espg/mortie PR 174's implementation — over the column before the blockedmocs_andpass, and only surviving records are gathered and materialized. This is that op's first production consumer, closing the loop on the consumer-evidence rationale it shipped under. (This is distinct from question (4)'sprefilter_order=idea, which is a coarse-order geometry prefilter on the geometry path; that question stands.)Two structural points:
granule_recordsdropped are walked too (index_footprintsgives them zero-width runs, which cost nothing) andhits[rows]discards them.surv = np.flatnonzero(hits[rows])— original record indices, strictly increasing — and the gather blocks now cut at survivor counts with ownerssurv[start : start + B], neverstart + i.survincreasing is what keeps_regroup_hits' non-decreasing-owners invariant. Parity is pinned by the existing block sweep (test_batched_cells_equal_the_scalar_loop, whose interleaved empties make survivor blocks genuinely differ from record blocks) plus a newtest_prefilter_edges_all_empty_all_hit_and_middle_run(named for what its survivor shape really is — a contiguous middle run{4, 5}behind leading and ahead of trailing empties, not an interleave): all-empty (a disjoint but non-empty AOI, so the empty-inputs gate does not fire and the predicate itself must drop every record), all-hit (the survivor gather is the identity; every record pinned assigned), and the middle run, wheresurv[0] != 0— the off-by-one shape a block-local owner mapping would fail — each against the scalar oracle at blocks 1, 3, 4 and 10. The review fold hardened this further: the refusal message is exercised on a gapped, non-0-based survivor set (surv == [3, 6],test_batch_refusal_range_reads_the_survivor_owners), an over-reporting predicate is pinned to still return{}rather than die in the regroup (test_predicate_overreport_still_returns_empty, restoring a one-branch guard), and the edge fixture's cell-quantization margin is argued from the granule gap withcos(lat)accounted (0.101° arc ≈ 3.5 order-11 cells).The input-copy term was verified at clone scale, not assumed — and it blocks the predicate too. mortie documents
mocs_intersect's peak as the binding's input copy; on the clone's order-9 column (values are 1,551 MB) the whole-column single call measured 1,096 MB over plateau in-process, and 3,305 MB vs 1,739 MB at block 512 in the harness knee (--knee full --knee-arm cells --blocks 512,4096,65536,555867; walls 1.75 / 1.55 / 1.69 / 1.77 s — flat, single-shot scatter). So the predicate reuses_CELLS_BATCH_RECORDSas contiguous row-slices: ~0.1 s of predicate wall (0.88 s whole-column vs 0.98 s blocked, min-of-3 in-process) buys ~1.1 GB less peak, and the sizing argument is the same bytes-not-records one as the gather pass (512 rows ≈ 1.2 MB/slice on an order-9 column, ~30 MB on an order-13 one).Blocking the survivor pass stays too. At clone scale survivors are 2,356 of 555,867 (0.42%) — five blocks, where one call would also be fine. But survivors are AOI-proportional, not catalog-proportional: an AOI covering the catalog passes everything, and an unblocked survivor pass is then the ~5.2 GB whole-catalog call phase 4 blocked away. Blocking measured at-worst-free on wall, so
_CELLS_BATCH_RECORDS = 512stays for both passes; its justifying comment is updated in place with these numbers.Measured — against the phase-4 blocked baseline (same harness, same process isolation; digest asserted green between arms on every row below, and the clone row's digest is the measurement comment's
-8201871529712101455exactly):(Phase-4 fixture rows were min-of-3 at order 9 and min-of-2 at order 13; the prefiltered rows are min-of-3 throughout, batch↔cells arms in the same run. The batch arms reproduced within ~1–4% of their phase-4 values in the same children — 88s o13 batch 55.93 vs 56.47 s — so the sessions are comparable.)
The verdict, stated honestly: the win is where the empty slots are, and only there. The operator-scale case — the clone against a regional AOI, 99.6% of slots empty — goes 2.53 → 1.70 s (−33%) at an unchanged peak: both sit on the ~1.74 GB column-materialization floor that no block size or prefilter removes. Fixture populations that mostly hit pay the predicate as pure overhead: +8% on 88s o9 (0.04 s absolute) and +30% on 88s o13 (4.61 → 5.99 s min-of-3) — the fat-column configuration the order section above already anti-recommends indexing at. Neon, mostly-empty like the clone, gets faster at both orders. Landed because the win falls on the recommended configuration (index at the shard order) at the scale that motivated the phase, and the regression is confined to the anti-recommended one — recorded rather than smoothed, and if an o13-indexed column ever becomes a supported posture the prefilter should be re-measured there.
With the prefilter in, the clone arm's remaining ~1.7 s is the predicate's own blocked walk over the 555,867-MOC column (~1.0 s in-process) plus the id→row plan alignment; the survivor gather +
mocs_and+mocs_to_ordersare ~50 ms combined. mortie is the majority term again — a faster path, if ever needed, is upstream, not more zagg-side numpy.Engagement gate, and the refusal
The stored cover is mortie MOCs at the column's own order, so it may only answer a build that asked for exactly that. The fast path engages when all of: the resolved backend is
mortie;footprint="swath";mortie_orderwas not pinned by the caller; the grid is HEALPix; the column is present. Anything else takes the geometry path unchanged — in particular an exact-S2spherelyrun is never silently swapped for a MOC one (the ~0.01% polar omission, espg/mortie#32).metadata["footprint_cells"]records the verdict:Truewhen the index answered the build,Falsewhen the catalog is indexed but the build took geometry anyway. It is a bool rather than a present/absent key because the catalog's ownfootprint_cells_orderrides into the manifest either way, so a bare absent key beside it read as if the index had answered (a74a4fc9). A manifest with neither key came from a catalog that was never indexed.A column coarser than the grid's
parent_orderraises. Answering it would refine every cell onto all4^(parent_order − order)descendants and put ~every granule in ~every shard — the #92 failure — and silently falling through to geometry would hide that the index the operator paid for is useless for this grid. Pinned bytest_column_coarser_than_the_shard_order_is_refused.Row alignment is the load-bearing piece
granule_records()skips rows whose geometry is empty or non-polygonal, while the column has one entry per table row. Record index is therefore not row index, and a fast path that assumed it would hand every granule after the first gap its neighbour's footprint. Two things close that:index_footprintsscreens those rows with the same shapely predicategranule_recordsuses and gives them a zero-length run, so a catalog carrying a strayPointindexes rather than raising (mortie's coverage refuses a point outright, naming the blob). The screen costs 0.02 s against 2.65 s for the order-9 cover on the 35,639-granule 88S catalog — under 1% of a pass that runs once per catalog in time. In memory it is the pass's peak, and it is the one termfrom_wkbs's internal chunking does not bound: on the 555,867-row clone RSS goes 835 MB after the parquet read → 1,169 MB afterto_numpy(a full WKB copy) → 1,794 MB with the shapely objects live, ~960 MB for a type check.del geomskeeps that from stacking with the cover, so it is a peak and not a leak, but a whole-clone index wants headroom for it and this body previously implied the pass was cheap in both. The all-True short-circuit (45e04331) removes a second ~334 MB WKB copy in the case that actually occurs — nothing screened out, true for every catalog in the tree. Dropping the screen's peak outright means reading the WKB geometry-type word directly instead of building 555k shapely objects; not done here, because it trades the shared shapely predicate for a hand-rolled one.buildaligns records to rows by granule id, not by position, andtest_row_alignment_survives_rows_granule_records_dropsputs aPointand an emptyPolygonbetween good granules and asserts the whole manifest still equals the geometry build's.Measured — same harness, same conventions
bench/shardmap_batch_vs_serial.pygains a third arm,cells, alongsideserialandbatch: same process isolation (one child per measurement, sinceru_maxrssis a monotone high-water mark), same two peak baselines (resident plateau and high-water),--repsmin-of-N now on--casesas well as--knee, and the digest assert extended to require every arm present to agree exactly. The indexing pass runs in the same child but is timed separately asidx_s, because it is one-time per catalog where the other arms are per build. Phase 4 extends the same harness to the swap:--blockwith--path cellsoverrides_CELLS_BATCH_RECORDS(it overrode_MOC_BATCH_RINGSon the batch arm already), and--knee <case> --knee-arm cellssweeps it.Batched
cellsarm (phase 4, shipped block 512), re-measured rows — clone cases included this run since the clone was present; min-of-3 at order 9, min-of-2 at order 13:Every row above carries the harness digest assert between the arms shown (
serialis the geometry oracle; the o13 andfullrows assert against thebatchgeometry arm instead because their serial arms are minutes-to-an-hour). Thefullrow'scellswall floats 2.53–3.00 s across runs (the min-of-3 knee row is the tighter number); its peak likewise measured 1,305 MB in this child against 1,737–1,741 MB in the knee children — allocator-level scatter at this scale, quoted as the range rather than smoothed.Pre-swap scalar-loop rows, kept for the record (mortie 0.9.5-shape code, measured before phase 4 — the first three from this table's previous revision, the last from the measurement comment):
Against phase 2's geometry batch the index is now 12–23× per build at matched order (the harness's
b/ccolumn: neon o13 17.8×, 88S o13 12.2×, full o9 23.4×). Against the default build it is more still, because a default build's geometry path resolves the MOC order to the grid'schunk_order13 while the index answers from order 9 — the same manifest either way (below), so the honest end-to-end comparison isbatch@13vscells@9: neon 4.62 s → 0.01 s, 88S 56.47 s → 0.51 s (~110×).That cross-order identity is not assumed.
test_finer_column_coarsens_to_the_same_buildpins it synthetically (order-13 column,parent_order11 grid), and it was checked on both committed real catalogs through the public API —ShardMap.build(cat, grid, backend="mortie")(geometry, order 13) vsShardMap.build(cat.index_footprints(9), grid, backend="mortie")— identicalshard_keysand identical per-shard granule id lists onneon(4 shards / 286 pairs) and88s(564 shards / 2,074,785 pairs).The identity holds on single-part footprints, which is every CMR ATL03/06 granule and every catalog in the tree. The one intended divergence is a MultiPolygon:
index_footprintscovers the union of the rings in each blob whilegranule_recordsreads only the largest part's exterior ring, so the column is a superset and the index can place such a granule in shards geometry misses. That was stated as unqualified identity here and indocs/api/catalog.md; both are now qualified and the superset is pinned as the intended answer bytest_multipolygon_is_a_superset_not_an_identity(7fd25953). Antimeridian-split STAC footprints are the natural producer.The issue's "the o13 chunk order generalizes" is wrong, and the o13 rows are why
Issue #396's phase 2 proposed indexing at the chunk order. The measurements above say index at the shard order instead:
At the shard order the index pays for itself on the first build and its peak is within 8% of the batch arm's. At the chunk order it does not: the pass costs about as much as the build it replaces (53.3 s vs 59.3 s), the column is 33× larger, and the arm peaks at 2,269 MB against the batch arm's 213 — for resolution the order-9 shard cells cannot see. (Phase 4 sharpens the verdict rather than changing it: the batched build at o13 falls to 4.61 s so the 42.7 s indexing pass now costs ~9 builds, and the o13 arm's peak is the configuration
_CELLS_BATCH_RECORDS = 512was sized against.) Words per granule roughly double per order (88S: 32 / 64 / 137 / 288 / 682 / 1,529 / 7,207 at orders 6–13; NEON: 46 / 93 / 195 / 424 / 986 / 2,218 / 10,451). Extrapolating the per-granule figures to the 555,867-granule clone gives roughly 270–420 MB of parquet at order 9 against 9–13 GB at order 13 — an extrapolation, not a measurement, since the clone case is @espg's.index_footprints(order)therefore takes a required order with the tradeoff documented indocs/api/catalog.md, rather than defaulting to anything.Not a spec change
The column is upstream of the store:
docs/specification.mdis the Zarr store contract (ragged vlen-bytes layout, t-digest payload bytes, packed composition word, pyramid declarations, O11 content hash) and mentions neither "catalog" nor "geoparquet";tools/generate_spec_fixtures.pyhas zero catalog references. So this touches no wire format, no attrs grammar and no versionedspecmarker, and §4's same-PR spec+fixture obligation does not apply. Documented instead indocs/api/catalog.mdas a convention column: an indexed catalog is still an ordinary stac-geoparquet file that any other reader ignores.Staleness is structurally impossible, which is the reason for a column rather than a sidecar: it rides in the same file as the
geometryit was covered from, so there is no second artifact to re-sync and no version skew to detect. A subset carries it (filter_bboxtakes the column with the rows —test_filter_bbox_carries_the_column_aligned), and rewriting the geometry means rewriting the file.Out of scope, unchanged:
_intersect_spherely, the non-HEALPix mortie branch (already vectorized — it is the pattern phase 1 adopted), anddeployment/aws/.benchmarks/mortie_order_sweep.pyis left alone: it answers the order question, says in its own docstring that its catalog is synthetic, and already has a real-catalog successor inbench/neon_order_sweep.py(issue #202). (An earlier revision said this script "follows its conventions" for skipping when the clone is absent; it does not —neon_order_sweep.pycallspq.read_tableunguarded and raises. The_available()guard is new here.)Testing
Tests stay synthetic and hermetic — CI has no 305 MB clone. Phase 2 changed benchmarks only; the identity tests are untouched.
tests/test_shardmap.py::TestMortieBatch— 10 tests. The oracle is_intersect_mortie_serial, the pre-#396 loop copied verbatim into the test file (shardmap.py:307-340at ad8aa30, this PR's merge base). Keeping it in the test rather than inshardmap.pymeans one implementation ships while the replaced logic still stands as the reference. The pin is stricter than pair-set equality — it is fulldict ==, so per-shard granule order is pinned too and a build's manifest stays byte-identical. The benchmark script carries its own copy of the serial loop and says so, pointing at the test as the authoritative one: that copy is only ever the slow arm being measured, and any drift between the two shows up as a test failure rather than a quietly wrong benchmark.test_batch_equals_serial_swath— 12 overlapping SERC granules, order 11; asserts the fixture actually spans >4 shards and shares shards across granules before asserting identity.test_batch_equals_serial_beams— ATL03 swaths underfootprint="beams", assertinglen(_granule_footprints(...)) == 3first, so the multi-ring→one-granule union is genuinely exercised.test_identity_holds_across_block_boundaries—_MOC_BATCH_RINGSmonkeypatched to 5 over 12 overlapping granules (cuts at 5/10); asserts identity and that some shard drew granules from ≥2 blocks, so a passing test can't mean the cross-block regroup was never hit.test_build_pair_identity— the publicShardMap.buildpath against the oracle, by granule id.test_malformed_rings_dropped_not_fatal— NaN vertex / 2-vertex ring / lat-lon length mismatch between two good granules; both good ones still assign, and the whole result isdict ==the oracle, so drift between_flatten_rings' Python copy of mortie's rejection rules and mortie's actual rules is detectable. The all-rings-malformed case is pinned against the oracle too (both{}).test_batch_failure_falls_back_to_serial—mortie.polygons_to_morton_mocsmonkeypatched to raise for every call; result still equals the oracle.test_fallback_is_scoped_to_the_failing_block— block size 5, raise injected at block 0, 1 and 2 in turn. Assertsdict ==the oracle and that exactly two of the three blocks still went through the batch path, which is what pins the "for this block only" claim.test_fallback_warns_once_per_build— all three blocks fall back; exactly oneRuntimeWarningcomes out.test_empty_inputs_short_circuit— no records, and no shards.test_flatten_rings_offsets_contract—offsets[0] == 0,offsets[-1] == len(lats) == len(lons), every ring ≥3 vertices,owners == [0,0,0,1,1,1]in beams mode and[0,1]in swath mode.Phase 3 adds 19 tests, 11 in
tests/test_shardmap.py::TestFootprintCellsand 8 intests/test_sources.py::TestFootprintCells. The oracle for the build-side ones is the phase-2 geometry path itself, asserted as full shard-keys + per-shard granule-id equality, so the index can only ever be faster, not different:test_indexed_build_matches_the_geometry_build/test_finer_column_coarsens_to_the_same_build— order-11 and order-13 columns against an order-11 grid.test_column_coarser_than_the_shard_order_is_refused— the refuse-loudly path as its own test, matching on the message, not an incidental assertion.test_row_alignment_survives_rows_granule_records_drops— aPointand an emptyPolygoninterleaved with six good granules; every id still lands where the geometry build put it.test_gates_leave_the_index_unused(spherely backend, pinnedmortie_order),test_beams_footprint_leaves_the_index_unused,test_plain_catalog_still_takes_the_geometry_path— the four ways a build must not take the index.test_aoi_restricts_the_assignment— a region smaller than the catalog cuts shards, and matches the geometry build for that region. This is what pins themoc_and-instead-of-searchsortedclaim.test_batched_cells_equal_the_scalar_loop(phase 4) — the batch swap against the retired scalar loop, kept verbatim in the test file as_intersect_cells_serial: exact dict equality at blocks 1, 5, 7 and 24, on matched-order and coarsening columns, over zero-width slots, a dropped non-polygonal row and ragged-tail block boundaries.test_batch_refusal_names_the_record_rangepins the loud-refusal re-raise. See the Phase 4 section.test_empty_inputs_short_circuit— empty AOI and empty record list. Written first and it caught a real bug:compress_mocon a zero-length array panics in the rust core (PanicException: Morton index cannot be zero) rather than raising, so the short-circuit the geometry path already had is not optional here.test_cli_index_footprints_indexes_the_saved_catalog—--index-footprintsend to end with a fakedCMRSource: the persisted catalog carries the column and this build took the fast path (indexing after the write, or after the build, would each pass a weaker assertion).test_column_is_the_scalar_cover_of_each_footprint(every row's stored MOC is exactlymorton_coverage_mocof the ringgranule_recordsreads — the pin that makes the column trustworthy),test_non_polygonal_rows_get_an_empty_moc,test_all_rows_screened_yields_an_empty_column,test_reindex_replaces_rather_than_appends,test_geoparquet_round_trip_keeps_column_and_order,test_column_is_morton_typed_on_disk,test_filter_bbox_carries_the_column_aligned,test_absent_column_reads_as_none.Gates (re-run after the
origin/mainmerge and phase 3):ruff check src testsclean apart from the pre-existingN818onregistry.py:64;ruff format --check src testsclean apart from the pre-existingtests/data/benchmark/README.md;bench/shardmap_batch_vs_serial.pyis ruff-clean too.pre-commit runover the nine changed files passes ruff / ruff-format / codespell, and mypy reports the identical five pre-existingsrc/zagg/catalog/errors before and after the diff (same messages, shifted line numbers) — no new ones. Fullpytest -q: 3,682 passed before phase 3 → 3,701 after (+19), 37 skipped, 1 failed both times:tests/test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds— pre-existing, untouched here, local-environment-specific and green on CI. (tests/test_client_transport.py::TestStatusPoller::test_invoke_fault_burns_an_attempt_and_retries, flagged flaky on an earlier run, passed both full runs here.)Run on Python 3.13 with the exact-S2 spherely fork present, so the
TestBuildSpherelygate ran rather than skipped.shardmap.pyis 1,193 lines — under the 1,200 cap, but with 7 lines of headroom; see question (11).Phase-4 gates (fresh detached worktree, its own uv-resolved Python 3.12 venv, mortie 0.9.6 from PyPI, no spherely fork):
ruff check src testsandruff format --check src testsshow only the two known pre-existing offenders (N818atregistry.py:64,tests/data/benchmark/README.md);pre-commit runover the four changed files passes ruff / ruff-format / codespell, and the catalog-scoped mypy errors are the identical five before and after the diff (verified by stash-diffing the error lists; one line number shifts).pytest tests/test_shardmap.py: 96 passed pre-fold, 97 after the fold's refusal test. Fullpytest -q: 3,701 passed, 38 skipped, 4 failed — all pre-existing or environmental, none in the diff's blast radius: the knowntest_function_build_succeeds, plustest_parquet_round_trip/test_s3_prefix_stages_through_boto3/test_parquet_bytes_round_trip_without_pyarrow, which fail identically at the pristine phase-3 tip in this venv (fastparquet/pandas dtype-attribute drift in a fresh resolution; verified bygit stash+ rerun). CI's pinned matrix is the arbiter for those.shardmap.pyis now 1,305 lines after the review fold; see question (11) — exceeding 1,200 is espg-approved for this PR.Phase-5 gates (fresh detached worktree at 7efd38c, its own uv venv, Python 3.12, mortie 0.9.6 from PyPI, no spherely fork):
ruff check/ruff format --checkclean on the touched files (repo-wide only the knownN818and benchmark-README offenders);pre-commit runover the two changed files passes ruff / ruff-format / codespell; catalog-scoped mypy errors identical before and after the diff (stash-diffed).pytest tests/test_shardmap.py: 97 → 98 passed, and 100 after the review fold's two added tests. Fullpytest -q(pre-fold tip): 3,702 passed, 38 skipped, 5 failed — the four known environmental failures (re-verified failing identically at the pristine 7efd38c tip in this venv bygit stash+ rerun) plustest_invoke_fault_burns_an_attempt_and_retries, the previously-flagged flaky transport test, which passes in isolation.shardmap.pyis 1,354 lines, 1,369 after the fold (question (11): the growth is the prefilter plus its measurement-record comments).Questions for review
Lambda layer /Resolved — no infra edit needed.lambdaextra.deployment/aws/build_layer.sh:92-95single-sourcesMORTIE_SPECout of[project.dependencies]withtomllib(issue Move the mortie decimal-parse boundary off the private _decimal_to_word #322), so the layer installs against the floor frompyproject.tomlalone — still true at 0.9.5, and re-verified by grep across the tree.pyproject.tomlis the only version spec;CLAUDE.md§7's stale>=0.7.2prose is corrected in cd78699 and carried to 0.9.5 in 9005c3a.moc_to_orderis the remaining per-ring call. Ruled: out of scope for this PR even though 0.9.5 now shipsmocs_to_orders; filed upstream as mocs_to_orders: ragged batch moc_to_order — plus an API sweep for bulk-by-default operators espg/mortie#156 (a ragged batchmoc_to_order, the natural sibling topolygons_to_morton_mocs); adopting it in zagg is a follow-up PR, not this one. Worth noting from the real-catalog data: at order 13 it is now a much larger share of the remaining wall than the synthetic probe suggested, since the real MOCs it expands are ~40× bigger — so mortie#156 is worth more than the earlier "roughly a third" estimate implied. (Phase 4 update: the stored-index path now usesmocs_to_orders— mooting this question for that path — but the ruling's scope was the geometry path's per-ring call in_intersect_mortie, which stays scalar and stays a follow-up.)Silent block fallback.Resolved — implemented in d821597, oneRuntimeWarningper build carrying the mortie exception. Pinned bytest_fallback_warns_once_per_build.prefilter_order=onbuild— Ruled: wanted, but as its own PR, shipping with with/without comparison benchmarks. Real-data note for that PR: thefullcase is now 61.97 s single-pass at order 9, against the ~104 s the two-stage prefilter measured pre-batch, so the prefilter may now be a net loss at this order — the comparison should be run against the batch path, not the old baseline.Operator-scale California validation is still @espg's per the standing convention, and it is now scripted and path-portable (every input resolves relative to the script, so it runs from any checkout that has the clone):
uv run python bench/shardmap_batch_vs_serial.py --cases fullreproduces the 555,867-granule build end to end with wall and peak RSS on both paths.granules_assignedagainst the baseline is the invariant to check; this run reports the equivalentpairs/shardsand asserts batch == serial internally.Withdrawn. Re-scoped per espg's correction to "confirm peak-RSS behaviour at operator scale", and that confirmation is in the table: the 555,867-granule build peaks at 406 MB over its resident load plateau at order 9 and 545 MB at order 13 — against the extrapolated 143 GB. Even at the old block size it was 1.5 GB. No mortie issue is warranted on this evidence. (These figures are ~2.7–4× the ones first posted here: not a change in behaviour but a change in how peak is measured — see the note on baselines above. The direction of the error was to understate, and the refutation holds either way.)polygons_to_morton_mocsretains its output buffer / merge blocker.Block size 64 vs 32.Resolved — 32, shipped in 4df7beb. The second opinion this asked for came back against 64: the wall difference 64 was chosen on was inside single-run scatter (up to 57% within a single block), while the peak column reproduces across three independent runs. Re-measured min-of-3, 32 costs 2.3% wall on California for 4.4× less peak and ~4% on 88S for ~7% less peak. The 88S argument in that second opinion — that 32 was faster there — did not survive min-of-N either; see the knee section for the reversed-order re-run. The 7% wall figure quoted in this question was single-shot and is withdrawn.ShouldResolved — retired in 869ea65 ("if we keep a benchmark at all it runs on real data"). The file is gone; the order question is answered by the real-catalogbenchmarks/mortie_order_sweep.pybe retired?bench/neon_order_sweep.py.Should
backend="auto"prefer the index over spherely? Today it does not: the fast path is gated on the resolved backend beingmortie, so on a machine with the spherely fork installed,autostill picks exact-S2 and an indexed catalog buys nothing unless the caller passesbackend="mortie". That is deliberate — silently answering a spherely request with MOCs swaps the backend's semantics (~0.01% polar omission, morton_coverage: polygon-edge interpretation differs from S2 / shapely geodesic espg/mortie#32) behind the caller's back, and the Lambda/CI path (no spherely) already resolves to mortie. The alternative is that indexing a catalog is itself the operator saying "MOC assignment is what I want," in which caseautoshould prefer it and the gate becomesbackend != "spherely"explicitly. One-line change either way; leaving it conservative pending a call.index_footprintsdecodes the geometry column with shapely to screen it. That is the one place phase 3 still parses WKB in Python, and it exists only to reproducegranule_records' empty/non-polygonal skip so the column stays row-aligned. Measured at <1% of the cover it precedes (0.02 s vs 2.65 s on 88S at order 9), so it is not worth optimizing on wall — but it is the pass's peak in memory, now measured rather than estimated: on the 555,867-row clone 835 MB after the parquet read → 1,169 MB afterto_numpy→ 1,794 MB with the shapely objects live, ~960 MB for a type check. The all-True short-circuit (45e04331) takes the free half of that back; the rest stands. Options if that matters: read the WKB type code out of the Arrow buffer instead (vectorised, no GEOS, but a second implementation of a screen that must not drift fromgranule_records'), or try the whole column first and fall back to the screen only when mortie refuses a blob. Flagging rather than choosing.Ruled — exceeding is approved, verbatim: "exceeding 1200 is approved; we'll do a refactor later as spherely lands, and may make that the sole path in the future. Exceeding the line count is fine for now, and the least resistance path." The review fold did push it over as predicted — 1,224 lines, and phase 4 with its own review fold carries it to 1,305, and phase 5's prefilter (with its measurement-record comments) to 1,354, 1,369 after its review fold (the net code change of each is small; the growth is dominated by the constants' and passes' measurement-record comments). Nothing is split here, and the backend-split design (the two phase-3 functions into ashardmap.pyis at 1,193 of the 1,200-line cap.src/zagg/catalog/footprints.py) is deferred to its own issue rather than carried in this PR.