Optional strict-AOI cell mask (issue #101)#111
Conversation
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Phase-1 adversarial review against issue #101 acceptance criteria and CLAUDE.md. The HEALPix MOC core is sound: moc_and(aoi_moc, [shard_word]) is the right primitive for per-shard restriction of a mixed-order MOC (your Q1 — confirmed, since clip2order-equality would miss shards under a coarse interior word), uint64 dtype is held throughout, and empty-MOC short-circuits are handled in both healpix_shard_moc and healpix_mask_for_children. Findings below are diff-scoped; the inline comments carry the specifics. Nothing here blocks advancing to phase 2 — they're tightenings plus one latent version-parsing edge worth deciding on now.
Summary of the inline points:
_assert_mortie_versionaccepts pre-release0.8.2.devNstrings as ≥ 0.8.2, but a setuptools-scm0.8.2.devNis post-0.8.1, pre-0.8.2 — i.e. the cap-lift may be absent yet the assert passes.test_mask_matches_moc_to_order_membershipcomputesshards/assignonly to feed a tautologicalassert len(shards)— dead per §4.test_edge_shard_is_mixedis geometry-dependent (non-hermetic) and itsclip2order(parent_order, aoi_moc)enumeration silently can't refine coarse interior words to parent_order — fine for finding boundary shards, but worth a comment so the limitation is explicit.
No vague praise; the rest of phase 1 looks correct for its stated scope.
Generated by Claude Code
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Inline findings for phase 1 of #101 (HEALPix MOC mask). Details on each comment; nothing here blocks phase 2.
Generated by Claude Code
| parts: list[int] = [] | ||
| for token in str(raw).split(".")[:3]: | ||
| digits = "" | ||
| for ch in token: | ||
| if ch.isdigit(): | ||
| digits += ch | ||
| else: | ||
| break | ||
| parts.append(int(digits) if digits else 0) | ||
| while len(parts) < 3: | ||
| parts.append(0) |
There was a problem hiding this comment.
🤖 from Claude (review)
Latent version-gate hole: this parser strips non-digits per token, so a pre-release string like "0.8.2.dev3+g<sha>" parses to (0, 8, 2) and passes the >= (0, 8, 2) gate. But under setuptools-scm / hatch-vcs (which this repo uses for its own version, §7), 0.8.2.devN is the development version leading up to the 0.8.2 tag — i.e. it is built from a commit after 0.8.1 but before 0.8.2 is tagged, so the order-29 cap-lift (espg/mortie#59 + #70) may not yet be present. The assert would then pass against a build that still has the order-18 cap, which is exactly the silent-mis-sizing failure mode this function exists to prevent.
If mortie also uses scm-style local/dev versions, consider treating a .devN suffix on the target release as below it (drop dev builds of 0.8.2 itself), or pin the test to cover a "0.8.2.dev1" input so the intended behavior is asserted either way. At minimum the docstring should state that 0.8.2.devN is accepted, since that's a deliberate-looking but non-obvious choice.
Generated by Claude Code
| shards = np.unique(grid.shards_of(grid.assign(np.array([15.0]), np.array([15.0])))) | ||
| shard_key = int(grid.shards_of(grid.cells_of(flat[:1]))[0]) | ||
| children = grid.children(shard_key) | ||
| shard_moc = grid.aoi_shard_moc(aoi_moc, shard_key) | ||
| mask = grid.aoi_mask_for_children(shard_moc, children) | ||
|
|
||
| expected = np.isin(np.asarray(children, dtype=np.uint64), flat) | ||
| assert mask.dtype == bool | ||
| assert mask.shape == np.asarray(children).shape | ||
| np.testing.assert_array_equal(mask, expected) | ||
| assert mask.any() # the chosen shard is in-AOI | ||
| assert len(shards) # sanity: assign resolved |
There was a problem hiding this comment.
🤖 from Claude (review)
shards (line 39) is computed only to feed assert len(shards) (line 50, # sanity: assign resolved). len(shards) is >= 1 for any non-empty input, so this is a tautology that can never fail — it asserts nothing about the AOI mask. shard_key is derived independently from flat[:1], so the assign/shards_of round-trip on (15, 15) is dead per §4 (no dead code / no speculative scaffolding). Drop both lines; the load-bearing assertion is the assert_array_equal(mask, expected) above.
Separately, the real-but-weak assertion in this test is assert mask.any() (line ~48): the oracle expected = np.isin(children, flat) IS a sound independent cross-check (whole-AOI flat vs. the per-shard moc_and path, over children that all belong to the shard), but because shard_key is flat[:1]'s shard it could be a single-cell boundary shard, so .any() is the only liveness guard. Consider also asserting the per-shard moc_and actually restricted something (e.g. that flat has cells outside this shard, so the test would catch a moc_and that silently returned the whole AOI).
Generated by Claude Code
| # Find a shard touched by the AOI that is NOT fully contained: scan the | ||
| # MOC's coarsened parents and pick one whose mask is partial. | ||
| from mortie import clip2order | ||
|
|
||
| parents = np.unique(np.asarray(clip2order(grid.parent_order, aoi_moc))) | ||
| found_mixed = False | ||
| for p in parents.tolist(): | ||
| children = grid.children(int(p)) | ||
| shard_moc = grid.aoi_shard_moc(aoi_moc, int(p)) | ||
| mask = grid.aoi_mask_for_children(shard_moc, children) | ||
| if mask.any() and not mask.all(): | ||
| found_mixed = True | ||
| break | ||
| assert found_mixed, "expected at least one boundary shard with a mixed mask" |
There was a problem hiding this comment.
🤖 from Claude (review)
Two reliability notes on the edge-shard test:
-
Non-hermetic / geometry-dependent. The test only passes if the
(0,0)–(30,30)box atparent_order=5, child_order=9produces at least one parent with a partial mask. That's very likely true for a 30°×30° box, but the test is silently coupled to mortie's HEALPix tiling — a future mortie change to cell layout could make every touched parent fully in/out and flip this to a confusing failure. A box whose corner is engineered to sit mid-cell, or asserting on a specifically-constructed boundary shard, would make "mixed exists" structural rather than incidental. -
clip2order(grid.parent_order, aoi_moc)(line 86) can't enumerate interior shards.aoi_mocis mixed-order; coarse interior MOC words encode an order belowparent_order=5, and per theHEALPIX_REF_ORDERinvariantclip2order"cannot refine past its input order" — so those coarse words won't expand into the parent_order shards beneath them. The scannedparentsset therefore omits the deep interior (which is fine here, since you want boundary shards, which are fine-order ≥ parent_order). Worth a one-line comment that this enumeration is intentionally boundary-biased, so a later reader doesn't mistake it for a complete shard list and reuse it as one.
Generated by Claude Code
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Phase-2 adversarial review (rectilinear AOI-mask engine, issue #101). Scope: commit c6bda6a only — cell_centers / aoi_polygon / aoi_mask_for_children in rectilinear.py and TestRectilinearMask in test_aoi_mask.py.
Core math is correct. cell_centers (rectilinear.py:401-407) is the exact inverse of coverage/assign — x = xmin + (col+0.5)*res_x, y = ymax - (row+0.5)*res_y, with row = v//width, col = v%width — and matches the dimensional-coord centers at L457-458. res_x vs res_y are applied to the correct axes; no off-by-half, no axis swap, ymax-based y is right. The reprojection reuses the same to_crs(self._geobox.crs) path coverage uses, as the docstring claims. Methods are terse, no dead code, no silenced lint — CLAUDE.md-clean.
Inline findings (none block phase 3):
- Boundary inclusivity asymmetry (rectilinear.py:421-431) — strict
prep().containsexcludes on-boundary centers, while the HEALPix MOC path includes overlap/boundary cells. Cross-backend semantic divergence for the same mask; recommend an @espg decision / explicit doc. - Non-densified reprojection edge accuracy (rectilinear.py:410-420) —
to_crswith noresolution=makes the AOI ring straight chords, not geodesics, in polar/large-extent CRS; cell-center membership near edges can be wrong. Consistent withcoverage, but this mask is the strict-edge deliverable, so it matters most here. - Stale test comment (test_aoi_mask.py:141) — "20×20 m grid, centers (5,15,…)" is off 100×; grid is 1000 m res, centers 500/1500/….
- Weak oracle (test_aoi_mask.py:149-166) —
test_mask_matches_center_containsrecomputes with the samecontainspredicate (only prepared-vs-unprepared differs), so it validates wiring/ordering and the reprojection path, not the containment result independently. - All-true test misses transposition (test_aoi_mask.py:169-178) — symmetric total-coverage box + only 3 of 25 shards means a row/col swap in
cell_centerswouldn't be caught; assert the exact pattern in the partial case instead.
Items 1 and 2 are the substantive ones (semantics + accuracy); 3-5 are test/comment hardening.
Generated by Claude Code
|
|
||
| def aoi_mask_for_children(self, aoi_geom, children) -> np.ndarray: | ||
| """Boolean over ``children`` — ``True`` where the cell center is in the AOI. | ||
|
|
||
| ``aoi_geom`` is the reprojected polygon (:meth:`aoi_polygon`); ``children`` | ||
| the chunk's row-major flat cell ids (``children``). Cell centers are tested | ||
| with a prepared-geometry ``contains``. | ||
| """ | ||
| from zagg.grids.aoi import rectilinear_mask_for_centers | ||
|
|
||
| xs, ys = self.cell_centers(children) |
There was a problem hiding this comment.
🤖 from Claude (review)
Boundary-inclusivity asymmetry vs the HEALPix path — worth an @espg call. aoi_mask_for_children delegates to a shapely prep().contains test, which is strict: a cell whose center lands exactly on the AOI boundary is excluded. The HEALPix engine (healpix_mask_for_children, phase 1) instead tests MOC membership built from morton_coverage_moc, which is overlap/coverage-based and includes boundary and partially-covered cells. So the same conceptual "cell in AOI" mask has two different inclusivity rules depending on the grid backend.
For a center-in-polygon test, strict contains matches issue #101's "keep-whole-cell-if-center-in" wording, so the rect choice is defensible in isolation. But the cross-backend divergence is a real semantic inconsistency a client filtering with this mask would observe. Either (a) document the asymmetry explicitly, or (b) use covers/an intersects-on-boundary variant for parity with the HEALPix MOC path. Recommend surfacing this as a "Questions for review" item rather than silently shipping divergent semantics.
Generated by Claude Code
|
|
||
| def aoi_polygon(self, polygon_parts): | ||
| """Reproject the AOI ``[(lats, lons), ...]`` parts to the grid CRS. | ||
|
|
||
| Built once at the shard-map stage (same ``to_crs`` reprojection | ||
| :meth:`coverage` uses); the per-shard boolean (:meth:`aoi_mask_for_children`) | ||
| is precomputed against it and carried in the manifest. | ||
| """ | ||
| from zagg.grids.aoi import rectilinear_aoi_polygon | ||
|
|
||
| return rectilinear_aoi_polygon(polygon_parts, self._geobox.crs) |
There was a problem hiding this comment.
🤖 from Claude (review)
Reprojection accuracy at AOI edges. aoi_polygon → rectilinear_aoi_polygon reprojects the WGS84 ring with geom.to_crs(crs) and no densification (no resolution= arg). For a lat/lon box reprojected into a polar/large-extent grid CRS (e.g. EPSG:3413, as the test uses), the polygon edges become straight chords between only the corner vertices rather than following the true curved geodesics, so cell-center membership can be wrong for cells near the AOI edges by up to the chord-vs-arc deviation.
This is "consistent with how coverage reprojects" as the docstring claims (coverage at L311 also calls to_crs without densify), so it is not a regression — but note chunk_geometry (L383) does densify when going the other direction (to_crs("EPSG:4326", resolution=densify)), so the precedent within this file is mixed. Since the mask is the strict deliverable of #101 (the whole point is edge accuracy), the chord approximation is most consequential exactly here. Worth either densifying the AOI ring before to_crs or flagging the edge-slop bound under "Questions for review."
Generated by Claude Code
| def _grid(self): | ||
| from zagg.grids import RectilinearGrid | ||
|
|
||
| # A 20x20 m grid in a metres CRS; cell centers fall on (5,15,...)+10k offsets. |
There was a problem hiding this comment.
🤖 from Claude (review)
Stale/incorrect comment. This says "A 20x20 m grid ... cell centers fall on (5,15,...)+10k offsets", but the grid is resolution=1000.0 over bounds=[0,0,20000,20000] — i.e. a 20×20 cell grid at 1000 m resolution, with centers at 500, 1500, 2500, … (confirmed: cell_centers(children(0))[0] == [500, 1500, 2500, 3500]). The "20×20 m" and "(5,15,…)" figures are off by 100×. Fix the comment so it doesn't mislead a future reader about the oracle geometry.
Generated by Claude Code
| def test_mask_matches_center_contains(self): | ||
| from shapely.geometry import Point | ||
|
|
||
| grid = self._grid() | ||
| # AOI: a WGS84 box; reproject via the grid path and test each child center. | ||
| parts = _box(60.0, -40.0, 75.0, -20.0) | ||
| aoi_geom = grid.aoi_polygon(parts) | ||
| shard = 0 | ||
| children = grid.children(shard) | ||
| mask = grid.aoi_mask_for_children(aoi_geom, children) | ||
| xs, ys = grid.cell_centers(children) | ||
| expected = np.fromiter( | ||
| (aoi_geom.contains(Point(x, y)) for x, y in zip(xs, ys)), | ||
| dtype=bool, | ||
| count=len(children), | ||
| ) | ||
| assert mask.dtype == bool | ||
| assert mask.shape == np.asarray(children).shape |
There was a problem hiding this comment.
🤖 from Claude (review)
Weak oracle — the predicate is nearly self-referential. mask comes from rectilinear_mask_for_centers, which is prep(aoi_geom).contains(point) over cell_centers; expected here recomputes aoi_geom.contains(Point(x, y)) over the same cell_centers. The only difference between the two sides is prepared-vs-unprepared contains (which are defined to agree) — so this asserts mainly (a) prep==unprep and (b) the children→center ordering lines up, not that the containment result is correct against an independent reference. It does genuinely exercise the WGS84→grid aoi_polygon reprojection end to end (good — that's the real-path coverage), but the membership itself isn't independently checked here.
Suggest strengthening with a hand-computed expected set: pick an AOI whose grid-CRS extent you know, and assert specific (row, col) cells are in/out by construction (the test_partial_aoi_is_mixed/test_fully_inside boxes are closer to true oracles since they're explicit grid-CRS rectangles — but those skip the reprojection). One test that both reprojects and checks a known in/out cell would close the gap.
Generated by Claude Code
| def test_fully_inside_aoi_all_true(self): | ||
| # An AOI polygon directly in grid CRS covering the whole grid -> all True. | ||
| grid = self._grid() | ||
| # Build a generous WGS84 polygon by reprojecting the grid bounds back out: | ||
| # instead, reproject a grid-CRS box. Use cell-center membership against an | ||
| # explicit grid-CRS rectangle that contains every center. | ||
| from shapely.geometry import box as shapely_box | ||
|
|
||
| aoi_geom = shapely_box(-1000.0, -1000.0, 21000.0, 21000.0) | ||
| for shard in (0, 1, grid.n_col_blocks): |
There was a problem hiding this comment.
🤖 from Claude (review)
All-true test doesn't guard against a row/col swap. The grid is 5×5 blocks / 25 shards, but this only checks shards (0, 1, n_col_blocks==5). Since the AOI box [-1000,-1000,21000,21000] contains every center, mask.all() would pass even if cell_centers swapped rows↔cols or transposed within a chunk — the test can't distinguish a correct grid from a transposed one because the AOI is symmetric and total. The test_partial_aoi_is_mixed case (left-half box) is the one that constrains orientation, but it only asserts any() and not all(), not which cells. Consider asserting the exact True/False pattern in the partial case (e.g. cols 0–1 True, cols 2–3 False for the 4-wide tile) so an axis swap is actually caught.
Generated by Claude Code
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of the phase-3 diff only (fe1253a) against issue #101 and CLAUDE.md. The core wiring is sound and the byte-identity intent is mostly well-protected; one finding rises above nit level.
Blocking-ish — must be surfaced before "ready":
- Lambda path emits a present-but-empty mask.
process_shardacceptsaoi_payload, but onlyrunner._cell_work(local) passes it;deployment/aws/lambda_handler.py:251does not. Meanwhile_spec()gates theaoi_maskarray purely onget_aoi_mask(self.config), which runs on the Lambda setup path too. So a flag-on Lambda run allocates abool aoi_maskarray that is never written → all-False(entirely out-of-AOI), the inverse of the feature, with no error. The code degrades safely against crashing (theaoi_payload is not Noneguard holds; noAttributeError), so it's not broken-broken — but the PR body claims end-to-end wiring and says nothing about Lambda. Please flag this explicitly under "Questions for review" (local-backend only this phase) or gate the array so it isn't emitted where it can't be filled. Inline on worker.py.
Worth addressing in this phase:
_compute_aoi_maskdispatches onhasattr(grid, "aoi_moc")with an unconditional rectelseand no "unsupported grid" guard — flag-on against a grid lacking AOI methods eitherAttributeErrors cryptically or (via theconfig is Nonegate inbuild) is silently ignored. Also inconsistent with this module's existingparent_order/child_orderHEALPix predicate. Inline on shardmap.py.- Flag-off byte-identity test exercises the template+write seam but not the runner kwarg-threading it's meant to protect (
_cell_work's conditionalaoi_payload). Inline on test_aoi_mask.py.
Confirmed correct (no change needed):
- K>1 /
chunk_inner: both engines expand by membership overchunk_children(a subset ofchildren(shard_key)), order-independent; mask length aligns with the K>1 carrier (children=chunk_children). HEALPix MOCmoc_to_orderre-expansion and rect cell-id membership both hold at K>1. - Manifest round-trip omits
aoi_maskwhen off;from_json/to_jsoncarry it only when present;_aoi_payload_mapreturns{}on a pre-feature manifest. Byte-identity of the manifest holds. packaging.versionrewrite in aoi.py correctly rejects0.8.2.devN; tests cover it.
Nits: get_aoi_mask re-coerces with bool(...) while validate_config rejects non-bools — accessor/validator disagree on strictness if validation is skipped. hasattr(grid, "aoi_mask_from_payload") guard in the worker silently drops the column on a grid/manifest mismatch (same all-False failure shape). Both inline.
No CLAUDE.md violations spotted in the diff: messages terse, no silenced lint introduced, no dead code; module sizes not pushed past the ~1000-line limit by this change. Did not approve / request changes per review scope.
Generated by Claude Code
| handoff: str = "pandas", | ||
| ragged_out: dict | None = None, | ||
| chunk_results: list | None = None, | ||
| aoi_payload=None, |
There was a problem hiding this comment.
🤖 from Claude (review)
Lambda path produces a silently-wrong mask, not just an absent one. process_shard gains aoi_payload here, but the only caller that passes it is runner._cell_work (local backend). The AWS Lambda handler (deployment/aws/lambda_handler.py:251) calls process_shard(...) with no aoi_payload, so on Lambda every worker defaults to None and writes no aoi_mask column.
That alone would be "no mask on Lambda", but it is worse: emit_template on the setup path does create the aoi_mask zarr array, because the _spec() gate (healpix.py:435 / rectilinear.py:524) is purely config-driven (get_aoi_mask(self.config)) and runs identically on Lambda. So a flag-on Lambda run emits a fully-allocated bool aoi_mask array that is never written — every cell reads fill_value=False, i.e. an all-out-of-AOI mask. A client filtering on it gets zero in-AOI cells, the opposite of the feature's intent, with no error.
The runner/worker code does degrade safely against crashing (the aoi_payload is not None guard means no AttributeError), which is correct. But the schema gate and the write gate are decoupled across backends, so "flag on" yields a present-but-empty array on Lambda. This needs to be either (a) flagged explicitly in the PR body as a known limitation ("aoi_mask is local-backend only this phase; Lambda support is a follow-up"), or (b) the array gated so it isn't emitted on a path that can't fill it. The PR body currently claims the feature is wired and says nothing about Lambda — please surface this under "Questions for review" so @espg can decide.
Generated by Claude Code
| Computed once here (the shard-map stage) so the worker/Lambda path needs no | ||
| region plumbing — the mask depends only on (grid, AOI), never on observations. | ||
| """ | ||
| is_healpix = hasattr(grid, "aoi_moc") |
There was a problem hiding this comment.
🤖 from Claude (review)
Dispatch via hasattr(grid, "aoi_moc") has no fallback for a third grid type, and is inconsistent with how this module already detects HEALPix. Two points:
-
The
elsebranch assumes the grid is rectilinear and unconditionally callsgrid.aoi_polygon(parts)/grid.children/grid.aoi_mask_for_children. If a future grid (or anyOutputGridimplementer) lacks bothaoi_mocand the rect AOI methods, but hasconfigwith the flag on,build()willAttributeErrordeep in_compute_aoi_maskrather than failing with a clear message. Since the flag is opt-in, an explicit guard — e.g. raise a clearValueError("output.aoi_mask is not supported for grid type <...>")when neither capability is present — would fail loudly at the build stage instead of with a cryptic attribute error. -
The rest of this module decides HEALPix-vs-other via
hasattr(grid, "parent_order") and hasattr(grid, "child_order")(lines 124, 158). Here you switch tohasattr(grid, "aoi_moc"). Two different duck-type predicates for the same fork invites drift. Prefer reusing the existing predicate (or a shared helper) so the AOI path and the backend-selection path agree on what "HEALPix" means.
Generated by Claude Code
| # when ``output.aoi_mask`` is on — otherwise the manifest is unchanged. | ||
| from zagg.config import get_aoi_mask | ||
|
|
||
| grid_config = getattr(grid, "config", None) |
There was a problem hiding this comment.
🤖 from Claude (review)
grid_config = getattr(grid, "config", None) then gating on grid_config is not None and get_aoi_mask(grid_config) is a quiet failure mode: both HealpixGrid and RectilinearGrid always set self.config (defaulting to default_config("atl06")), so config being None here can only mean a grid type that doesn't carry config at all — in which case the flag is silently ignored even if the user set it. That's defensible, but combined with the absence of any "flag on, grid can't do it" error (see the _compute_aoi_mask comment), the net behavior is: enable the flag on an unsupported grid and you get no mask and no warning. Worth a one-line note or a log at minimum so an enabled-but-ignored flag isn't completely silent.
Generated by Claude Code
| assert actual.dtype == np.bool_ | ||
| np.testing.assert_array_equal(actual, mask) | ||
|
|
||
| def test_flag_off_store_byte_identical(self, mock_dataframe_factory): |
There was a problem hiding this comment.
🤖 from Claude (review)
The byte-identity test compares write_dataframe_to_zarr(df, ...) for default_config vs flag-off config — it exercises the template + write seam, which is good, but it bypasses the part of phase 3 that is actually new on the flag-off path: the runner._cell_work conditional kwarg threading (if aoi_by_shard: extra["aoi_payload"] = ...). The phases checklist explicitly lists "flag-off byte-identity" under runner wiring, and the claim being protected is that _process_and_write/process_shard are called with the pre-feature signature when the flag is off. Nothing here asserts that. A small test that runs _run_local (or at least _cell_work) against a manifest with no aoi_mask key and asserts no aoi_mask column / no aoi_payload kwarg reaches the worker would close the actual regression surface. As written, a future change that always-threaded aoi_payload=None would not be caught by this suite even though it would defeat the "identical to pre-feature signature" intent stated in runner.py.
Generated by Claude Code
| ------- | ||
| bool | ||
| """ | ||
| return bool(config.output.get("aoi_mask", False)) |
There was a problem hiding this comment.
🤖 from Claude (review)
Minor consistency note: get_aoi_mask returns bool(config.output.get("aoi_mask", False)), so a truthy non-bool value (e.g. "no", 0.0, []) is coerced rather than rejected. validate_config (config.py:212-214) does reject non-bools — but only when validate_config is actually called. If get_aoi_mask is ever reached on an unvalidated config (e.g. a hand-built PipelineConfig in a test or an alternate entry path that skips validation), "no" → True. Not a bug given the validation gate, but the accessor and the validator disagree on strictness; consider having the accessor trust the validated invariant (config.output.get("aoi_mask", False) already being a bool) rather than re-coercing, so the two can't silently diverge. Low priority.
Generated by Claude Code
| # over THIS chunk's cells (order-aligned with the carrier). None when the | ||
| # flag is off, so the carrier is byte-for-byte unchanged. | ||
| chunk_aoi_mask = ( | ||
| grid.aoi_mask_from_payload(aoi_payload, chunk_children) |
There was a problem hiding this comment.
🤖 from Claude (review)
Confirming the K>1 interaction is correct (not a defect): both engines expand by membership over chunk_children, which is a subset of children(shard_key). HEALPix re-runs moc_to_order(shard_moc, child_order) + np.isin(chunk_children, flat) (aoi.py:115), and rect does np.isin(chunk_children, true_ids), so a sub-tile maps correctly without depending on positional order. The mask length is len(chunk_children), which aligns with the carrier rows since _build_output is given children=chunk_children at K>1 — so the column length matches the coords. Good.
One robustness nit while here: the hasattr(grid, "aoi_mask_from_payload") guard means that if aoi_payload is non-None (manifest had a mask) but the grid somehow lacks the method, the column is silently dropped for that chunk while the schema array still exists → all-False region, same failure shape as the Lambda gap. Given both built-in grids define the method, this can only fire on a grid/manifest mismatch, but a manifest carrying aoi_mask against a grid that can't expand it is arguably worth a hard error rather than a silent skip.
Generated by Claude Code
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of phase 4 (3151019) + the three fold commits (f684fe0, 9b4108b, b412313) against issue #101 and CLAUDE.md.
Verified sound (ran the code locally against mortie 0.8.2):
- Notebook
aoi_mask.ipynbexecutes clean end-to-end and thenp.array_equal(mask, expected)assertion holds; cells use the real grid API (aoi_moc/aoi_shard_moc/aoi_mask_for_children);nbformat.validatepasses. Badge URL is well-formed forenglacial/zagg. - Docs storage-layout and read snippet are accurate: HEALPix
group_path == str(child_order)(→<child_order>/aoi_mask, matching thexr.open_zarr(".../13")example) and rectgroup_path == "rectilinear". The mortie>= 0.8.2claim and "asserts version at use" match_assert_mortie_version. Lambda limitation and WKB-deferral notes are correct. - Lambda refusal (
runner._run_lambda) readsget_aoi_mask(config)and raisesNotImplementedErrorat the very top, before any template write / side effect — correct call vs silently emitting an all-False mask. - Worker hard-error (
worker.py) is correctly defensive: a non-Noneaoi_payloadagainst a grid lackingaoi_mask_from_payloadis a manifest/grid mismatch; an empty-list payload (legit out-of-AOI shard) still has the method, so it can't wrong-fire. - Rect oracle test is genuinely independent (pyproj back-reprojection, not shapely-on-grid). All touched tests pass;
ruff checkclean on the changed source. No@-mentions, terse code.
Three diff-scoped findings inline:
shardmap.py:201— the dispatch-predicate reconciliation a prior self-review asked for wasn't done (hasattr(aoi_moc)vs the module'sparent_order/child_orderpredicate used 3x). Not a live bug; latent inconsistency.docs/aoi_mask.md:96/ notebook badge — "launchable on Binder" with no Binder env file in the repo;mortie>=0.8.2+zaggwon't resolve on a default Binder build (CLAUDE.md §4).tests/test_aoi_mask.py:212—agree.mean() > 0.9is far looser than the actual1.0agreement; tighten toagree.all()so it guards real regressions.
No blocking issues; all three are improvements, not correctness stoppers.
Generated by Claude Code
| neither with the flag on is a misconfiguration, raised here rather than left to | ||
| a cryptic ``AttributeError`` downstream. | ||
| """ | ||
| if hasattr(grid, "aoi_moc"): |
There was a problem hiding this comment.
🤖 from Claude (review)
The earlier self-review asked to reconcile this dispatch with the module's own HEALPix predicate; that wasn't done. _compute_aoi_mask keys HEALPix-vs-rect on hasattr(grid, "aoi_moc") / hasattr(grid, "aoi_polygon"), but everywhere else in this same module the family test is is_healpix = hasattr(grid, "parent_order") and hasattr(grid, "child_order") (lines 124, 158, 294). Two divergent predicates for the same fork is exactly the inconsistency that was flagged. They happen to agree for HealpixGrid/RectilinearGrid today (rect has aoi_polygon+no parent_order; HEALPix has both), so this is not a live bug — but a future grid that exposes parent_order/child_order without aoi_moc (or vice-versa) would be classified one way here and the other way 30 lines up. Recommend dispatching on the same is_healpix predicate the rest of the module uses, then branching to aoi_moc vs aoi_polygon inside, keeping the new ValueError for the genuinely-unsupported case.
Generated by Claude Code
|
|
||
| See the runnable, data-free example in | ||
| [`notebooks/aoi_mask.ipynb`](https://github.com/englacial/zagg/blob/main/notebooks/aoi_mask.ipynb) | ||
| (launchable on Binder), which builds a small HEALPix grid + AOI box and shows the |
There was a problem hiding this comment.
🤖 from Claude (review)
"launchable on Binder" (and the matching Binder badge in notebooks/aoi_mask.ipynb) looks unbacked: there's no Binder environment file in the repo (environment.yml / requirements.txt / binder/ / postBuild), and none of the other four notebooks ships one either. On a default Binder build the notebook's hard deps wouldn't resolve — it imports zagg and needs mortie >= 0.8.2 (the order-29 MOC cap that _assert_mortie_version enforces; older mortie returns an empty/wrong mask), and zagg itself isn't a plain pip install on Binder. As written the badge will land users on a kernel that errors at the first import. Either add a Binder env pinning zagg + mortie>=0.8.2 (CLAUDE.md §4 requires notebooks be Binder-runnable), or drop the "launchable on Binder" claim and the badge until the env exists. (Note the notebook itself is HEALPix-only, so it does not need the non-PyPI spherely fork — only the rect path would.)
Generated by Claude Code
| oracle = (clon >= lon0) & (clon <= lon1) & (clat >= lat0) & (clat <= lat1) | ||
| # Straight-chord vs geodesic edge: allow boundary-cell slop, exact interior. | ||
| agree = mask == oracle | ||
| assert agree.mean() > 0.9 |
There was a problem hiding this comment.
🤖 from Claude (review)
The independence here is good — reprojecting cell centers back to WGS84 with pyproj and testing the axis-aligned box is a genuinely separate oracle from the shapely contains path, and mask.sum()/oracle.sum() match. But agree.mean() > 0.9 is far looser than reality: on this 20x20 (400-cell) UTM tile the two agree exactly (agree.mean() == 1.0, 0 disagreements — verified locally), because straight-chord vs geodesic edges is sub-cell at 1 km / mid-latitude UTM. A > 0.9 gate tolerates up to 40 cells flipping, so a real regression in the rect containment (e.g. an off-by-one tile shift or a boundary-handling change touching dozens of cells) would still pass. Tighten to the observed behavior — assert agree.all() (or at most one or two boundary cells, (~agree).sum() <= 2) — so the oracle actually guards the result it's meant to.
Generated by Claude Code
|
🤖 from Claude Folded the three phase-4 self-review findings in one commit (
AOI suite green (28 passed); Generated by Claude Code |
|
needs conflicts resolved ahead of merge |
|
🤖 from Claude Conflicts resolved. Merged current Four files conflicted;
Green locally: One reconciliation decision worth your eye: the Non-blocking flag: pre-existing This PR's four Questions for review in the description (the Generated by Claude Code |
Seems reasonable-- confirmed.
Document explicitly and keep as is.
...does shapely solve this for us? Or rather, could it solve this for us? To be clear, is this something we need to think about for both healpix and rect, or is this another rect specific issue?
No, we should implement that in lambda_handler.py now, as part of this PR. You are approved to edit that file and expand the PR scope to implement above
See above; we're doing it here, not as a follow up
I expect that espg/mortie#89 will land before this PR, so we'll roll support for WKB/WTK here too. |
# Conflicts: # tests/test_runner.py
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of the Phase-5 diff (6f49fec..2f9e1fd) — Lambda backend threading of the per-shard strict-AOI payload (issue #101). No blocking issues; Phase 5 looks correct and well-tested.
Correctness (end-to-end threading). The payload now flows cleanly: _run_lambda builds aoi_by_shard via _aoi_payload_map, _cell_work sets extra["aoi_payload"] only when the manifest carries a mask, _invoke_lambda_cell adds the "aoi_payload" event key only when non-None, and lambda_handler._handle_process reads event.get("aoi_payload") and forwards it to process_shard. This mirrors the local runner's threading.
Flag-off byte-identity — verified. process_shard defaults aoi_payload=None (worker.py:60) and the worker body is guarded by if aoi_payload is not None: (worker.py:383). So the handler passing aoi_payload=None explicitly is behaviorally identical to omitting the kwarg — no aoi_mask column allocated, no output drift. The event payload also stays key-for-key identical when off (no aoi_payload key), matching the established profile (#100) pattern. The handler-passes-None vs runner-omits asymmetry is purely stylistic.
Dead-import cleanup — verified. With the NotImplementedError refusal removed, get_aoi_mask has zero remaining references in runner.py, so dropping the import is correct and leaves no stale comment.
Tests — strong. Both directions are covered at both layers: handler-level (test_aoi_payload_threaded_to_process_shard asserts the list is forwarded; test_no_aoi_payload_passes_none asserts the absent key yields aoi_payload is None), event-construction level (test_aoi_payload_adds_event_key / test_default_event_has_no_aoi_payload_key), and runner level (test_lambda_threads_aoi_payload asserts exact per-shard payloads seen[10]==[1,2], seen[13]==[4,5] and that the no-manifest path omits the kwarg via the "OMITTED" sentinel). The old test_lambda_refuses_aoi_mask was correctly replaced rather than weakened. Assertions are exact, not loose. The remaining diff churn in test_runner.py is ruff-format reflow only.
No undiscussed dependencies. Diff adds none.
Nits (non-blocking, see inline):
_run_lambda/_run_localuseaoi_by_shard.get(int(shard_key))which silently yields None (→ key omitted) for a shard missing from the map even when the flag is on. Can't fire with today's_aoi_payload_map(built fromzip(shard_keys, aoi_mask)), but the parallel-length invariant is load-bearing — fine to leave for @espg.
Out of scope: runner.py is now 1180 lines, over the ~1000-line soft limit in CLAUDE.md §4. It was already 1172 before this PR (Phase 5 adds only ~8 net lines), so this is pre-existing, not introduced here — flagging for awareness rather than asking this PR to fix it.
Generated by Claude Code
| config=config, | ||
| chunk_results=chunk_results, | ||
| profile=profile, | ||
| aoi_payload=aoi_payload, |
There was a problem hiding this comment.
🤖 from Claude (review)
Byte-identity confirmed for the flag-off path. The handler always passes aoi_payload=event.get("aoi_payload") (None when the key is absent), and process_shard's signature defaults aoi_payload=None (src/zagg/processing/worker.py:60) with the body guarded by if aoi_payload is not None: (worker.py:383). So the explicit-None call here is behaviorally identical to the runner's omit-the-kwarg style — no mask column is allocated, no output drift. The asymmetry (handler passes None explicitly, local runner omits) is purely stylistic and harmless; both collapse to the same default. No change needed.
Generated by Claude Code
| # pre-feature path (issue #101). Mirrors the local runner's _cell_work. | ||
| extra = {} | ||
| if aoi_by_shard: | ||
| extra["aoi_payload"] = aoi_by_shard.get(int(shard_key)) |
There was a problem hiding this comment.
🤖 from Claude (review)
Nit / latent edge (not blocking): when the flag is on but a shard is missing from aoi_by_shard, .get(int(shard_key)) returns None, so extra["aoi_payload"] = None is set. _invoke_lambda_cell then guards if aoi_payload is not None: and omits the event key — so that one cell silently gets no mask payload while the flag is on, rather than an empty in-AOI payload. With the current _aoi_payload_map (built from zip(shard_keys, aoi_mask)) every dispatched shard is present, so this can't fire today; but it mirrors the same .get() shape in _run_local (runner.py:495), and if the manifest's aoi_mask ever became shorter than shard_keys the mismatch would pass silently. Consider documenting the parallel-length invariant (it is noted in _aoi_payload_map's docstring) or asserting it. Fine to leave for @espg.
Generated by Claude Code
espg
left a comment
espg
left a comment
There was a problem hiding this comment.
🤖 from Claude (review)
Fresh-context adversarial review of the Phase 6 (docs) + Phase 7 (densify) diff only (2f9e1fd..04d1f35). No blocking issues — the change is correct, well-tested, well-documented, and dependency-free.
Verified against the installed stack (not from memory):
odc.geo.geom.Geometry.to_crsacceptsresolution="auto"(signatureUnion[float, Literal['auto'], None]);"auto"→sqrt(area)*4/100≈ 100 pts/side in the source CRS. So the densify is real, valid, and not a typo.- Flag-off byte-identity preserved: the densified
rectilinear_aoi_polygonis reachable only via theoutput.aoi_mask-on path (shardmap.pyhasattrguard). No off-path bytes change. - The new test genuinely proves geodesic-following: 103 vs 5 vertices and ~2% area delta in EPSG:3413 (threshold is
1e-4, ~200x margin — not flaky, not tautological). Suite passes locally (7 passed). - Boundary-inclusivity doc section matches the code exactly (HEALPix MOC inclusive; rect
prepared.containsstrict). No contradiction with the densify paragraph.
Verdict on the "auto" design choice (the question flagged for review): resolution="auto" is a safe, clean default — not something that needs to be escalated to a config knob in this PR. It is parameter-free, adapts to AOI extent, has no degenerate-output risk for a valid AOI, and reuses the existing to_crs densify mechanism. If anyone later wants reproducible cross-tool vertex counts or to trade precision for output size, exposing the resolution becomes a clean follow-up — but it is not a gate on landing this. Worth a one-line "Questions for review" note so @espg can opt to make it configurable, nothing more.
Nits (none blocking):
- Docstring + docs say densify uses "the same primitive
shard_footprintuses" — same method, butshard_footprintpasses a numericmax(edge)/32, not"auto". Slight overstatement; reword to "same densify-before-reproject mechanism (here viaresolution=\"auto\")". - The new test's chord baseline uses
parts[0]only, so themultipolygonbranch ofrectilinear_aoi_polygonis untested;not densified.equals(chord)is redundant with the area-delta assert.
Inline comments on aoi.py:161, the test, and docs/aoi_mask.md have the specifics.
Generated by Claude Code
| geom = polygon(rings[0], crs="EPSG:4326") | ||
| else: | ||
| geom = multipolygon([[r] for r in rings], crs="EPSG:4326") | ||
| return geom.to_crs(crs, resolution="auto").geom |
There was a problem hiding this comment.
🤖 from Claude (review)
Densify is correct and well-scoped. Verified against the installed odc.geo: Geometry.to_crs(crs, resolution=...) does accept the literal "auto" (signature resolution: Union[float, Literal['auto'], None]), and "auto" resolves to _auto_resolution(g) = sqrt(area)*4/100 (~100 points per side) in the source CRS — here EPSG:4326, so the segmentation step is in degrees. So the change is real and safe:
- Flag-off byte-identity holds:
rectilinear_aoi_polygonis reached only viaRectilinearGrid.aoi_polygon, which is only called fromcatalog/shardmap.pyunder thehasattr(grid, "aoi_polygon")mask path — i.e. only whenoutput.aoi_maskis on. Nothing in the off path changed. - No degenerate/empty risk for a valid AOI; in a non-curving (or small-extent) CRS the inserted points are ~collinear, so the mask result is unchanged — an acceptable no-op.
One doc-accuracy nit (not blocking): the docstring says this is "the same primitive RectilinearGrid.shard_footprint uses." Same method (to_crs(resolution=...)), but shard_footprint passes a numeric resolution (max(edge)/32), not "auto". Consider "the same densify-before-reproject mechanism shard_footprint uses (here via resolution=\"auto\")" so the phrasing doesn't imply identical arguments. Same nit applies to docs/aoi_mask.md.
Generated by Claude Code
|
|
||
| def test_aoi_ring_is_densified_before_reproject(self): | ||
| # issue #101: rectilinear_aoi_polygon densifies the WGS84 ring before | ||
| # to_crs (resolution="auto") so AOI edges follow the geodesic instead of | ||
| # collapsing to straight chords in a polar CRS. A 4-corner box reprojected | ||
| # WITHOUT densification keeps ~5 ring vertices; the densified path inserts | ||
| # many. In a curving CRS (EPSG:3413) the two polygons genuinely differ. | ||
| from odc.geo.geom import polygon as odc_polygon | ||
|
|
||
| from zagg.grids.aoi import rectilinear_aoi_polygon | ||
|
|
||
| grid = self._grid() | ||
| parts = _box(60.0, -40.0, 75.0, -20.0) | ||
| densified = rectilinear_aoi_polygon(parts, grid.crs) | ||
|
|
||
| # Same parts, reprojected as straight chords (no densification) -- the old | ||
| # behavior, built here directly as the baseline. | ||
| lats, lons = parts[0] | ||
| ring = [(float(x), float(y)) for x, y in zip(lons, lats)] | ||
| chord = odc_polygon(ring, crs="EPSG:4326").to_crs(grid.crs).geom | ||
|
|
||
| n_dense = len(densified.exterior.coords) | ||
| n_chord = len(chord.exterior.coords) | ||
| assert n_dense > n_chord # densification inserted edge vertices | ||
| assert n_chord <= 5 # the raw box is just its corners | ||
| # The geodesic-following polygon is not the chord polygon: in a polar CRS | ||
| # the curved edges enclose a measurably different area. | ||
| assert not densified.equals(chord) |
There was a problem hiding this comment.
🤖 from Claude (review)
Test is genuine, not tautological. Reproduced its assertions against the installed stack with the test's own EPSG:3413 grid and _box(60,-40,75,-20):
n_dense = 103vsn_chord = 5— densification really inserts edge vertices.- relative area delta = ~0.0206 (>> the
1e-4threshold), so the geodesic-following polygon genuinely differs from the straight-chord one in a polar CRS. The vertex-count check alone could pass on a trivially-resampled straight edge, but the area-delta check is what proves the edges actually curve — together they're a meaningful proof.
The 1e-4 threshold is conservative (true value is ~200x larger), so it won't be flaky. Suite passes locally (7 passed). Two minor nits, neither blocking:
- The chord baseline is built from
parts[0]only. Fine here (single-part box), but it means the test doesn't exercise themultipolygonbranch ofrectilinear_aoi_polygon. Optional: a second multipart assertion would close that gap. not densified.equals(chord)is redundant once the area-delta assert is in place, but harmless.
Generated by Claude Code
| ## Boundary inclusivity differs by grid family | ||
|
|
||
| The two engines settle a cell that *straddles* the AOI edge differently, and the | ||
| difference is **intentional and kept as-is** — each rule is the natural one for | ||
| its engine. A client filtering on `aoi_mask` should expect slightly different | ||
| edge semantics across grid families: | ||
|
|
||
| - **HEALPix is overlap/coverage-based (inclusive at the boundary).** | ||
| `morton_coverage_moc` builds a MOC that *covers* the AOI, so a leaf cell whose | ||
| area overlaps the AOI — even partially, including one whose center lies just | ||
| outside — is in the MOC and marked `True`. The MOC is a superset cover, so the | ||
| HEALPix mask leans **inclusive**: it never drops a cell that touches the AOI. | ||
|
|
||
| - **Rectilinear is a strict center test (exclusive at the boundary).** Each cell | ||
| is marked `True` only if its **center** falls inside the reprojected AOI polygon | ||
| (`prepared.contains(center_point)`). `contains` is strict — a center lying | ||
| *exactly on* the boundary returns `False` — so a cell straddling the edge whose | ||
| center is outside (or on) the polygon is **excluded**. This matches the | ||
| "keep-whole-cell-if-its-center-is-in" rule. | ||
|
|
||
| So for the same AOI edge, the HEALPix mask tends to **include** an edge-overlapping | ||
| cell while the rectilinear mask **excludes** one whose center is outside. Both are | ||
| defensible in isolation (the HEALPix MOC is the native, decode-free primitive; the | ||
| rect center test matches how rect cells are addressed), and the asymmetry is at | ||
| most one cell-width at the boundary. Keep this in mind when comparing strict-AOI | ||
| counts across a HEALPix and a rectilinear run of the same region. |
There was a problem hiding this comment.
🤖 from Claude (review)
Boundary-inclusivity section is accurate and consistent with the code:
- HEALPix "inclusive at the boundary" matches
healpix_aoi_moc→morton_coverage_moc, which builds a superset MOC cover, expanded viamoc_to_order+np.isinmembership (aoi.py:115-131) — a leaf overlapping the AOI is kept. - Rectilinear "strict center test (exclusive)" matches
rectilinear_mask_for_centers, which isprepared.contains(point)(aoi.py:177) —containsexcludes a point exactly on the boundary. Correct.
No contradiction with the new densify paragraph: densify changes which polygon the center test runs against (geodesic vs chord edge), it does not change the inclusive/exclusive rule. The two sections are consistent.
The "Both backends" / WKB-deferred Notes updates (Phase 6) also read correctly and match the Phase-5 Lambda threading described. No issues here.
Generated by Claude Code
|
🤖 from Claude Answering the rect-densification question (3) — "does/could shapely solve this for us? Is this rect-specific or also HEALPix?" Short answer: yes, it's solvable cleanly and dependency-free; it's rect-specific; I've implemented it in phase 7 (commit 04d1f35) using the densification primitive we already use elsewhere. Is it rect-specific or also HEALPix? Rect-specific. The chord-vs-arc error comes entirely from reprojecting a sparse polygon ring through Does shapely solve it? Either tool can: What I did (phase 7): One point left for your call: I used Also folded your other answers: (1) Generated by Claude Code |
|
🤖 from Claude Status — folded your answers (comment 4834768382), landed Lambda + boundary-doc + densification this run. Marking Conflict resolved. Merged current Phase 5 — AWS Lambda AOI threading (you authorized editing Phase 6 — boundary-inclusivity asymmetry documented (your directive 2). New explicit section in Phase 7 — densification answer + implementation (your question 3). Full answer is in the comment just above. Gist: it's rect-specific (HEALPix tessellates the native WKB/WKT (Q4) — deferred, blocked on espg/mortie#89. You expected espg/mortie#89 to land before this PR. As of this run it's still open / not merged (itself Self-reviews: a fresh-context adversarial review ran after each phase push (above). No blocking findings; I folded the two diff-scoped doc/test nits from the phase-6/7 review into Local gates: Open for you: (3) keep Generated by Claude Code |
2 participants
Closes #101
What this does
Adds an optional, default-OFF per-cell boolean
aoi_maskaligned to the output cell grid —Truewhere the cell falls inside the AOI polygon. This is "package, don't clip": no observation is dropped, and a flag-OFF run is byte-identical to today. The mask is metadata a client uses to filter to a strict-AOI view, recovering the order-independent in-AOI subset (the #100 overhang finding). Refs #100.The mask is computed at the shard-map build stage via native morton (HEALPix) / shapely cell-center test (rectilinear) — not via lat/lon-center decode — and carried per shard in the shard-map JSON. Each worker (local and AWS Lambda) expands its shard's payload to a cell-order boolean over the
children()it already enumerates and writes it as one more coordinate-shaped column, only when the flag is on.Follows the locked decisions in the issue plan (comment 4804178324) and @espg's confirmations (comments 4814504937 and 4834768382): native-morton MOC at the shard-map stage, HEALPix + rectilinear as one feature/two engines,
mortie>=0.8.2, manifest carrier = the shard-map JSON, rect carrier = precomputed packed bool, Lambda threading done in this PR.Approach
morton_coverage_moc(lats, lons, order=child_order)builds a compact AOI MOC; per shard it is intersected with the shard's coarse word viamoc_and(a coarse interior MOC word spans many shards, soclip2order-equality would miss them —moc_andkeeps coarse coverage and trims boundary detail). The worker expands the per-shard sub-MOC withmoc_to_order(..., child_order)and testschildren()membership — already in cell/storage order, no decode.containsafter reprojecting the AOI polygon to the grid CRS. The WGS84 ring is densified beforeto_crs(odc.geoto_crsresolution densification,resolution="auto"— the same mechanismshard_footprintalready uses) so AOI edges follow the geodesic, not straight chords, in polar/large-extent CRS (answers Q3).shapely/odc-geoare already core deps.output.aoi_mask(default false), validated invalidate_config, read viaget_aoi_mask(config). Thebool aoi_maskarray is declared in each grid's_spec()only when on (off-runs gain no array).ShardMapgains an optionalaoi_masklist parallel toshard_keys(HEALPix = compact MOC ints; rect = in-AOI cell ids), carried into_json/from_jsononly when present.process_shardgains anaoi_payloadkwarg; the local runner threads each shard's payload (omitting the kwarg entirely when off) and the Lambda runner forwards it through the per-cell event (aoi_payloadkey, present only when on) →deployment/aws/lambda_handler.py→process_shard. Both backends keep flag-off byte-identity.>=0.8.1→>=0.8.2(ships the order-29 MOC cap, moc xor + domain-bounded not espg/mortie#59 + Temporal aggregation infrastructure (Refs #12) #70). The mask code asserts the resolved mortie is ≥0.8.2 at use (viapackaging.version). Thelambdaextra does not pin mortie (it rides core deps), anddeployment/aws/build_layer.shinstallsmortieunpinned — so no infra change is needed.Phases
moc_andrestriction + per-children expansion overchildren()) + tests.contains, reprojected to grid CRS) + tests.output.aoi_mask+ validation +get_aoi_mask) + schema (bool aoi_maskin_spec()when on) + manifest carrier (to_json/from_json) + worker expand/write + local runner wiring + tests, incl. flag-off byte-identity and runner kwarg-threading.docs/aoi_mask.md, added to nav) + a self-contained, data-free Binder-launchable notebook (notebooks/aoi_mask.ipynb).deployment/aws/lambda_handler.py→process_shard), removing the prior flag-onNotImplementedErrorrefusal. Tests cover the flag-on Lambda threading + flag-off event byte-identity (authorized by @espg, comment 4834768382).docs/aoi_mask.md(HEALPix MOC overlap/coverage = inclusive at the edge; rect strict centercontains= exclusive), behavior kept as-is per @espg's directive (2).to_crsvia the existing odc.geo densification (resolution="auto"); confirmed rect-specific (HEALPix tessellates on the sphere, never reprojects a polygon). Test + doc.Known limitations / deferred
(lats, lons)rings) — @espg expects WKB/WKT geometry I/O: ingest + dissolved emit (issue #71) espg/mortie#89 to land before this PR so it could be rolled in here (comment 4834768382). As of this run WKB/WKT geometry I/O: ingest + dissolved emit (issue #71) espg/mortie#89 is still open / not merged (it is itselfwaitingon a Python-vs-Rust decision and has an unfinished polar-cap dissolve sub-piece), and the pinnedmortie>=0.8.2exposes no WKB/WKT entry point — itsfrom_wkbreference is only a test helper, not public API. So WKB/WKT support is deferred, blocked on WKB/WKT geometry I/O: ingest + dissolved emit (issue #71) espg/mortie#89; I have not faked it and have not bumped the mortie pin to an unreleased version. The AOI continues to be supplied as(lats, lons)rings. See "Questions for review" (3).How tested
uv run pytest -q(935 passed, 10 skipped locally),ruff check --select=E,F,W,I --ignore=E501 src testsclean,ruff format --checkclean on touched files. Coverage intests/test_aoi_mask.py,tests/test_shardmap.py(TestBuildAOIMask),tests/test_runner.py,tests/test_lambda_handler.py:contains; independent pyproj-reprojected-back oracle; version gate)._run_lambdathreads each shard'saoi_payloadinto_invoke_lambda_cell; the event gains anaoi_payloadkey only when on (flag-off event byte-identical);lambda_handler._handle_processforwardsevent.get("aoi_payload")intoprocess_shard(flag-on threading + flag-offNoneno-op).test_aoi_ring_is_densified_before_reprojectpins the vertex-count growth and area delta vs the chord baseline in EPSG:3413.Self-review findings folded across phase commits (latest: reworded the "same primitive" densify claim to "same mechanism,
resolution="auto"", dropped a redundant test assert).Questions for review
(1)
moc_andprimitive — ✅ confirmed by @espg (comment 4834768382). No change.(2) Boundary-inclusivity asymmetry — ✅ resolved per @espg: documented explicitly in
docs/aoi_mask.md, behavior kept as-is.(3) Rect densification resolution — implemented with
resolution="auto"(extent-adaptive, parameter-free, dependency-free; ~100 pts/side). If you'd rather pin a guaranteed edge-slop bound, the densify resolution could be exposed as a config knob — say the word and I'll wire it. Otherwise"auto"stands.(4) WKB/WKT AOI input — deferred, blocked on espg/mortie#89 (not merged; pinned mortie 0.8.2 has no WKB/WKT API). Rolling it in here would require an unreleased mortie pin, which I won't do. Two options for your call: (a) leave it deferred to a follow-up PR once espg/mortie#89 merges and is released (this PR lands the Lambda + densify + boundary-doc work now), or (b) hold this PR until espg/mortie#89 lands and add WKB/WKT here. I've taken (a) by default — this PR is otherwise complete.
(5) Module size (non-blocking) —
src/zagg/runner.pyis now ~1180 lines (was 1172 before this PR; this PR adds ~8 net), over CLAUDE.md §4's ~1000-line soft limit. Pre-existing, not introduced here; flagging per §4 rather than splitting it in this PR.