-
Notifications
You must be signed in to change notification settings - Fork 1
GEDI waveforms 1/3: §2 counts/flux weights declaration + δ=8,192 raise #431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
0fab8eb
95e2492
c80da29
676b35d
55e588a
cba7546
27a0287
7b2cfac
90b3135
c04d91f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -167,7 +167,7 @@ def fold_column(slabs: dict, fields: dict, *, cell_order: int, resolutions: list | |
| a fractional fold factor, which no guard downstream can read as a divisor | ||
| (both classes would surface it as an opaque numpy failure instead). | ||
| """ | ||
| from zagg.sweep_overview import decode_digest, fold_dense, fold_digests | ||
| from zagg.sweep_overview import decode_digest, fold_dense, fold_digests, overview_fold_delta | ||
|
|
||
| cell_order = int(cell_order) | ||
| fields = composable_fields(fields) | ||
|
|
@@ -192,7 +192,7 @@ def fold_column(slabs: dict, fields: dict, *, cell_order: int, resolutions: list | |
| else: | ||
| dtype = meta.get("dtype") or "float32" | ||
| inner = tuple(meta.get("inner_shape") or (2,)) | ||
| delta = int(meta.get("delta") or 512) | ||
| delta = overview_fold_delta(meta) | ||
| if slab.shape[0] % factor: | ||
| raise ValueError( | ||
| f"cannot fold {slab.shape[0]} cells {factor}-to-one for {name!r}" | ||
|
|
@@ -223,11 +223,14 @@ def _column_provenance(meta: dict) -> dict: | |
| shared helper — the overview's identical gap is a spec call for the | ||
| issue #383 phase 4 section, not a reason to leave this artifact short. | ||
| """ | ||
| from zagg.sweep_overview import _field_provenance | ||
| from zagg.sweep_overview import _field_provenance, overview_fold_delta | ||
|
|
||
| entry = dict(_field_provenance(meta)) | ||
| if meta.get("class") == "approximate": | ||
| entry["delta"] = int(meta.get("delta") or 512) | ||
| # The budget the column fold actually compressed at (issue #424): | ||
| # the split overview_delta, not the leaf δ. | ||
| entry["overview_delta"] = overview_fold_delta(meta) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude (review) A new key in the versioned This line makes every column written from here on carry
Suggested fix: extend §4.6's example + prose with
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 from Claude Fixed in 7b2cfac — spec and fixture both updated, per the #340 same-PR rule.
Test:
|
||
| entry["dtype"] = meta.get("dtype") or "float32" | ||
| entry["inner_shape"] = list(meta.get("inner_shape") or (2,)) | ||
| return entry | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 from Claude (review)
§2.0 ships a normative reader MUST that zagg's own reader does not honor.
grids.base.weights_declarationimplements exactly this sentence, but nothing on the read path calls it.readers/tdigest_tensor._open_ragged(src/zagg/readers/tdigest_tensor.py:263-300) strict-checks theraggedblock'sspec, elementdtypeand shape — the precedent this sentence is modeled on — and never looks atweights. So on this branch:weights: "photons") decodes silently throughread_tensors, which is precisely what "MUST be refused, never read as either defined value" forbids;weights: "flux"payload is rasterized as if its weight column were counts, anddtype="uint32"tensors additionally truncate the fractional weights.The reader is the party this MUST is addressed to, and the check is two lines next to the existing spec gate:
If binding flux semantics in the reader is deliberately #426's P5 read-validation phase, that is fine — but then either narrow the sentence (e.g. "a reader that binds the weight column MUST …") or record the deferral under "Questions for review", so the spec is not shipping a MUST ahead of the only in-tree reader. As written,
tests/test_spec_conformance.pypins the fixture's attrs but no test asserts that a reader refuses an unknown declaration off a store.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 from Claude
Fixed in 90b3135 — the reader honors the MUST, so the §2.0 sentence stands as written rather than being narrowed.
readers/tdigest_tensor._open_raggednow callsweights_declaration(dict(arr.attrs))right beside the existingraggedspec gate, so an undefined value raises at open for every reader that goes through it (read_tensors,read_cell,read_raw_values,read_locations). Both defined values still open: what a reader BINDS of flux semantics is issue #426's read-validation phase, and this only refuses the undefined ones — which is exactly the sentence's scope ("never read as either defined value").On the uint32 truncation: recorded as a
dtypedocstring note inread_tensorsrather than a behavior change — a flux field's weights are positive reals, so an integer dtype rounds each bin's photoelectron estimate away andfloat32is the right choice; theRaisessection names the new refusal too. Binding flux at rasterization stays #426.Tests in
TestReadTensors:test_unknown_weights_declaration_raises(aweights: "photons"payload refused through bothread_tensorsandread_cell) andtest_defined_weights_declarations_open(countsandfluxboth open and yield).