Skip to content

gedi01b template: enable the section-2.0 weights: flux declaration - #521

Draft
espg wants to merge 4 commits into
mainfrom
claude/gedi-weights-flux
Draft

gedi01b template: enable the section-2.0 weights: flux declaration#521
espg wants to merge 4 commits into
mainfrom
claude/gedi-weights-flux

Conversation

@espg

@espg espg commented Aug 25, 2026

Copy link
Copy Markdown
Member

Refs #424 (the §2.0 counts/flux weights declaration, merged as PR #431). No dedicated issue tracks this leftover; the authorization is espg's in-session ruling (2026-08-24, the production-notebook build session), and the standing flag is PR #510's "Questions for review" — its second bullet records both halves of this fix: the template ships weights: flux commented out pending the #431 rebase, and "post-#431 the carried gain is a bare scalar without the §2.0-required name/version."

What

The packaged GEDI template src/zagg/configs/gedi01b_waveform_healpix_hive.yaml builds rx_flux digests whose weight column is (count - noise_mean) * gain floats — calibrated flux — but shipped the §2.0 weights: flux declaration commented out (a PR #431 rebase leftover). Spec §2.0 makes an absent key read as counts (integer observation counts), so every store and ladder built from the template self-describes wrongly. Two changes, one flip:

  1. weights: flux enabled on rx_flux, the GEDI waveforms 1/3: §2 counts/flux weights declaration + δ=8,192 raise #431-era "pending rebase" comment trimmed to a normal explanation of why the key is load-bearing (leaf array attrs, zagg.pyramid.declared_fields manifest entry, sweep_overview._overview_config at every ladder level).
  2. attrs.gain reshaped to the §2.0 provenance mapping {name: unit, version: gedi01b-v002-placeholder, value: 1.0}, replacing the flat pre-GEDI waveforms 1/3: §2 counts/flux weights declaration + δ=8,192 raise #424 gain_name/gain_version/gain: 1.0 strings. This is not optional polish: validate_config refuses weights: flux without a gain mapping carrying at least name and version (config.py::_validate_output_kind, "requires calibration provenance"), so the uncommented declaration would not validate at all in the old attrs shape. (params.gain: 1.0 — the reducer kwarg — is unchanged.)

Tests

  • tests/test_read_vlen.py::TestGediTemplate::test_ratified_shape — the parsed template declares weights: "flux" with the exact gain mapping (was: gain_name truthy). It also pins the mirror the template comment asserts but nothing else enforced, assert flux["params"]["gain"] == flux["attrs"]["gain"]["value"], so a one-sided edit to a real GEDI constant cannot ship confidently-wrong provenance.
  • tests/test_semantics.py::TestGediTemplateFluxDeclarationboth semantic hashes pinned as constants so the identity flip is deliberate and visible: shipped flux-declaring template 7e0bf1818efa9259194bb43d0d1eaf7f23b5d9122d00b14797e28f1a5d9d02dd, prior counts-defaulting form c812c910f86057f9899aef3128a685d2ef78c5cbe26e7c90ae044f4d77feb4bd (weights: "flux" is output-defining per semantics.py; counts normalizes away as the absent-key default). _prior_form() reconstructs that prior form from HEAD's template rather than a frozen snapshot, so the literal is what freezes the identity: a later semantic edit moves the reconstruction, the constant fails, and the drift cannot pass silently.
  • tests/test_sweep_overview.py::TestWaveformPyramidDeclaration — the exact-manifest-entry pin now carries the real keys the declaration reaches: fields["rx_flux"]["weights"] == "flux" and fields["rx_flux"]["gain"] == {name, version, value} (pyramid.declared_fields keys the pair only when non-default, and build_pyramid_block carries it to every level — asserted on the block too).
  • tests/test_config.py::TestWeightsDeclaration::test_shipped_gedi_template_refuses_the_pre_424_flat_gain — the seam at template level: load the shipped gedi01b template, revert only its attrs to the flat pre-GEDI waveforms 1/3: §2 counts/flux weights declaration + δ=8,192 raise #424 keys while weights: flux stays declared, and validate_config must refuse it with "requires calibration provenance". TestWeightsDeclaration otherwise covers that refusal on _ragged_cfg synthetics only, and the packaged config is otherwise exercised only in its already-correct form, so this seam had no pin anywhere. (Every packaged template still passing validate_config verbatim is already covered incidentally by TestPackagedConfigsAreDispatchable::test_every_packaged_config_serializes_strictly, which calls default_config(name) — and default_config validates by default — so no separate method restates it.)
  • Cross-check: the production notebook's client-side graft of the same declaration (demo/12, untracked) hashes its delta-4096 recipe to 4f8287947a83abd3…; the template + the notebook's uniform-δ4096 override reproduces that hash exactly, so the template declaration and the notebook graft are semantically identical.

Local: ruff format --check src tests clean (155 files); ruff check src tests clean but for the pre-existing N818 below; pytest -q 4,851 passed, 39 skipped with the two pre-existing deselects noted below.

Scope — existing stores are NOT retconned

The existing SERC GEDI stores (serc_gedi_flux.zarr and the probe stores) were built from the counts-defaulting template: their frozen configs and hashes stay valid for their stores — the c812c910… pin above is exactly that identity, kept alive in the test. This PR changes only what new builds declare. The intended beneficiary is the CA GEDI store (not yet built); the §2.0 fold gate (check_weights_match) will correctly refuse folding old counts-defaulting arrays under a flux-declaring config, which is the spec working as designed, not a regression. The pyramid: false tail comment's stale "until PR #431 enables it" note is updated to match.

Spec obligation — checked, none owed

§4's normative-spec rule fires on a change to a wire format, attrs grammar, or versioned spec marker. The gain provenance mapping is already normative: docs/specification.md:275 requires "a gain key carrying at minimum the gain constant's name and version", and docs/specification.md:959 already requires the manifest entry to carry weights together with that provenance. This PR only conforms one shipped template to grammar that landed with #424 / #431 — it adds no grammar and moves no spec marker — so docs/specification.md, tools/generate_spec_fixtures.py, and tests/data/spec/ are correctly untouched.

Pre-existing failures (flagged, not fixed — CLAUDE.md §4)

tests/test_lambda_build.py::TestFunctionBuild::test_function_build_succeeds and tests/test_client_transport.py::TestStatusPoller::test_invoke_fault_burns_an_attempt_and_retries both fail on clean origin/main in this environment (local deploy-script pip environment / transport timing); deselected in the run above. ruff check src flags a pre-existing N818 on src/zagg/registry.py:64, untouched here and byte-identical to origin/main — and outside the PR lint bot's E,F,W,I selection.

Questions for review

  • The gain mapping's name: unit / version: gedi01b-v002-placeholder values are carried over verbatim from the old flat attrs — still placeholders. If a real GEDI gain constant should be named before the CA build, that's a template value edit on top of this shape (the shape is now spec-enforced).
  • Adversarial review raised pinning the SERC store identity against something independent of HEAD (a snapshot YAML under tests/data/) rather than relying on the c812c910… literal. Deliberately left standing rather than folded, to avoid expanding this PR's scope — the literal already makes drift loud, so this is a durability preference, not a defect.

🤖 Generated with Claude Code

https://claude.ai/code/session_014eRcohZWarXGLrNMsLXD4b

@espg espg added the implement label Aug 25, 2026
Comment thread tests/test_config.py Outdated
# the template does not ship (e.g. `weights: flux` requires the
# attrs.gain provenance MAPPING, spec §2.0 / issue #424).
for name in self._packaged_names():
validate_config(default_config(name, validate=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)

This test is an exact duplicate of the sibling test_every_packaged_config_serializes_strictly three lines above. default_config(name, validate=False) followed by validate_config(cfg) is literally the body of default_config(name) (src/zagg/config.py:286-293), and the sibling already iterates the same _packaged_names() calling default_config(name) — so "every packaged template passes validate_config verbatim" was already covered before this PR.

Verified rather than assumed: loading the shipped template and reverting only the attrs shape (flat gain_name/gain_version/gain: 1.0 under weights: flux) makes validate_config raise

ValueError: Variable 'rx_flux': weights 'flux' requires calibration provenance in attrs — a 'gain' mapping with at least 'name' and 'version' (spec §2.0…

which default_config(name) re-raises, so the pre-existing serializes-strictly test fails on that config too. The PR body's claim that this is "the test class that would have caught" the bug class is true of the class, but the new method adds no coverage.

Either drop it, or keep the explicit name and say in the comment that it pins, by name, coverage the sibling gets incidentally — rather than presenting it as the net that was missing.

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

Folded in bfafe96 — you were right that the method was pure duplication, so it is deleted from TestPackagedConfigsAreDispatchable; test_every_packaged_config_serializes_strictly already calls default_config(name) over the same _packaged_names(), and default_config validates by default (src/zagg/config.py:286-293).

Replaced rather than dropped outright, because the thing the sibling catches only incidentally is worth a named pin, and it is not the one the deleted method wrote. New method in TestWeightsDeclaration (tests/test_config.py), beside the synthetic flux cases:

cfg = default_config("gedi01b_waveform_healpix_hive", validate=False)
rx = cfg.aggregation["variables"]["rx_flux"]
assert rx["weights"] == "flux"
del rx["attrs"]["gain"]
rx["attrs"].update(
    {"gain_name": "unit", "gain_version": "gedi01b-v002-placeholder", "gain": 1.0}
)
with pytest.raises(ValueError, match="requires calibration provenance"):
    validate_config(cfg)

That is your own reproduction turned into the assertion: the shipped template with only the attrs shape reverted to the flat pre-#424 keys under weights: flux is refused. TestWeightsDeclaration covers the refusal on _ragged_cfg synthetics only, and the packaged config is otherwise exercised only in its already-correct form — so the template-level seam had no pin anywhere.

gain:
name: "unit"
version: "gedi01b-v002-placeholder"
value: 1.0

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)

value: 1.0 here and params.gain: 1.0 (line 201) are two independent literals that the comment directly above declares to be one thing ("value is the constant itself, mirroring params.gain") — but nothing enforces the mirror. _validate_output_kind only requires the gain mapping to carry name + version (extras are free-form under §2.0's "at minimum"), and the template shape test pins params.delta and attrs.gain but never params.gain. So editing the reducer's gain to a real GEDI constant — exactly what this PR's "Questions for review" anticipates — leaves the stored calibration provenance silently asserting 1.0, and provenance that is confidently wrong is worse than the absent provenance §2.0 exists to outlaw.

One line in tests/test_read_vlen.py::TestGediTemplate closes it beside the assertions this PR already adds there:

assert flux["params"]["gain"] == flux["attrs"]["gain"]["value"]

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

Folded in 29718ef, exactly the line you wrote, in tests/test_read_vlen.py::TestGediTemplate::test_ratified_shape beside the attrs.gain mapping assertion:

assert flux["params"]["gain"] == flux["attrs"]["gain"]["value"]

with a comment saying why the mirror is load-bearing (a one-sided edit to a real GEDI constant ships confidently-wrong provenance, which is worse than the absent provenance §2.0 exists to outlaw). Left the validator alone — §2.0 requires only name + version, and value is a template-local extra, so the template test is the right place to enforce the mirror rather than _validate_output_kind.

Comment thread tests/test_semantics.py
FLUX_HASH = "7e0bf1818efa9259194bb43d0d1eaf7f23b5d9122d00b14797e28f1a5d9d02dd"
# The prior counts-defaulting form: no weights key, flat gain_name /
# gain_version / gain attrs — the config the existing SERC stores froze.
COUNTS_HASH = "c812c910f86057f9899aef3128a685d2ef78c5cbe26e7c90ae044f4d77feb4bd"

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)

Both constants recompute correctly today — I reproduced 7e0bf181… from the shipped template, and c812c910… both from _prior_form() and from git show origin/main:src/zagg/configs/gedi01b_waveform_healpix_hive.yaml loaded verbatim (their semantic_core dicts compare equal), so the reconstruction is faithful and the pins are right.

The concern is durability of the claim, not the number. COUNTS_HASH is documented as "the config the existing SERC stores froze", but _prior_form() derives it from HEAD's template. Any later semantic edit to that template (a delta change, a filter, time_source) moves both constants together; whoever makes it updates both numbers, and this docstring then asserts SERC identity for a hash no SERC store ever carried — with nothing failing to signal that. The pin is self-referential, so it can only ever prove "these two forms differ", never "this is what SERC froze".

If the SERC identity is the thing worth keeping alive, pin it against something frozen (a snapshot YAML under tests/data/, or the bare literal with a comment that it holds only while the rest of the template is unchanged). If it isn't, the accurate framing for the docstring is "the same template minus this PR's flip" — which is all _prior_form() actually constructs.

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

Folded in dd1db21, taking your second option — the docstring, not a fixture.

Reasoning for the cheaper fix: the pinned literal already does the durability work you want. _prior_form() tracking HEAD means a later semantic edit moves the reconstruction, and then test_the_prior_counts_form_hashes_differently FAILS on the constant — so the drift cannot be silent; it forces a deliberate decision at the moment of the edit. A snapshot YAML under tests/data/ would buy the same signal at the cost of a second copy of the template to keep honest. What was wrong was only the claim, so the comment on COUNTS_HASH now says what is actually true:

    # The prior counts-defaulting form: no weights key, flat gain_name /
    # gain_version / gain attrs. This is what the SERC stores were built from,
    # but `_prior_form()` only RECONSTRUCTS it — from HEAD's template, so it
    # tracks "this template minus this PR's flip", not a frozen snapshot. The
    # LITERAL below is the freeze: a later semantic edit to the template (a
    # `delta` change, a filter, `time_source`) moves the reconstruction, this
    # constant fails, and whoever makes the edit has to decide deliberately
    # rather than drift past it.

If espg wants the SERC identity pinned against something independent of HEAD, that is a separate follow-up and I have left it standing rather than expanding this PR's scope.

@espg

espg commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Status check this run: no new work was needed — the PR is content-complete. Nothing new was pushed; HEAD is still dd1db217.

What was verified

  • The single implementation phase (ead469a) plus all three adversarial-review folds (29718ef, bfafe96, dd1db21) are landed and pushed. Every inline review thread has a 🤖 *from Claude* reply naming its fix sha.
  • CI is green across the board on dd1db217: ruff, test (3.12), test (3.13), build, build / build-x86_64, build / build-arm64 all pass (run).
  • Reproduced green locally: ruff format --check src tests clean (155 files); pytest -q4,851 passed, 39 skipped, with only the two known pre-existing deselects. ruff check src tests reports only the pre-existing N818 on src/zagg/registry.py:64, which is byte-identical to origin/main here and outside the lint bot's E,F,W,I selection — flagged, not fixed, per §4.
  • Confirmed no docs/specification.md / conformance-fixture obligation applies. §4's normative-spec rule fires on a change to a wire format, attrs grammar, or versioned spec marker. The gain provenance mapping is already normative — docs/specification.md:275 requires "a gain key carrying at minimum the gain constant's name and version", and docs/specification.md:959 already requires the manifest to carry weights together with that provenance. This PR only conforms one shipped template to grammar that landed with GEDI waveforms 1/3: §2 counts/flux weights declaration + δ=8,192 raise #424 / GEDI waveforms 1/3: §2 counts/flux weights declaration + δ=8,192 raise #431; it adds no grammar and moves no spec marker, so tools/generate_spec_fixtures.py and tests/data/spec/ are correctly untouched.

One conflict flagged rather than guessed (CLAUDE.md preamble)

CLAUDE.md §5 says a PR whose phases are all complete, findings folded, and CI green should get waiting and be marked ready for review, "don't leave it draft, so the state is unambiguous". This run's routine prompt instructs the opposite — never mark ready for review, draft only. Rather than pick a side, I took the reversible half both agree on: waiting applied, left in draft. Flipping it with gh pr ready is a one-liner whenever that is the call.

Standing for review (neither blocks the PR)

  1. The Questions for review item already in the body: name: unit / version: gedi01b-v002-placeholder are carried over verbatim from the old flat attrs and are still placeholders. The shape is now spec-enforced; naming a real GEDI gain constant before the CA build is a value edit on top of it.
  2. Left standing from the third review fold (discussion): _prior_form() reconstructs the counts-defaulting hash from HEAD's template, so the c812c910… literal is what freezes the SERC identity. Pinning that identity against something independent of HEAD (a snapshot YAML under tests/data/) would be a separate follow-up — deliberately not folded here to avoid expanding scope.

The three inline review threads are left unresolved by design — the fold replies on them, resolving is the reviewer's call.


Generated by Claude Code

@espg espg added the waiting label Aug 25, 2026
@espg

espg commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

🤖 from Claude

Follow-up to the status comment above: I also corrected the PR description, which had gone stale against the landed code. No code changed — HEAD is still dd1db217.

The Tests section named tests/test_config.py::TestPackagedConfigsAreDispatchable::test_every_packaged_config_validates_as_shipped, but the first review fold (bfafe96) deleted that method as pure duplication and replaced it with TestWeightsDeclaration::test_shipped_gedi_template_refuses_the_pre_424_flat_gain. The body was still advertising a test that no longer exists and claiming coverage the replacement provides differently. Corrected, and while there:

  • the test_ratified_shape bullet now mentions the params.gainattrs.gain.value mirror assertion added by 29718ef;
  • the test_semantics.py bullet now states what the c812c910… literal actually freezes, matching the docstring dd1db21 landed;
  • the local-run line and the pre-existing-failure list now match a verified run in this environment rather than the earlier session's numbers;
  • added a short Spec obligation — checked, none owed section recording why §4's docs/specification.md + fixture rule does not fire here, so the next reader does not have to re-derive it.

Description edits only — the code, tests, and hashes are untouched.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant