PR #47 follow-ups: TEP `column: 0` divergence + `_read_group` / `_planned_read_group` dedup

[espg]

Summary

Tracking the two open decisions surfaced during PR #47's review folds. Both are non-blockers for the merge of #47 — the PR ships sane defaults — but each warrants a deliberate design call before going further. To be addressed in a follow-up PR once decisions land.

---

1. column: 0 vs .any(axis=1) semantic divergence (signal_conf_ph TEP filter)

Where it stands. Both shipped ATL03 templates (atl03.yaml, atl03_waveform_counts.yaml) carry {column: 0, op: ne, value: -2} against signal_conf_ph to drop TEP photons. The pre-merge _quality_mask did .any(axis=1) (drop iff -2 across every surface-type column); the post-merge _predicate_mask from #43 requires a column for N-D flags and so cannot express the old reduce-across-all behaviour. The two filters agree exactly when TEP is uniform across columns (which the ATL03 v3 data dictionary asserts), but diverge silently if any granule has signal_conf_ph[i, 0] == -2 with non--2 values in other columns.

Background: original analysis at #47 (comment).

Three options:

• (A) Keep column: 0 as shipped. Accept the divergence on the v3-dictionary guarantee; tighten the YAML comment to make the choice explicit. Zero code change.
• (B) Extend _predicate_mask to allow column: null on an N-D flag with a documented .any() reduction semantic. Recovers the prior behaviour exactly; ~5 LOC in src/zagg/processing.py, plus a TestPredicateMask case + a mention in the structured-filter docs. The ATL03 templates would then drop the column: 0 key.
• (C) Add an explicit reduce: any|all knob to the structured filter that, when set with column: null, applies a configurable reduction across the trailing axis. More general than (B), bigger surface, more docs.

Default position: (A) for now — matches the v3 dictionary and is what's shipped. (B) is the safest if TEP is ever partial-column on a future granule format. (C) is overkill for one use case.

---

2. Refactor: extract a shared post-read helper from _planned_read_group + _read_group_full

Where it stands. Phase 5's self-review (#47 (review)) flagged that src/zagg/processing.py is now ~1448 lines and _planned_read_group is largely a structural twin of _read_group_full — both follow the same template:

read coords -> spatial mask -> variables / filter datasets -> structured filters
            -> cross-level mask -> expression filters -> pandas / arrow output

The duplication landed because phase 5 was scoped to "wire the planning seam into the read path" without touching the existing flow. CLAUDE.md §4 targets ~1000 lines per module; we're 45 % over.

Proposed shape. A _apply_filters_and_build_output(...) helper takes:

• lats, lons, arrays_by_path (the just-read data),
• the data_source config (for coordinates, variables, filters),
• shard / grid context,
• optional global_idx + full-length cross_mask for the planned-path case,
• arrow flag.

Returns the same pandas.DataFrame | pyarrow.Table | None. The planned and full paths shrink to "do my IO + hand off."

Open questions:

• Where does the cross-level filter logic live — in the helper, or as a pre-computed mask passed in? (Phase 5 puts it on each path; a shared helper would naturally take the precomputed full-length cross_mask + global_idx.)
• Does this also subsume _read_group_full's [min_idx, max_idx] hyperslice optimization, or stay separate? They're structurally different (single contiguous slice vs. multi-slice list).
• Worth adding a _read_group_kernel benchmark to confirm the shared helper doesn't regress the full-read path.

Lower risk if landed as its own PR with a parity test against both the planned and full paths' outputs.

---

Phasing / dependencies

Independent. Either can be addressed first; (1) is smaller and might land first if @espg picks (B) or (C). (2) is the bigger cleanup and benefits from being its own self-contained PR.

Refs

• PR ATL03 template + region benchmark + hierarchical-read wiring (Refs #30, #43) #47 (ATL03 templates + region benchmark + Phase C wiring)
• Phase 4 column choice: ATL03 template + region benchmark + hierarchical-read wiring (Refs #30, #43) #47 (comment)
• Phase 5 module-size note: ATL03 template + region benchmark + hierarchical-read wiring (Refs #30, #43) #47 (review)
• Phase 5 fold-summary surfacing both: ATL03 template + region benchmark + hierarchical-read wiring (Refs #30, #43) #47 (comment)

[espg]

🤖 from Claude

Two genuinely independent decisions, so I'll take them separately. Both are small follow-ups; neither blocks the other.

1. column: 0 vs .any(axis=1) (TEP filter divergence)

The body lays out (A) keep column: 0, (B) extend _predicate_mask with column: null → .any(), (C) full reduce: any|all knob.

Recommendation: (B). The reasoning:

• (A) is the smallest diff but bakes in an undocumented invariant ("TEP is uniform across surface-type columns") into the YAML. The day it stops being true on a future ATL03 version we'll get a silent semantic regression — the kind that masks bad photons rather than failing loudly. Even if v3 guarantees uniformity today, that's not a contract zagg owns.
• (B) restores the prior behaviour exactly while adding only ~5 LoC + one TestPredicateMask case. column: null is a single new path through _predicate_mask; the schema impact is "this key is now optional on N-D flags."
• (C) generalises (B) by exposing reduce: any|all, but we don't have a second use case for all in any shipped template. CLAUDE.md §4 calls out speculative abstraction — better to wait for the second template that wants all before landing the knob.

Tradeoffs to call out before locking: (B) makes the YAML slightly more permissive (a missing column key now means "reduce") — that's a sharp edge for someone who meant to write column: 0 and forgot. The mitigation is to require column: null explicitly rather than letting it default to missing, so the choice is always written down. Worth confirming you want it explicit, not implicit.

Questions:

• Lock on (B) with explicit column: null required (no implicit default)?
• The .any() semantics need to be documented in docs/configs/filters.md (or wherever the structured-filter schema lives) alongside the change. Confirm that's in scope for the same PR.

2. Extracting _apply_filters_and_build_output(...) helper

processing.py is 1596 lines as of main (just confirmed locally — already 60% over the ~1000-line target, not the 45% in the body, so the pressure is real).

Recommendation: do the extraction, with the helper taking a precomputed cross_mask + global_idx rather than recomputing per path. That naturally consolidates the cross-level filter logic, and gives a single seam to write parity tests against (planned-path output vs full-path output on the same arrays).

On your three open questions:

• Cross-level filter location: pre-compute it before the helper, pass full-length cross_mask + global_idx in. Two reasons: (i) the planned path needs it materialized anyway, and (ii) it makes the helper's contract pure — same arrays in, same table out, no IO assumptions. The current per-path scatter is exactly the duplication we're trying to remove.
• Hyperslice optimization ([min_idx, max_idx]): keep it separate. It's structurally an IO concern (one contiguous read vs a multi-slice read), and folding it into a "build output" helper would smuggle IO awareness into the seam. The helper should start after the read.
• _read_group_kernel benchmark: worth it, but it can be a lightweight tests/test_processing_parity.py that asserts byte-identical outputs on synthetic arrays between planned and full paths — a parity test is the stronger guarantee than a microbenchmark for this kind of refactor. We can layer a benchmark on top later if perf regresses.

Tradeoff to flag: the helper signature carries a lot of context (arrays_by_path, data_source config, shard/grid context, global_idx, cross_mask, arrow flag). Six positional/named args is borderline; if it grows further during implementation, I'll likely package the IO outputs into a small ReadResult dataclass rather than letting the signature sprawl. Flag if you'd prefer the dataclass up front.

Suggested phasing

• (1) lands first as a tiny PR — column: null + reduce semantic + doc + one test. ~30 LoC.
• (2) lands as its own PR with a parity test; bigger but contained. Targets bringing processing.py back under or near 1000 lines.

If the recommendations above hold, I can open the PR on (1) right away (claude/61-predicate-mask-column-null); (2) makes sense to stage after — its parity test exercises filter behaviour shipped by (1).

---

Generated by Claude Code

[espg]

This is an old issue, and there'd by 40+ new (issues + PRs) since it was posted. What's the status of this? is it still relevant for our codebase, or did we fix this in subsequent PR's already?

[espg]

🤖 from Claude

Investigated the current codebase. Status of the two follow-ups this issue tracks:

Item (1) — TEP column: 0 vs .any(axis=1) divergence: STILL LIVE, unchanged.

• The shipped templates still use {column: 0, op: ne, value: -2}. Every ATL03 config carries it: src/zagg/configs/atl03.yaml (lines 33-36), atl03_gain_bias_healpix.yaml (43-44), atl03_tdigest_healpix.yaml (33-34), atl03_waveform_chunk.yaml (48-49), atl03_waveform_counts.yaml (39-40).
• _predicate_mask (now at src/zagg/processing/read.py:215) still requires an integer column for N-D arrays and rejects column on 1-D arrays (lines 223-229). There is no column: null / .any() reduction and no reduce: any|all knob — so options (B)/(C) remain unimplemented. The current code is option (A) by default.
• Whether this matters is the same as when filed: the atl03.yaml header (lines 11-13) asserts TEP (-2) is flagged uniformly across all 5 surface-type columns, so column: 0 is operationally equivalent to .any(axis=1) for the TEP drop specifically. The divergence only bites if someone writes a non-TEP signal_conf_ph predicate (e.g. "keep conf>=3 in any surface type"), which no shipped config does.

Item (2) — _planned_read_group / _read_group_full dedup + ~1000-line module: module-size concern RESOLVED; the duplication itself REMAINS.

• The processing.py monolith is gone — split into the src/zagg/processing/ package (worker.py 426, read.py 735, aggregate.py 868, write.py 462, __init__.py 122 lines). Every module is now under the §4 ~1000-line target, so the size motivation is obsolete.
• However the two functions are still structural twins. _planned_read_group (read.py:267) and _read_group_full (read.py:559) each independently re-implement the same five blocks: filter partition (base/coarse/expression), ANDed _predicate_mask keep-mask, coarse-level expand-to-base, data-dict build, expression-filter loop, and arrow-or-DataFrame return. No _apply_filters_and_build_output helper was ever extracted (grep across src/ + tests/ finds zero references). They diverge only in how they read base-rate data (planned hyperslices vs full-coord read + [min_idx:max_idx] slice).

Recommendation: The original framing of (2) — "split the giant module" — is obsolete; the package split already did that. What's left is an optional readability refactor (extract the shared post-read filter/build tail), which is lower priority and would need care so the two row-orderings stay parity-identical (the #43 Phase C invariant). Item (1) is still a live design decision.

To close this out, which would you prefer?

• (a) Close PR #47 follow-ups: TEP column: 0 divergence + _read_group / _planned_read_group dedup #61 as resolved-by-refactor and treat the remaining items as not worth a change (keep column: 0; leave the twin functions as-is).
• (b) Keep PR #47 follow-ups: TEP column: 0 divergence + _read_group / _planned_read_group dedup #61 open but narrowed to item (1) only — pick (A) keep column: 0, (B) support column: null + .any() reduction in _predicate_mask, or (C) add a reduce: any|all knob — and drop the now-obsolete module-split half.
• (c) Keep both, but re-scope item (2) from "split module" to "extract the shared filter/build tail" as a pure cleanup.

My lean is (b) with sub-option (A): the codebase already does (A) correctly for the only real use case, and adding (B)/(C) is speculative until a non-TEP signal_conf_ph predicate is actually needed.

---

Generated by Claude Code