GEDI waveforms 2/3: generic vlen reader primitives, paired-asset shardmap, flux transform + gedi01b template - #432
Conversation
| return None | ||
|
|
||
| # ---- The gather map: (record, within-record) index per base row. | ||
| parent_idx, within_idx = expand_link_indices(ibeg_arr, cnt_arr, index_base, n_base) |
There was a problem hiding this comment.
🤖 from Claude (review)
The gather map is built at full base rate even when a plan is active, which undoes the planned read's whole purpose.
expand_link_indices(ibeg_arr, cnt_arr, index_base, n_base) is called with the granule-wide n_base (line 262) before anything is sliced to the plan, so it allocates parent_idx and within_idx as two int64 arrays of length n_base — 16 bytes per sample in the whole group, independent of how selective the shard is. Only afterwards are they subset via global_idx (lines 264-266).
For the shipped template that is the dominant allocation of the read. A GEDI L1B beam group is ~1420 samples × O(10^5) shots, i.e. n_base ≈ 10^8; 16 B/row ≈ 1.6 GB for the two index arrays alone, against worker.memory: 2048 in gedi01b_waveform_healpix_hive.yaml. The planned arm exists precisely so an o9 shard touching a handful of shots never materializes anything at sample rate (issue #43's OOM motivation) — here it does, and it does so before the plan.base_slices reads that were kept small. The full-read arm adds a third full-rate int64 array (global_idx = np.arange(n_base), line 268).
Failure mode is a hard Lambda OOM on the first realistic granule, invisible to the test suite because the fixture's n_base is 13.
Fix shape: build the map only over the planned rows — for each (s, e) in plan.base_slices, the owning records are already known from plan.parent_runs, so parent/within can come from a np.repeat over just those records (and int32 suffices for within_idx). The unplanned arm can also skip global_idx entirely (it is the identity).
There was a problem hiding this comment.
🤖 from Claude
Fixed in 4680997 — the planned arm now builds the gather map at plan rate.
New _planned_gather_map(ibeg, cnt, index_base, n_base, base_slices) in read_vlen.py returns (global_idx, parent_idx, within_idx) sized to Σ(e - s) instead of n_base, walking only the records that overlap each planned slice (vectorised candidate selection, then the same index_beg/index_base placement in record order, so a later record shadows an earlier one exactly as the full-rate map did). Validation still runs over every record, so a granule whose link does not tile the base extent raises the same two ValueErrors it did before — a plan that skips a broken record must not skip its error.
The full-read arm keeps expand_link_indices (O(n_base) by necessity) but no longer materialises the third full-rate array: global_idx is None there and a local _to_planned() applies the identity, so cross_full / segment broadcasts are sliced without an arange.
Tests (TestPlannedGatherMap, tests/test_read_vlen.py): the plan-rate map equals the full-rate map restricted to global_idx, gap rows inside a planned slice still come back -1, both validation errors match, and the route test monkeypatches expand_link_indices to raise — so the planned arm regresses loudly if it ever reaches base rate again.
Still standing, in the same defect class but outside this finding's scope — the planned arm has two other full-base-rate allocations, both on the shipped template's path:
_read_segment_broadcasts(..., n_base, ...)returns onen_base-long value array per record-level variable (read.py:195). The GEDI template declares six, so that is ~6 × 8 B × n_base — larger than the index pair this commit removed. It could be aseg_values[parent_planned]gather instead of a broadcast, but only for the coordinates level; another level's link maps differently, so it is a read.py-shared refactor with its own test surface rather than a fold edit._expand_mask_to_baseallocates ann_basebool for each record-level / sibling-asset predicate (the template'sdegradeand the L2A trio) before_to_plannedslices it.
Leaving both for @espg to scope (a follow-up on #425, or fold into the #426 fleet-validation pass) rather than widening this fold.
| return [r[key] for r in records if r.get(key)] | ||
|
|
||
|
|
||
| def _resolve_granule_entries(records: list, driver: str | None) -> list: |
There was a problem hiding this comment.
🤖 from Claude (review)
Two of the five granule-URL dispatch sites were not converted, so the Lambda fan-out drops sibling assets and every paired run fails.
_resolve_granule_entries replaced _resolve_urls at runner.py:2813, :3029, :3638 — but the client transports still call _resolve_urls:
src/zagg/client.py:975—granule_urls = runner._resolve_urls(records, self.driver), fed straight intorunner._invoke_lambda_cell(...).src/zagg/client_transport.py:407— same call inside the asyncEventfan-out (_build_cell_event→_cell_payload→client.invoke).
Both build the worker event from plain URL strings, so assets never reaches the payload. The worker then opens no sibling handle, and the first group read raises
granule carries no open sibling handle for asset 'l2a';
was the shard map built with the paired-asset join?
(read_vlen.py:478) for every granule of every cell — the run fails wholesale, not degrades. That is the D8 invoke-transport path, i.e. the production route for a fleet GEDI build, and TestRunnerParity asserts client.py and _run_lambda build identical events, so the two are now provably out of step.
Nothing in tests/test_read_vlen.py::TestResolveGranuleEntries covers the client paths, which is why this is green.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 20c571c — both client dispatch sites now resolve through _resolve_granule_entries:
src/zagg/client.py_shard_work(the syncRequestResponsefan-out)src/zagg/client_transport.pydispatch_event_shards(the v2Eventfan-out)
The clamp arithmetic is unchanged: entries stay one-to-one with _resolve_urls (same href-less drop rule), which TestResolveGranuleEntries::test_count_matches_resolve_urls already pins, so _clamped_data_source(..., len(granule_urls)) still sees the same count. runner.py:3076's granule_ids=_resolve_urls(...) is deliberately left as-is — that feeds the stats record's granule-id list, which wants plain URL strings, not payload entries.
Two tests, one per transport, both driving a shardmap record that carries assets:
tests/test_client.py::TestRunnerParity::test_paired_asset_entries_reach_both_paths— the client's cell event and theaggpath's cell event both carry{"url": ..., "assets": {"l2a": ...}}for the paired cell, and a plain string for the single-asset cell.tests/test_client_transport.py::TestEventDispatch::test_paired_asset_entries_ride_the_event_fanout— same assertion on theEventpayloads.
Before the fix both tests fail on the bare URL string, i.e. they reproduce the "no open sibling handle for asset 'l2a'" wholesale failure without needing a worker.
| from zagg.processing.read_vlen import _vlen_read_group | ||
|
|
||
| if self.write_back: | ||
| self._prebuild_group_maps(h5obj, group, data_source) |
There was a problem hiding this comment.
🤖 from Claude (review)
index: inline with write_back: true cannot read a vlen source at all — _prebuild_group_maps chokes on both new grammar forms.
This branch calls self._prebuild_group_maps(h5obj, group, data_source) for the vlen route, but that helper predates the grammar and makes two assumptions the vlen source breaks (inline.py:548 and :557):
paths = [tmpl.format(group=group) for tmpl in data_source["coordinates"].values()]
paths += [tmpl.format(group=group)
for tmpl, _ in _variable_specs(data_source["variables"]).values()]Checked against the shipped template:
coordinates values: ['/{group}/geolocation/latitude_bin0',
'/{group}/geolocation/longitude_bin0', 'shots']
_variable_specs -> KeyError 'path'
coordinates.values()now yields the non-path sentinel"shots"(coordinates.level), so the prebuild tries to build a chunk map for a dataset literally namedshots— per its own docstring, "Missing datasets raiseKeyError"._variable_specs(read.py:242) doesentry["path"]for any mapping entry, and asynthesize:entry has nopath→KeyError: 'path', as reproduced above.
So the combination the template's own comment advertises ("the vlen route also runs behind index: inline") raises before a single byte is read whenever write-back coverage is on. _variable_specs needs the same synthesize-skip the read route applies at read_vlen.py:313, and the coordinate loop needs to skip level.
Related test gap: no test drives InlineIndex.read_group on a vlen source at all, so neither this nor the compiled-read_fn arm of _vlen_read_group (the execute_read_plan path with a backend-supplied read_fn) is exercised anywhere — the whole suite runs the read_fn is None bridge.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 85f9366 — _prebuild_group_maps is now vlen-aware on three counts (the two you found, plus one the same reproduction turned up):
coordinatesiterates items and skips thelevelkey — it names a record LEVEL, not a dataset, so nothing tries to mapshots.- Variables go through a new
read_vlen.path_form_variables()(variablesminus thesynthesize:entries) before_variable_specs, so the normalizer never sees apath-less entry. The synthesized column's record-rate endpoints (start/stop) are added instead — those datasets genuinely are read, so they belong in the manifest set. - Sibling-asset filters (
f["asset"] is not None) are skipped:/{group}/quality_flag,/{group}/sensitivity,/{group}/rx_assess/rx_clipbin_countlive in the paired granule, which this backend never opens — mapping them wouldKeyErroron the primary exactly asshotsdid.
The level loop now covers read_plan.spatial_index and coordinates.level (deduped; the same level in any config declaring both), so a vlen source with no read_plan still contributes its record coordinates and index_beg/count.
read_vlen._vlen_read_group uses the same path_form_variables helper now, so the skip rule has one definition.
Tests: tests/test_read_vlen.py::TestInlineWriteBackPrebuild, four cases driving InlineIndex(write_back=True)._prebuild_group_maps over the shipped template's data_source with build_chunk_map spied — the spy raises KeyError for any path the synthetic granule does not hold, so it reproduces the real failure. It asserts the covered set (rxwaveform, the link pair, the record coordinates, the synthesis endpoints, deduped), that shots is absent, that no L2A path is mapped, and that the no-read_plan case still maps the link.
On the wider gap you flag — no test drives InlineIndex.read_group end-to-end on a vlen source, because that needs a real chunk-mappable GEDI granule (the fixture suite's _open_fixture() is ATL03). The prebuild is now covered; the compiled-read_fn arm of _vlen_read_group stays uncovered and is left standing for @espg — it fits the #426 fleet-validation pass better than a synthesised HDF5 fixture.
| # Present (possibly empty) exactly when a sibling join ran, so the | ||
| # caller can distinguish "nothing dropped" from "never joined"; | ||
| # rides the manifest JSON as part of the build result. | ||
| meta["pairless"] = pairless |
There was a problem hiding this comment.
🤖 from Claude (review)
metadata["pairless"] is an unbounded per-granule list stored in the map metadata, and map metadata is copied into every cell payload and every leaf sub-map object.
The list holds one {id, missing} dict per unpaired granule with no cap. For a mission-scale GEDI build a few percent pairless over ~10^5 granules is O(MB) of JSON. It then rides two paths that were designed for small metadata:
runner.py:3646(andclient_transport.py:409) buildsubmap = {"grid_signature": ..., "metadata": catalog_data["metadata"], "granules": records}per cell._cell_payloadsilently drops the whole submap block when the event exceeds the 256 KB async cap (runner.py:5506,logger.debug) — so a largepairlesslist turns off the issue Unified second-pass rollup sweep: stats, shardmap sub-maps, MOC, overviews #300 leaf sub-map for the entire run, and the only trace is a debug log.sweep.write_leaf_submapexplicitly strips run-wide fields that must not be republished per leaf —for stale in ("aoi_mask", "build_wall_s", "reproject"): meta.pop(stale, None)(sweep.py:326).pairlessis exactly that class of field and is not in the list, so every leaf object publishes the run-wide pairless list.
The espg amendment asks for reporting, not for the list to be in the hot payload. Suggest keeping a count (+ a capped sample, e.g. first 50) in metadata and writing the full list to a sidecar next to the map, and/or adding pairless to write_leaf_submap's strip list.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 616d9ab, on both paths you name — the report stays on the run's own map, which is where it is read.
sweep.write_leaf_submap:pairlessjoins the strip list (for stale in ("aoi_mask", "build_wall_s", "reproject", "pairless")). It is exactly that class of run-wide field.- New
runner._submap_metadata(metadata)drops it from the per-cell sub-map block at all three construction sites —runner.py(_run_lambda),client.py_shard_work,client_transport.pydispatch_event_shards— so it never counts against the 256 KB async cap whose only remedy is the silentlogger.debugdrop of the whole block.sibling_assetstays: it is one short string and it is the block's identity.
Deliberately not done: capping the list or moving it to a sidecar. The espg amendment ratified "report, never just count" for metadata["pairless"], and the size problem was the copies, not the original — the run map is written once, not per cell and per leaf. If a mission-scale run makes the run map itself unwieldy, the sidecar split is an @espg call, not a fold one.
Tests: test_sweep.py::TestSubmapRollup::test_leaf_submap_strips_the_run_wide_pairless_report (500-entry list in, absent from the leaf object, sibling_asset retained), test_client.py::TestDispatch::test_pairless_report_stays_out_of_the_cell_submap, and the event-transport assertion in test_client_transport.py::TestEventDispatch::test_paired_asset_entries_ride_the_event_fanout.
| if len(coarse_lats) == 0: | ||
| _record_obs_read(io_stats, 0) | ||
| return None | ||
| n_base = int(cnt_arr.sum()) |
There was a problem hiding this comment.
🤖 from Claude (review)
n_base = cnt_arr.sum() makes the documented gap-row handling unreachable, and turns a real gap into a crash rather than a dropped row.
n_base is derived from the link counts, so by construction the records tile exactly n_base rows. Consequences:
- The
parent_idx == -1path advertised inexpand_link_indices("a gap surfaces as an unassignable row, never as data borrowed from a neighbor") and the NaN path insynthesize_linspacecannot be reached through_vlen_read_group: a granule whose records leave a hole shifts every later record up, so the last record tripsbeg + cnt > n_base→ValueError: record N range [...] exceeds base sizeand the group read dies.TestExpandLinkIndices::test_gap_rows_unassignedandTestSynthesizeLinspace::test_gap_rows_nanonly pass because they call the helpers directly with a hand-pickedn_basethe route never produces. - The mirror case — the flat dataset being longer than
Σcount(trailing samples, a truncated last record) — is worse in the unplanned arm:arrays_by_path[path]is then the true-length array whilemask_spatialhas lengthn_base, andvalues[mask_spatial](line 401) raisesIndexError: boolean index did not match indexed array, with nothing pointing at the tiling assumption.
The dense route documents the same cnt.sum() assumption (read.py:444-449) but does not claim gap tolerance on top of it. Either derive n_base from the base dataset's own length (h5coro exposes shape) and let the -1 rows do their job, or drop the gap-tolerance claims from both docstrings so the contract matches the code.
There was a problem hiding this comment.
🤖 from Claude
Taking your option (b'): leave the derivation, make the contract honest — a1deb47.
n_base = Σcount is the dense route's own assumption, stated verbatim at read.py:444-449 ("#43's contiguity assumption ... identical to ibeg_arr[-1] - index_base + cnt_arr[-1] when contiguity holds"), and _planned_read_group requires the same shape of the vlen source's read_plan.spatial_index. Deriving it from the base dataset's own length instead would make the two routes disagree about what "the base extent" is, on the reader's hottest assumption, in a fold commit — that is an @espg call, not mine.
What changed is the documentation, in two places:
- at the derivation in
_vlen_read_group: the route never reaches theparent_idx == -1rows by construction; a granule that violates contiguity raises out of the gather map ("does not tile the declared base extent") rather than silently borrowing a neighbour's record — which is the property those rows exist to guarantee, just enforced as a raise instead of a drop. - in
expand_link_indices's docstring:n_baseis the caller's, the-1tolerance is for the offline/unit callers that pass their own base extent, and through the route it is unreachable.
So the gap-handling claim is now scoped to the surface where it is reachable, and TestExpandLinkIndices::test_gap_rows_unassigned / TestSynthesizeLinspace::test_gap_rows_nan read as what they are — unit contracts of the primitives, not route behaviour. The trailing-samples mirror case (values[mask_spatial] IndexError) stays a raise; it is the same contiguity violation, and it fails loudly rather than silently mis-tiling.
| for rec in sibling_records: | ||
| key = sibling_join_key(rec["id"]) | ||
| if key is not None: | ||
| sib_by_key[key] = rec |
There was a problem hiding this comment.
🤖 from Claude (review)
Duplicate sibling join keys silently collapse — last one wins, and the loser is neither paired nor reported.
sib_by_key[key] = rec overwrites on collision. Two sibling records sharing a key (a paginated CMR query returning a granule twice, a reprocessed L2A granule whose id differs only in the release/production fields the key deliberately ignores, or two .h5/.h5.iso.xml-style entries) means:
- the surviving record is arbitrary — it is whichever
granule_records()yields last, not the newest; - the dropped duplicate never appears in
pairless, becausematchedis keyed on the join key, not the record, so the loop at the end sees the key as matched and stays quiet.
The whole point of the pairless report is that nothing leaves the build silently. Since the key deliberately drops the release/production fields (_SIBLING_ID_RE matches L1B 005 against L2A 003), collisions are a realistic consequence of that design decision, not a pathological input. Suggest detecting a second record for an existing key and either raising or reporting it ({"id": ..., "missing": "duplicate"}) — and doing the same for duplicate keys on the primary side, which currently pair N primaries to one sibling without comment.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 0137843 — the join is now deterministic and nothing leaves it silently.
Sibling side: sib_by_key keeps the first record on a key (catalog order, where last-writer-wins made the survivor arbitrary) and every shadowed record is reported as {"id": ..., "missing": "duplicate-key"}. That entry is accurate in the report's own terms — a shadowed sibling contributes nothing to the build, exactly like a pairless one.
Primary side: two primaries on one key still both pair to the same sibling and are both kept — excluding a primary is destructive, and a duplicate key there means a duplicated acquisition in the primary catalog, which is the operator's data-selection call rather than the join's. It is no longer quiet, though: a warning names the count, the asset, the first five ids, and says they were kept, not excluded.
ShardMap.build's docstring lists the third missing value; _pair_sibling_records's explains the first-wins rule and why collisions are a consequence of the key's design (_SIBLING_ID_RE matching L1B 005 against L2A 003), not a pathological input.
Tests in TestPairedAssetBuild: test_duplicate_sibling_key_is_deterministic_and_reported (two L2A records differing only in the release field — the paired asset is the first, the second is in pairless as duplicate-key) and test_duplicate_primary_keys_are_kept_but_warned.
| op: ge | ||
| value: 0.9 | ||
| - asset: l2a | ||
| dataset: "/{group}/rx_assess/rx_clipbin_count" # saturation counter: 0 = unclipped |
There was a problem hiding this comment.
🤖 from Claude (review)
The template's HDF5 paths are pinned only by a fixture that copies them, so a wrong path is undetectable until a fleet run.
tests/test_read_vlen.py::TestGediTemplate::test_template_reads_and_aggregates_the_fixture looks like end-to-end validation of the template, but _l2a_arrays() / _l1b_arrays() key their canned arrays off the same strings the template declares (/{group}/rx_assess/rx_clipbin_count, /{group}/geolocation/degrade, …). The test therefore passes for any self-consistent pair of strings, including a path that does not exist in GEDI01_B/GEDI02_A. A wrong path fails at read time on the first real granule — after fleet dispatch, on every cell.
The L2A trio in particular is worth a citation to the product dictionary before this ships, especially rx_assess/rx_clipbin_count (the saturation counter). Its semantics are also asserted only by a comment (# saturation counter: 0 = unclipped); if the real dataset is per-algorithm or lives at a different depth, op: le, value: 0 silently rejects (or admits) every shot.
Two cheap ways to make this real, either is fine: (a) drop a tiny real-granule dataset-name listing under tests/data/ and assert the template's declared paths are a subset of it, or (b) note in the template header which product-dictionary version/table each path came from, so review can check them by hand. As it stands the "how tested" claim for phase 5 is weaker than it reads.
There was a problem hiding this comment.
🤖 from Claude
Agreed on the diagnosis, declined on the remedy — with your option (b) applied as documentation, 71ad0ef.
The reading is right: _l2a_arrays() / _l1b_arrays() key off the strings the template declares, so the fixture pins the grammar wiring (origin-1 gather, expand, synthesis, the record-level and sibling-asset predicate plumbing, and that the whole surface aggregates), not NASA's product dictionary. It cannot fail on a wrong path.
Why not option (a) here: a real-granule dataset listing under tests/data/ is a real-product artifact, and real-product validation for this template is #426's scope — @espg executes fleet runs himself (the operator-runs convention), so an agent-fabricated listing would be a claim about GEDI01_B/GEDI02_A rather than an observation of one, and a wrong listing would pin the template to the wrong thing while reading as verification. Worse than the current honest gap.
So option (b), in the template header: the paths follow the GEDI V002 layout; the suite pins the grammar, not the dictionary; a wrong path fails on the first real granule; verification is part of #426, and rx_assess/rx_clipbin_count is what to check first there — a per-algorithm or deeper dataset would make op: le, value: 0 admit or reject every shot, which is exactly your point about the semantics resting on a comment.
The "how tested" claim for phase 5 should be read as scoped that way; leaving the stronger form of the finding standing for @espg to settle when #426 runs.
| if pairless: | ||
| logging.warning( | ||
| "ShardMap.build: %d pairless granule(s) excluded from the build " | ||
| "(no %s sibling / no primary); e.g. %s", |
There was a problem hiding this comment.
🤖 from Claude (review)
Nothing checks that the sibling catalog was queried over the same AOI/time window as the primary, and the failure mode is silent data loss.
_pair_sibling_records treats "no sibling record in this catalog" as "this acquisition has no L2A" and excludes the primary from the build. But the sibling catalog is just whatever the caller passed. If the two CMR queries differ at all — a different AOI polygon, a slightly different temporal window, a paging/retry that dropped results, or CMR's great-circle edge sag on the polygon (a known ~2.4% differential on the 88S catalogs) — then genuinely paired granules land in pairless and their data never enters the product. The build reports it, but as an expected-looking "no l2a sibling" line among the real ones, so the operator has no signal distinguishing "L2A genuinely missing" from "I built my sibling catalog wrong".
Two concrete guards worth considering, given exclusion is destructive and this runs unattended:
- compare the two catalogs' query metadata (collection/temporal/AOI, whatever
Catalog.metadatacarries) and warn loudly when the sibling's window/AOI is not a superset of the primary's; - make a high pairless fraction fatal rather than a warning (e.g. raise above a threshold, overridable), so a mis-scoped sibling query fails the build instead of quietly producing a thinned product.
ShardMap.build's docstring should also state the requirement explicitly — right now the only description of the sibling catalog is "e.g. the GEDI02_A catalog beside a GEDI01_B primary".
There was a problem hiding this comment.
🤖 from Claude
Took your guard (2), in the loud-warning form — 2481fe7.
When the sibling join reports any pairless granules, the build now also computes the primary unpaired fraction (missing == sibling_asset, over paired + unpaired primaries) and, past _PAIRLESS_ALERT_FRACTION = 0.5, emits a second warning that names the fraction, says the granules are EXCLUDED, and points at the cause: "check that the sibling catalog was queried over the same AOI and time window as the primary (a mis-scoped sibling query thins the product silently)". That is the signal the operator was missing — per granule the two causes are indistinguishable, in aggregate they are not.
Not raising, and not comparing the two catalogs' query metadata:
- raising turns a data-quality condition into a new build-failure mode with a threshold nobody has calibrated against real GEDI coverage yet — and a genuinely sparse-L2A region would then fail a legitimate build. A loud warning is reversible; a raise on an unattended run is not.
- the metadata comparison needs a shared, trustworthy shape for
Catalog.metadataacross CMR and STAC sources (today it carries whatever the source put there, e.g.collection+bbox), so the "is the sibling's window a superset" test would be doing geometry on optional fields. That is a design question for @espg rather than a fold edit; leaving it standing.
ShardMap.build's docstring now states the requirement explicitly — the sibling catalog is taken as authoritative and must be queried over the same AOI and time window as the primary, with the escalation named.
Tests: test_mostly_unpaired_primary_escalates_the_warning (1 of 3 primaries pair → escalated) and test_mostly_paired_primary_does_not_escalate (2 of 3 → the ordinary pairless warning only).
|
🤖 from Claude Unblocked: #431 merged (rebase-merge, head ed8a31a), so the §2.0 weights declaration + δ machinery this PR builds on is now on main. Synced by merging
|
|
🤖 from Claude CI red on the merge commit is not this PR's code: this was the first zagg CI run to resolve mortie 0.9.8 (released today), whose authalic latitude default changes every morton cell id — so Fix in flight on |
|
🤖 from Claude Conflict with post-#436 main resolved in 769945a — and it was semantic, not textual. This branch pairs sibling records before the intersection and its comment relied on the fast path re-aligning "to the catalog table by id"; PR #440 (merged) deleted by-id alignment — the stored-index plan now aligns positionally over the raw table ( Resolution: a paired build never plans — when An indexed paired fast path (intersect positionally first, pair only the hit records, with catalog-wide pairless reporting preserved) is a real future optimization once GEDI catalogs are routinely indexed — deliberately not attempted in a merge resolution; it changes pairless-reporting semantics and deserves its own issue when wanted. Local: |
1 participant
Closes #425. Refs #422 (design + rulings in comments); depends on #431 (the phase-1 spec PR:
weights:declaration + δ raise) — see "Dependency on #431" below.Blocked by #431(#431 merged 2026-08-16 — unblocked; main merged into this branch at 344c767)What this does
GEDI waveforms 2/3: makes vlen-packed waveform products (GEDI L1B
rxwaveform) pure config, per the rulings on #422:src/zagg/processing/read_vlen.py(split generic per the 1,200-line cap rule —read.pyis at ~1,110 and the route would cross it):link(index_beg/count/index_base: 1for GEDI's origin-1rx_sample_start_index— the exact ATL03ph_index_begprecedent). Planned reads slice the flat datasets to the shard's records via the existingplan_read/execute_read_planmachinery.data_source.coordinates.level: shots) expand by sample count to base rate; per-record scalars reuse the existing segment-levelvariablesbroadcast.{synthesize: linspace, level: shots, start: …, stop: …}variable entries compute per-sample values (GEDI elevation fromelevation_bin0/elevation_lastbin), endpoints exact, single-sample records getstart._read_groupkeyed oncoordinates.level; the dense routes are untouched.ShardMap.build(..., sibling_catalog=, sibling_asset="l2a")): catalog-time sibling join on the shared granule-id core (sibling_join_key: datetime/orbit/sub-orbit/track + theV00Ngeneration suffix, so pairing is pinned within a product generation). Paired entries carryassets: {l2a: {id, s3, https}}via the existing_granule_entryassets passthrough — ATL03 entries byte-identical. Pairless granules are excluded and reported:metadata["pairless"]lists{id, missing}both directions (primary without sibling, sibling without primary) plus a build-time warning (espg amendment). Composes with the ShardMap.build at bulk-catalog scale: process-pool parallel build; persistent footprint index #396footprint_cellsfast path (pairing runs before the plan; the plan re-aligns by id). Reproject/refine carries sibling assets through.data_source.assets.<name>.join.{left,right}declares the record-key datasets (shot_numberboth sides); filters gain anasset:key and evaluate on the joined sibling rows per record, expanding to base rate through the record link. A record with no sibling match fails the predicate — unfilterable shots never pass a quality gate silently.src/zagg/stats/waveform.py: parameterized only by (noise model, gain constant, operating point) — threshold-to-zero clip at n_σ·σ with n_σ derived from the declared false-positive target (pure-numpy probit, per-shot effective-trial count from the sample count and noise correlation length),weight = (count − noise_mean) × g, zero-weight rows dropped.build_waveform_digestsorts(elevation, weight)sub-centroids and runs the shared_compresspath ofstats/tdigest.py, so the payload is §2-sorted and pooled multi-shot merge is the existingmerge_tdigests_kwayfold. Companions (plain names per ruling):shot_count,shot_number(sentinel 0 when mixed),single_shot_valuepass-throughs (NaN when mixed).src/zagg/configs/gedi01b_waveform_healpix_hive.yaml: o9 shards / o18 cells /chunk_inner: 12, sharded hive, pooled (composabilitynone, no pyramid),delta: 8192, provenance attrs (gain name+version, operating point, clip params), L1Bdegradefilter + the L2A trio (quality_flag,sensitivity,rx_assesssaturation counter), companions passed through from L1B.Phases
read_vlen.py,_read_groupdispatch, config grammar validation incl.asset:filters) + synthetic mini-granule testsstats/waveform.py: flux transform over the shared compress path + companions (round-trip, rx_energy closure, pooled-merge tests)gedi01b_waveform_healpix_hive.yamltemplate + packaged-config test (GEDI waveforms 1/3: §2 counts/flux weights declaration + δ=8,192 raise #431-coupled edit marked below)Dependency on #431
The
weights: fluxdeclaration (config key + attrs stamp + spec §2.0) is implemented in #431 (claude/424-weights-declaration-delta). This PR keeps its #431-coupled surface minimal and marked:weights: fluxline commented, marked#431 rebase— everything else in the template is independent.weights: fluxline + its attrs interplay are enabled in a single small commit.How tested
tests/test_read_vlen.py: synthetic GEDI-shaped mini-granule (canned arrays behind the_FakeH5stub — thetest_processingfixture convention; both products) — origin-1 gather pinned against off-by-one, expand-by-count coordinates/companions, linspace synthesis endpoints + single-sample records + gap rows, planned-vs-full parity, record-level filter expansion, expression filters over synthesized columns, the L2Ashot_numberjoin (unmatched shot dropped, predicates ANDed, missing handle loud), and the vlen config-validation surface.tests/test_shardmap.py::TestSiblingJoinKey/TestPairedAssetBuild: key parsing (release-field invariance, generation pinning), paired entries carry the sibling trio, pairless excluded + reported both directions, warning fires, empty-list-when-fully-paired, JSON round-trip, no-join maps unchanged.tests/test_waveform.py: threshold derivation (1σ pin, trial/correlation monotonicity), threshold-to-zero + gain weights, loss-free rows equal the clipped samples, the round-trip reconstruction acceptance, rx_energy closure, pooled-build == k-way merge, mass additivity, companion sentinels, and the params-as-column wiring throughcalculate_cell_statisticswith the exact template params.tests/test_read_vlen.py::TestGediTemplate: the shipped template's declared surface driven end-to-end over the synthetic mini-granule (read filters + join into cell stats + companions).uv run --extra test pytest).Questions for review
tests/test_lambda_build.py::TestFunctionBuild::test_function_build_succeedsfails locally on this machine (the build script's pip resolves against a Python 3.10 interpreter, sozarr>=3.1.5has no candidate — an env artifact; the real dual-arch build runs in CI'slambda-build.ymlimages). Also:ruff check src testsfails withN818onsrc/zagg/registry.py(UnknownCapability), andruff format --checkflagstests/data/benchmark/README.md— both predate this branch (flagging per §4, not fixing).V\d+generation suffix and ignores the release/production fields between track and generation (L1B005vs L2A003). If a future GEDI collection changes the id shape,sibling_join_keyreturnsNoneand the granule reports as pairless rather than mispairing — is that the right failure posture, or should an unparseable id raise?level:on an asset filter is rejected. Flag if a base-rate sibling dataset join is a foreseeable need.