Skip to content
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

- unindexed shard-map builds cover at `parent_order` and intersect before decoding records (#445) ([#447](https://github.com/englacial/zagg/pull/447)) by @espg
- An unpinned unindexed HEALPix mortie `swath` build now covers at the output
grid's `parent_order` instead of its chunk order, so `metadata["mortie_order"]`
in newly built manifests records the shard order (e.g. 9, not 13). Assignment
is measurably identical at the production order pairs (the
`bench/neon_order_sweep.py` invariant, verified at 555,867 granules); at
coarse grids the new default is a documented conservative superset, never a
subset. Explicit `mortie_order=` pins are unchanged.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

"Explicit mortie_order= pins are unchanged" is contradicted by this entry's own third bullet and by PR #447's delta 6.

What #447 claims unchanged is narrow — "An explicit mortie_order= is still honored literally and still validated against parent_order (#92)" — i.e. the resolved order. The assignment for a pinned build did move, measurably, for one population:

6. A pinned mortie_order puts an indexed catalog on the live cover too. … measured on a two-part MultiPolygon at HealpixGrid(11, 19, chunk_inner=13) with mortie_order=13, 8 shards on main -> 13 here for the multi-part granule.

Pinned in the merged suite at tests/test_shardmap.py:1470 (TestLiveCover::test_a_pinned_indexed_build_covers_live_too).

So a reader gets "your pinned build is unaffected" on line 17 and, three lines down, "an explicit mortie_order= pin always covers live, indexed or not, so a MultiPolygon footprint assigns as a union-of-parts superset". Both cannot be read at face value. Scoping the first to what #447 actually claims — e.g. "The order an explicit mortie_order= pin resolves to is unchanged" — leaves the third bullet to carry the assignment consequence without the entry arguing with itself.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in 4f02e5c — scoped to what #447 actually claims, so the first bullet no longer argues with the third:

subset. The order an explicit mortie_order= pin resolves to is unchanged (it is still honored literally and still validated against parent_order).

That leaves the assignment consequence entirely to the third bullet (delta 6 / TestLiveCover::test_a_pinned_indexed_build_covers_live_too), which is where it belongs.

- Unindexed builds cover from the catalog's WKB column and intersect before
materializing granule records — ~2.5x faster at clone scale (87 s -> 39 s).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

The ~2.5x multiplier disagrees with its own quoted endpoints, and the 2.5x figure in PR #447 is a different ratio entirely. 87 s → 39 s is 2.2x.

From PR #447's measured table (555,867-granule clone, same machine): before, mortie_order=9 total 86.6 s, after, default total 39.3 s → 86.6 / 39.3 = 2.20x. The most generous pairing available (86.9 s from the issue vs the best after-run 37.0 s) is still only 2.35x; pinned→pinned (86.6 → 37.4) is 2.32x. No pairing yields 2.5x.

Where 2.5 comes from in #447 is its opening paragraph, and it is not a speedup at all:

that was 86.9 s to assign 2,357 granules, ~2.5x the cost of index_footprints(9) + an indexed query put together

i.e. old-records-path cost vs index-build-plus-indexed-query cost — a different pair of quantities than the before/after this bullet describes.

Two ways out: say ~2.2x faster at clone scale (87 s → 39 s), or quote the reading that actually matches this entry's subject — the first bullet is about the unpinned default, whose before/after is 1075 s → 39 s (~27x), the number #447 calls "the cost espg's ruling removes".

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in df6714d — took the second option: the bullet now quotes the reading its own subject describes.

At clone scale (555,867 granules) the unpinned default — the case the bullet above changes — goes 1,075 s -> 39 s (~27x), no longer covering every footprint at the chunk order to answer a shard-order question; a build that already pinned mortie_order=9 goes 86.6 s -> 39.3 s (~2.2x).

Both numbers come straight from PR #447's measured table (before, default 1075.0 s / before, mortie_order=9 86.6 s / after, default 39.3 s), and neither is the ~2.5x of #447's opening paragraph, which you're right is old-path cost vs index_footprints(9) + indexed query — a different pair of quantities. The PR body's matching sentence was corrected too.

- Disclosed: an explicit `mortie_order=` pin always covers live, indexed or not,
so a MultiPolygon footprint assigns as a union-of-parts superset; single-part
CMR granules are unaffected.
- rename parent_morton event field to shard_key (#24) ([#42](https://github.com/englacial/zagg/pull/42)) by @espg
- Concurrency-aware Lambda orchestrator: pre-flight probe + FD-exhaustion guard ([#41](https://github.com/englacial/zagg/pull/41)) by @espg
- drop shapely as an intersection backend (#36) ([#39](https://github.com/englacial/zagg/pull/39)) by @espg
Expand Down
13 changes: 10 additions & 3 deletions tests/data/benchmark/configs/s2_neon_o9.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,15 @@
# the shipped src/zagg/configs/sentinel2_l2a.yaml except ``parent_order: 9`` --
# the o9 dispatch shard matching the point-pipeline legs (~12.8 km shards, 4
# over the SERC box, 4^10 order-19 cells each; the same override espg's
# operational S2 SERC run pins).
# operational S2 SERC run pins) -- and the harness-local ``pyramid: false``
# opt-out below. Both are packaging: the semantic hash (D19) of this config
# equals the shipped one, so the leg measures the shipped product.
#
# Layout notes: ``store_layout: hive`` -- the production default for HEALPix
# raster since issues #247/#253 (issue #237 promoted, ratified by @espg on
# issue #272; the flat interop profile is deprecated). ``sharded`` does not
# apply to the (time, cells) slab writes (raster is never sharded, issue #247).
# issue #272; the flat interop profile is deprecated), restated here, not a
# divergence. ``sharded`` does not apply to the (time, cells) slab writes
# (raster is never sharded, issue #247).
data_source:
reader: raster
bands:
Expand All @@ -30,6 +33,10 @@ output:
# ratified issue #272). One leaf zarr object per array per dispatch shard.
store_layout: hive
pyramid: false # overview sweep opted out pending Phase E fleet sizing (issue #201)
# Time axis as mortie toc words (spec §8, issue #443) -- output-defining, so
# it tracks the shipped config (issue #451): a store born on toc is a
# different product from a legacy-axis one.
time_encoding: toc
grid:
type: healpix
indexing_scheme: nested
Expand Down
31 changes: 31 additions & 0 deletions tests/test_raster_benchmark.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,37 @@ def test_raster_targets_manifest_consistent():
assert get_store_layout(cfg) == "hive"


def test_s2_neon_o9_tracks_the_shipped_sentinel2_config():
# The leg's config claims to be the shipped src/zagg/configs/sentinel2_l2a.yaml
# with the o9 dispatch shard (issue #451); hold it to that mechanically so a
# knob added to one and not the other (as time_encoding was, issue #443)
# cannot drift silently. Divergence budget: parent_order and the harness's
# pyramid opt-out -- both packaging, so the semantic hash (D19) matches.
from zagg.config import get_store_layout, load_config
from zagg.semantics import semantic_hash

bench = load_config(str(BENCH / "configs" / "s2_neon_o9.yaml"))
shipped = load_config(str(REPO / "src" / "zagg" / "configs" / "sentinel2_l2a.yaml"))

assert bench.data_source == shipped.data_source
assert bench.aggregation == shipped.aggregation
assert semantic_hash(bench) == semantic_hash(shipped)

def normalized(cfg):
# store is a run-local output path (the harness overrides it); grid
# indexing_scheme is descriptive-only (config.py rejects any other value).
out = {k: v for k, v in cfg.output.items() if k not in ("store", "grid", "pyramid")}
out["store_layout"] = get_store_layout(cfg)
out["grid"] = {k: v for k, v in cfg.output["grid"].items() if k != "indexing_scheme"}
return out

b, s = normalized(bench), normalized(shipped)
assert b["grid"].pop("parent_order") == 9
assert s["grid"].pop("parent_order") == 11
assert b == s
assert bench.output["pyramid"] is False

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude (review)

The divergence budget is enforced one-sidedly: pyramid is dropped from the comparison on both sides but asserted only on the bench side, so the shipped config can grow any pyramid declaration and this test still passes.

normalized() excludes "pyramid" (line 65), and line 74 pins only bench.output["pyramid"] is False. Nothing constrains shipped.output.

Verified by mutation on this branch — a harness reproducing the test body verbatim (data_source/aggregation equality, semantic_hash, normalized(), the parent_order pops, b == s, the line-74 pin), run against load_config-loaded copies of both configs:

PASS   baseline
PASS   shipped gains pyramid: {spacing: 2}
PASS   shipped gains pyramid: false          # divergence disappearing also unnoticed
FAIL   shipped gains emit_cell_ids           # correct
FAIL   bench pyramid becomes a mapping       # correct
FAIL   shipped reverts time_encoding         # correct

The hole is not incidental: the one output key exempted from the dict comparison is the same key the docstring names as half the divergence budget ("Divergence budget: parent_order and the harness's pyramid opt-out"). The stated purpose — "a knob added to one and not the other (as time_encoding was, issue #443) cannot drift silently" — holds for every knob except this one. And pyramid is not inert like the two other drops: src/zagg/configs/sentinel2_l2a.yaml acquiring pyramid: {overviews: [...]} (the issue #382 grammar) turns the overview family on for the shipped product while the leg keeps measuring it off, at an unchanged semantic hash — output.pyramid never enters semantic_core, so the D19 assertion above cannot see it either.

One line next to 74 closes it: assert "pyramid" not in shipped.output.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 from Claude

Fixed in 52b2531 — your one-liner, with a comment naming why the hole exists (the pyramid drop from normalized() plus output.pyramid being outside semantic_core, so neither the dict comparison nor D19 can see it):

    assert bench.output["pyramid"] is False
    assert "pyramid" not in shipped.output

The normalized() drop stays on both sides as-is; only the assertion is now two-sided. uv run --extra test pytest tests/test_raster_benchmark.py -q → 11 passed.

Separately in 44bc654, this test now resolves the bench config through the targets manifest (base / manifest["targets"]["raster_s2_neon_2025"]["config"]) instead of the hardcoded BENCH / "configs" / "s2_neon_o9.yaml", matching test_raster_targets_manifest_consistent beside it — so a manifest repoint cannot leave this test parity-checking a stale file.



def test_pinned_s2_catalog_carries_raster_entries():
# The pinned catalog is the fixed granule set (offline, no STAC): 2025
# Earth Search c1 items over the SERC box, with the raster entry fields
Expand Down
Loading