Skip to content

Inference performance: vectorized energy detection, fused per-channel workers, per-cadence streaming, downsample-at-extraction#120

Open
zachtheyek wants to merge 6 commits into
feature/test-suitefrom
feature/inference-energy-detection-perf
Open

Inference performance: vectorized energy detection, fused per-channel workers, per-cadence streaming, downsample-at-extraction#120
zachtheyek wants to merge 6 commits into
feature/test-suitefrom
feature/inference-energy-detection-perf

Conversation

@zachtheyek

Copy link
Copy Markdown
Owner

Closes #115
Stacked on #116 (feature/test-suite) — this PR's base is feature/test-suite; review/merge #116 first. Note: #117 (the parallel train-chain head) shares the config/cli/README-CLI-regen surface with this PR; cross-branch conflicts there are expected to be resolved at rebase time by the coordinator.

Summary

CSV inference was CPU-bound at 2 h 13 m for the 2-cadence subset (GPUs busy ~32 s total). This PR overhauls the four dominant costs. Measured result on the same subset, same config, same hardware: 21 m 30 s end to end (~6.2x), with energy detection down from ~45 min/cadence to ~2 min/cadence and stamp storage cut exactly 8x.

1. Vectorized energy detection (_sliding_normality_k2)

Replaces the per-window Python loop over scipy.stats.normaltest (~8190 windows/channel x 3 ON files x all coarse channels — ~45 min/cadence) with a closed-form vectorized derivation: per-block power sums S1..S4 in float64 -> moving window sums -> central moments -> scipy's exact skewtest/kurtosistest Z transforms transcribed elementwise -> k2 = Z1^2 + Z2^2; chi2.sf runs only on the hit subset. Step-sized blocks when step divides window (the default config), gcd-sized blocks via np.add.reduceat otherwise. Zero-variance windows yield NaN (never hits), matching scipy.

Correctness gates (committed unit tests):

  • k2 matches scipy.stats.normaltest(window.flatten()) to rtol=1e-9 per window across 6 distributions/scales x 4 window/step geometries (measured max rel err ~7e-11; most cases < 1e-13).
  • Identical hit sets on recorded real data: committed fixture tests/fixtures/ed_real_slice.npz (16 x 16384 float32 slice of coarse channel 469 of a GBT DDO210 observation, centered on the channel's DC spike, RFI-dense; bitshuffle-decoded on blpc3). After identical DC-spike removal + spline flattening, vectorized vs the historical scipy loop: identical hit sets, statistic max rel err 9.7e-16.

2. Fused per-channel workers + one persistent pool

One task per coarse channel (read own h5 slice -> DC-spike removal -> pluggable bandpass flatten (spline now; PFB comes in PR-07) -> vectorized threshold -> small hit list) on a single persistent pool per run, which stamp extraction reuses. Replaces 3 pools + 2 block-sized SHM copies per 28-channel block (x 18 blocks x 3 ON files per cadence). The DC spike sits at the channel center and its interpolation offsets reach at most +/-3 bins, so per-channel processing touches exactly the bins the block path did (verified; noted in code). parallel_coarse_chans is now a progress-logging chunk knob only (default None -> manager.n_processes).

Behavior note: every complete coarse channel is now processed — the old block path floored to a block multiple and silently dropped coarse channels 504-511 of the 512-channel subset files (see the +54/+27 extra stamps below).

3. Downsample-at-extraction

_extract_stamps_worker downsamples stamps along frequency before writing (downscale_local_mean — the same op the loader applied afterward, so end-to-end values are identical): storage / 8, and the separate ~24 min downsample pass disappears (loading now only log-norms, vectorized in the pool workers via the new _lognorm_worker, resolving the old "downsample & log-norm simultaneously" TODOs). Metadata JSON gains stored_width / downsample_factor_applied; load_inference_data branches on the stored .npy width so legacy full-width files (e.g. the July subset_test_*.npy on blpc3) still load through the historical path. New store_downsampled_stamps config/flag (default on).

4. Per-cadence streaming inference

inference_command now drives a per-cadence loop: models load once (InferencePipeline), a prefetch-depth-1 background thread preprocesses cadence i+1 (the persistent pool's child processes do the work) while the main thread loads/encodes/classifies/writes cadence i. Peak memory = one cadence's stamps, independent of catalog size. Per-cadence provenance (target / session / band / cadence_id / timestamp_observed / h5_path, plus per-stamp frequency_mhz from the metadata's stamp_frequencies_mhz) is now populated end to end via the new derive_cadence_provenance — resolving the TODO cluster at inference.py:226-247 / main.py:397-399. The legacy --test-files path and the coarse retry loop are unchanged (per-cadence .npy resume still works; PR-08 owns the manifest overhaul).

Bug fixes along the way:

  • Tail-drop bug: prepare_distributed_inf_dataset computed inf_steps = n // global_batch with drop_remainder=True — the final partial batch was silently never processed (the July subset run dropped 2,429 of 94,589 stamps; with per-cadence batches, a cadence smaller than one global batch would process nothing). Now pads with duplicate rows to the next batch multiple (train.py's viz-dataset pattern) and truncates the encoded latents back to the real count. Regression unit tests added. This run processed all 94,670 snippets (padding 73,001->81,920 and 21,669->30,720).
  • _write_inference_results stored a single 8-dim latent row picked by snippet index (wrong row, wrong shape) as a candidate's latent vector; it now stores the snippet's full (num_observations * latent_dim) vector, matching the DB contract.

Config / CLI

  • inference.parallel_coarse_chans: default 28 -> None (= use manager.n_processes); progress-logging chunk only.
  • New inference.store_downsampled_stamps (default True) + --store-downsampled-stamps/--no-store-downsampled-stamps.
  • New validation: stamp_width % downsample_factor == 0 when downsampled storage is active with --inference-files.
  • Config.to_dict() updated; README CLI Reference regenerated with PYTHONPATH=src python utils/print_cli_help.py all.

Verification

Unit (blpc3, NGC 25.02 container, CUDA_VISIBLE_DEVICES="", branch @ 03e5093)

$ python -m pytest -m "not gpu and not cluster" -q
237 passed, 2 deselected

(2 deselected = the gpu/cluster integration smokes.) pre-commit run --all-files passes. The suite was run in the container because the local dev box's x86_64 TF wheel aborts at import under Rosetta (cpu_feature_guard CheckFeatureOrDie) — the numeric equivalence checks were additionally exercised locally with a TF module stub.

Cluster smoke (blpc3, subset_test.csv, dummy test_v17 model, 5 GPUs, --max-retries 1, fresh --preprocess-output-dir so energy detection ran from scratch; tag 20260711_172857)

Stage July 2026 baseline This PR (measured)
Energy detection, DDO210 (512 coarse ch x 3 ON files) ~45 min 1 m 51 s
Energy detection, NGC7454 ~45 min 2 m 16 s (fully overlapped with DDO210's load/encode)
Stamp extraction + write, DDO210 (73,001 stamps) (full-width writes, ~114.7 GB) 12 m 00 s (downsampled writes, 14.4 GB)
Stamp extraction + write, NGC7454 (21,669 stamps) (34.0 GB) 3 m 58 s (4.3 GB)
Downsample pass at load ~24 min / 94,589 stamps eliminated (log-norm-only load: 73 s + 31 s)
GPU encode + RF + DB write ~32 s (single giant batch) 30 s + 25 s (per cadence)
Total wall (launch -> clean teardown) 2 h 13 m 21 m 30 s (~6.2x)

Pipelining evidence from the log timeline: NGC7454's energy detection started at 17:43:10.71 — the same second DDO210's load began on the main thread — and its third ON-source file was being processed (17:44:34) while DDO210's GPU encode ran (17:44:27 -> 17:44:52; GPU util peaked at 63%). Resource-monitor DB for the run: system CPU averaged ~34% during ED (bursting to 100%) and ~49% during extraction — with the per-window scipy cost gone, ED is now bounded by /datag reads + bitshuffle decode rather than CPU, which is the "minutes, I/O + spline bound" regime the plan predicted (the "CPU near-saturated" expectation is superseded by the bottleneck moving to I/O).

Hit-set / artifact comparison vs the July 2026 baseline artifacts (identical config: verified config_test_v17.json carries the same window/step/threshold/spline/stamp/overlap parameters)

  • DDO210: all 72,947 July stamp starts reproduced exactly (identical list order); statistics max rel diff 1.96e-11 (float64 rounding differences of the closed form vs scipy's implementation — hit sets are what must and do match). +54 stamps beyond the old coverage boundary (the coarse channels 504-511 the block path never processed).
  • NGC7454: all 21,642 identical; statistics max rel diff 5.86e-12; +27 beyond the boundary.
  • Storage: per-stamp ratio exactly 8.00x (114,735,710,336 -> 14,352,580,736 bytes; 34,039,922,816 -> 4,260,298,880 bytes); stored_width=512, downsample_factor_applied=8 in metadata.
  • End-to-end candidates: 0 in both runs (dummy test_v17 RF at threshold 0.99 produced no positives in July either — the inference_results table has no rows for any prior tag), so the candidate sets are trivially equal.
  • Cluster hygiene: /dev/shm empty post-run, benchmark preprocess dir + temp logs removed, cluster repo restored to master.

Deferred validation

  • bla0: nothing to verify there — CSV-inference raw data lives under /datag (blpc3-only).
  • Candidate-set equality on a model that actually produces positives: the dummy test_v17 model yields zero candidates at threshold 0.99, so the "equal candidate sets" check reduces to 0 == 0 here; the stamp-level exactness above (identical inputs to the encoder, bit-identical downsample math) is the substantive equivalence evidence.

AI assistance disclosure

Implemented with Claude Code (Anthropic). Extent: implementation, unit tests (including the recorded real-data fixture), documentation/README regeneration, multi-angle code review, and local + cluster verification runs.

🤖 Generated with Claude Code

zachtheyek and others added 4 commits July 12, 2026 06:28
…extraction

Replace _threshold_hits_worker's per-window Python loop over
scipy.stats.normaltest (~8190 windows/channel x 3 ON files x all coarse
channels — the #1 inference cost at ~45 min/cadence) with a closed-form
vectorized derivation (_sliding_normality_k2): per-block power sums
S1..S4 in float64 -> moving window sums -> central moments -> scipy's
exact skewtest/kurtosistest Z transforms transcribed elementwise ->
k2 = Z1^2 + Z2^2, with chi2.sf evaluated only on the hit subset.
Fast path uses step-sized blocks when step divides window; general
window/step ratios fall back to gcd-sized blocks via np.add.reduceat.
Zero-variance windows yield NaN (never hits), matching scipy.

Replace the block-based read/bandpass/threshold stages (3 pools +
2 block-sized SHM copies per 28-channel block) with one fused task per
coarse channel (_energy_detect_channel_worker: read -> DC-spike removal
-> bandpass flatten -> vectorized threshold -> small hit list) running
on a single persistent pool per run (start/stop_energy_detection_pool),
which the stamp-extraction stage now reuses. The DC spike sits at the
channel center with interpolation offsets of at most +/-3 bins, so
per-channel processing touches exactly the bins the block path did.
Bandpass flattening is now a pluggable picklable callable
(_get_bandpass_flattener; spline implementation today, PFB planned).
Every complete coarse channel is processed — the block path silently
dropped up to parallel_coarse_chans - 1 trailing channels when n_chans
was not an exact block multiple.

Downsample-at-extraction: _extract_stamps_worker now downsamples stamps
along frequency (downscale_local_mean by data.downsample_factor) before
writing, cutting per-cadence .npy storage ~8x and removing the separate
downsample pass at load time; metadata JSON gains stored_width /
downsample_factor_applied. load_inference_data branches on the stored
width: already-downsampled files get log-norm only (vectorized in the
workers via the new _lognorm_worker, resolving the old combined
downsample+log-norm TODO); legacy full-width files keep the historical
downsample-then-log-norm path. New config/flag store_downsampled_stamps
(default on) with a stamp_width %% downsample_factor validation.

parallel_coarse_chans becomes a progress-logging chunk knob only
(default None -> use manager.n_processes); README CLI reference
regenerated. find_hits() is now a thin wrapper over plan_cadences() +
process_pending_cadence(), which the streaming inference loop drives
directly.

Part of #115

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…p bug

Restructure inference_command around a per-cadence streaming loop
(_run_streaming_csv_inference): models load once into a single
InferencePipeline; a prefetch-depth-1 background thread preprocesses
cadence i+1 (the persistent pool's child processes do the real work)
while the main thread loads cadence i's stamps, encodes on the GPUs,
runs the RF, and writes per-cadence results. Peak memory is now one
cadence's stamps plus the in-flight preprocessing state — independent
of catalog size — where the old flow preprocessed and loaded the whole
catalog into a single array. The legacy --test-files path keeps its
single-shot run_inference_pipeline flow, and the coarse retry loop is
unchanged (per-cadence .npy resume already works).

Fix the tail-drop bug in prepare_distributed_inf_dataset: inf_steps was
n // global_batch with drop_remainder=True and no padding, so the final
partial batch was silently never processed (and a cadence smaller than
one global batch would process nothing at all). The data is now padded
with duplicate rows to the next global-batch multiple (mirroring
train.py's viz-dataset pattern) and the encoded latents are truncated
back to the real sample count.

Populate observational provenance end to end: derive_cadence_provenance
maps each cadence's group key + metadata JSON onto target / session /
band / cadence_id / timestamp_observed (header tstart) / h5_path, and
per-stamp frequency_mhz now comes from the metadata's
stamp_frequencies_mhz (resolving the TODO cluster at inference.py and
main.py). _write_inference_results also now stores the full
(num_observations * latent_dim) latent vector for a candidate snippet —
it previously stored a single observation row picked from the wrong
index. The per-call tf.function trace and TF session clearing are
hoisted so repeated run_inference calls on one pipeline don't
accumulate state.

Part of #115

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- _sliding_normality_k2 equivalence gates: k2 matches
  scipy.stats.normaltest per window to rtol=1e-9 across six
  distributions/scales and four window/step geometries (fast + gcd
  paths), plus zero-variance NaN, minimum-sample, and window-count
  parity checks.
- Real-data gate: committed fixture tests/fixtures/ed_real_slice.npz
  (16 x 16384 float32 slice of coarse channel 469 of a GBT DDO210
  observation, centered on the DC spike, bitshuffle-decoded on blpc3)
  must produce identical hit sets and rtol=1e-9 statistics between the
  vectorized path and the historical scipy loop.
- Fused _energy_detect_channel_worker vs an explicitly composed
  read -> DC spike -> spline flatten -> scipy threshold pipeline;
  absolute index offsets.
- derive_cadence_provenance mapping (case-insensitive columns,
  unparseable cadence ids, sparse metadata).
- _extract_stamps_worker downsample-at-extraction vs
  downscale_local_mean, and the factor-1 raw path.
- _process_cadence end to end on synthetic .h5 cadences (sequential):
  stamp shapes, metadata fields, resume path, and the
  store_downsampled_stamps=False full-width layout.
- load_inference_data branch tests: downsampled log-norm-only path,
  legacy downsample+log-norm path, mixed files, unrecognized widths.
- prepare_distributed_inf_dataset tail-padding regression tests: the
  padded tail is processed (old code dropped it; a cadence smaller than
  one global batch processed nothing) and order/padding are
  deterministic.

Part of #115

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…copies

- Streaming inference exits with an error when no cadence produced a
  stamp .npy (preserving the historical find_hits contract) instead of
  reporting an empty success.
- Skip the redundant float32 copy of each cadence's loaded stamps
  (astype copy=False) and the double float64 allocation in
  _sliding_normality_k2 (astype + in-place mean subtraction).

Part of #115

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Claude finished @zachtheyek's task in 2m 23s —— View job


Code Review: Inference Performance PR

  • Read and analyze changed files
  • Review vectorized energy detection (preprocessing.py)
  • Review fused per-channel workers and streaming inference (main.py, inference.py)
  • Review config/CLI changes
  • Review test coverage
  • Post detailed review feedback

Summary

This is a well-executed performance overhaul — 6.2x end-to-end speedup with strong correctness evidence. The architecture is sound: fusing per-channel work into single tasks eliminates pool/SHM churn, the vectorized energy detection is mathematically rigorous with excellent unit-test gates, and per-cadence streaming bounds memory independent of catalog size. The tail-drop bug fix is an important correctness win.


Strengths

  1. Vectorized _sliding_normality_k2 — The closed-form derivation is carefully documented, handles edge cases (zero variance → NaN, < 8 samples → raise), and the unit tests pin equivalence to scipy at rtol=1e-9 across 24 distribution×geometry combinations plus a recorded real-data fixture. This is exemplary numerical code.

  2. Tail-drop bug fix — The padding approach in prepare_distributed_inf_dataset correctly mirrors the train-side pattern and the regression tests cover the critical edge cases (partial tail, cadence smaller than one batch, exact multiple, zero).

  3. Atomicity.tmp.npyos.replace for both stamps and metadata prevents the resume path from seeing partial writes.

  4. Test coverage — 483 new lines in test_preprocessing.py + 115 in test_inference.py covering dedup, CSV grouping, filename sanitization, JSON coercion, DC-spike removal, spline fitting, vectorized k2, fused worker, downsample-at-extraction, provenance derivation, load paths, and the real-data fixture. Very thorough.


Issues & Suggestions

Medium Priority

  1. sys.exit(1) inside _run_streaming_csv_inference (main.py:323) — This bypasses the retry loop in inference_command. An empty catalog is a config error, not transient, so it's defensible — but it breaks the convention that helper functions raise and callers decide exit behavior. See inline comment.

  2. Prefetch exception propagation is one iteration late (main.py:338-345) — If process_pending_cadence(units[i+1]) raises inside the prefetch thread, the exception surfaces on the next iteration's future.result(). This is fine (the retry loop catches it), but deserves a brief comment so future readers aren't surprised.

  3. Stale .tmp.npy from killed processes — If a previous run was SIGKILL'd mid-extraction, the .tmp.npy persists. On retry, _process_cadence will overwrite it (the open_memmap(..., mode="w+") creates fresh), so it's not a correctness bug — but if the canonical .npy doesn't exist and the .tmp.npy does, a user might be confused. Consider logging a warning or deleting stale .tmp.npy at the start of extraction.

Low Priority / Nits

  1. Power-sum loop (preprocessing.py:287-296) — The row-by-row Python loop is correct and keeps peak memory at (width,), but for future instruments with larger time_bins it could be vectorized with data.sum(axis=0), (data**2).sum(axis=0), etc. at ~2 MB peak for the current 16×16384 case. Not blocking.

  2. Padding copy in prepare_distributed_inf_dataset (inference.py:93) — np.concatenate([data, data[pad_indices]]) momentarily doubles memory for the cadence's stamps. Bounded by the per-cadence streaming design, but worth a comment for future reference.

  3. _extract_stamps_worker uses np.lib.format.open_memmap — This is a private/semi-public API that could change across numpy versions. The repo is pinned to a specific numpy range so this is safe today, but worth noting if a numpy bump is planned.


Security

  • Filename sanitization in _cadence_npy_filename uses a strict allowlist regex ([^\w\-.]+_), which prevents path traversal, shell injection, and control chars. Good.
  • No secrets in logs, no user-controlled format strings.
  • h5py file handles are opened per-worker (fork-safe), and pool workers ignore SIGINT correctly.

Verdict

Approve with minor suggestions. The correctness evidence is strong (identical hit sets on real data, rtol=1e-9 numeric gates, regression tests for the tail-drop fix), the performance win is substantial and well-measured, and the code quality is high. The medium-priority items above are non-blocking — they're about defensive robustness and code convention rather than correctness. Nice work.

@claude claude Bot added the needs-issue PR is missing a linked issue label Jul 12, 2026
@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Contribution compliance note: The Closes #115 keyword in the PR body isn't creating a formal issue link (verified via GitHub's GraphQL API), likely because this PR targets the feature/test-suite branch rather than the default branch — GitHub only registers closing references for PRs targeting the default branch. Once #116 merges and this PR is rebased onto main/master, the link should activate automatically. Labeling needs-issue as a tracking reminder; feel free to remove it once the formal link is confirmed after retargeting. See CONTRIBUTING.md for linking guidelines.

Comment on lines +287 to +291
p1 = np.zeros(width, dtype=np.float64)
p2 = np.zeros(width, dtype=np.float64)
p3 = np.zeros(width, dtype=np.float64)
p4 = np.zeros(width, dtype=np.float64)
for t in range(time_bins):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit (performance): The explicit Python for t in range(time_bins) loop over rows, while keeping temporaries at (width,) shape, may become the bottleneck for large time_bins. Consider vectorizing this with np.einsum or direct array ops:

p1 = data.sum(axis=0)
p2 = (data**2).sum(axis=0)
p3 = (data**3).sum(axis=0)
p4 = (data**4).sum(axis=0)

The peak memory would be (time_bins, width) per power (one at a time if computed sequentially), which for the typical 16×16384 slice is ~2 MB — negligible. The current row-by-row loop is correct and well-commented, so this is purely an optimization suggestion for a future pass if time_bins grows significantly (e.g., different instruments).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Added a NOTE in 61f8528: the row loop is deliberate — it caps peak memory at a few (width,) float64 vectors per worker (32 workers x 1M-bin channels). At the default time_bins=16 the loop overhead is noise next to the h5 read + spline fit; the NOTE points to the (channel**k).sum(axis=0) vectorization if time_bins ever grows.

Comment thread src/aetherscan/main.py
Comment on lines +338 to +341
future = prefetch.submit(preprocessor.process_pending_cadence, units[0])

for i, unit in enumerate(units):
cadence_result = future.result()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Potential issue — future submission after exception: If future.result() on line 341 raises (e.g. the prefetch thread hit an unrecoverable error), the exception propagates up, but the previous future submitted on line 345 (from iteration i-1) is still running in the background. The ThreadPoolExecutor context manager will wait for it to finish on exit, but any exception from that abandoned future is silently swallowed.

More importantly, the prefetch.submit(...) on line 338 fires the first unit's preprocessing. If units[0]'s preprocessing itself raises, future.result() on line 341 will raise inside the for-loop, and the code correctly skips to the finally block. That's fine.

But consider: if units[i+1]'s prefetch raises inside the thread (line 345), the exception won't surface until the next iteration's future.result(). At that point cadence_result would be the exception, not None — it would raise there. This is acceptable behavior (the retry loop catches it), but worth a brief comment noting that prefetch exceptions propagate one iteration late.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Added a comment in 61f8528 clarifying the timing: a prefetched task's exception surfaces one iteration later, when its future.result() is resolved, and then propagates to the retry loop. In practice process_pending_cadence swallows per-cadence failures (returns None), so only infrastructure-level errors (e.g. a broken worker pool) raise here. The with ThreadPoolExecutor exit also waits for any in-flight task before the pool is torn down in finally, so there's no submission-after-shutdown window.

Comment on lines +446 to 447
) = args
out = np.lib.format.open_memmap(npy_path, mode="r+")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Robustness: np.lib.format.open_memmap(npy_path, mode="r+") will raise if npy_path doesn't exist or is corrupted (e.g. a partial write from a killed previous run). Since this worker is invoked from a pool, the exception propagates as a task failure, which is correct — but it may produce a confusing traceback when the .tmp.npy was partially written due to a prior crash. The atomicity guarantee (.tmp.npyos.replace → canonical name) protects the canonical path, but the .tmp.npy itself can be left behind by a killed process. Consider adding a note that the caller should remove stale .tmp.npy files before re-dispatching extraction tasks (or do so in _process_cadence before writing the memmap header).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Addressed in 61f8528: _process_cadence now detects a stale .tmp.npy left by an interrupted run (e.g. SIGKILL between memmap creation and os.replace), logs a warning, and removes it before extraction. Note the worker's open_memmap(..., mode="r+") can only ever see the freshly created tmp file (the parent creates it with w+ immediately before dispatching tasks), so the stale-file risk lived at creation time, not in the workers. The canonical npy_path is only ever produced by os.replace after all workers finish, so a partial canonical file can't exist.

if n_padded > n_samples:
pad_count = n_padded - n_samples
pad_indices = np.arange(pad_count) % n_samples
inf_data = np.concatenate([data, data[pad_indices]], axis=0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor: np.concatenate([data, data[pad_indices]], axis=0) creates a full copy of the original data plus padding. For a large cadence (e.g. 73k stamps × 6 × 16 × 512 × 4 bytes ≈ 14 GB), this doubles peak memory momentarily. Since the streaming path only ever holds one cadence's worth of stamps, this is bounded, but worth noting for future reference that a np.pad-style approach or a custom generator that cycles would avoid the copy.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Added a NOTE in 61f8528: the concatenate does briefly double the cadence's footprint (~4.6 GB peak for the 73k-stamp DDO210 cadence — fine at per-cadence scale, and far below the old whole-catalog behavior). If this ever matters, the alternative is yielding pad rows from the generator instead of materializing them; deferred until profiling shows a need.

Comment thread src/aetherscan/main.py
Comment on lines +322 to +323
units = preprocessor.plan_cadences()
if not units:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Design: sys.exit(1) inside a helper function (_run_streaming_csv_inference) called from within the retry loop in inference_command means the retry logic never gets a chance to handle the "no cadence work units" case. This is intentional (an empty catalog is a configuration error, not a transient failure), but it breaks the convention that helpers raise and callers decide exit behavior. Consider raising a dedicated exception (or ValueError) and letting inference_command decide to sys.exit — consistent with how preprocessor.load_inference_data() raises ValueError("No data loaded successfully") in the legacy path.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 61f8528: both permanent failure modes (empty catalog, zero cadences producing stamps) now raise NonRetryableInferenceError and the helper never exits the process itself. inference_command's retry loop catches that exception type before the generic except Exception and fails fast (no retry attempts burned), while transient failures keep the existing retry semantics. Regression tests added in tests/unit/test_main.py.

…otes

- _run_streaming_csv_inference now raises NonRetryableInferenceError
  (instead of calling sys.exit inside the helper) for the two permanent
  failure modes — empty catalog and zero cadences producing stamps; the
  retry loop in inference_command re-raises it immediately so permanent
  config errors don't burn retry attempts, while transient failures keep
  the existing retry semantics. Regression tests added
  (tests/unit/test_main.py).
- _process_cadence detects a stale .tmp.npy left by an interrupted
  previous run (e.g. SIGKILL between memmap creation and os.replace),
  logs a warning, and removes it before extraction; covered by the
  process-and-resume test.
- Clarifying comments: prefetch exceptions surface one iteration later
  via future.result() (and what actually raises given
  process_pending_cadence swallows per-cadence failures); the padding
  concatenate briefly doubles a cadence's memory footprint; the
  power-sum row loop is deliberate for peak-memory reasons;
  np.lib.format.open_memmap is a semi-public numpy API.

Part of #115

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@zachtheyek

Copy link
Copy Markdown
Owner Author

All review feedback addressed in 61f8528:

  • Medium — sys.exit in helper: _run_streaming_csv_inference now raises a dedicated NonRetryableInferenceError for its two permanent failure modes (empty catalog; zero cadences producing stamps). The retry loop catches it ahead of the generic handler and fails fast without burning retry attempts; transient failures keep the existing retry semantics. Regression tests in tests/unit/test_main.py.
  • Medium — prefetch exception timing: comment added at the future.result() site explaining the one-iteration-later surfacing and what can actually raise there.
  • Medium — stale partial outputs: _process_cadence now warns about and removes a stale .tmp.npy left by an interrupted previous run before extraction starts; covered in the process-and-resume test.
  • Nits: NOTEs added for the padding memory doubling, the deliberate power-sum row loop (peak-memory trade-off + vectorization pointer), and the np.lib.format.open_memmap semi-public API dependency.

Verification: full default suite re-run in the NGC container on blpc3 at 61f8528239 passed, 2 deselected (includes the 2 new regression tests); pre-commit run --all-files passes.

@zachtheyek

Copy link
Copy Markdown
Owner Author

@claude please perform a second-pass code review of this PR. Re-review the current diff for any remaining correctness bugs, security issues, or significant quality problems, and post your findings. If the diff is clean, please say so explicitly. (Final verification round before merge.)

Comment thread src/aetherscan/preprocessing.py Outdated
Comment on lines +306 to +311
edges = np.arange(0, width, block)
blocks_per_window = window_size // block
blocks_per_step = step_size // block

def _window_sums(col_sums: np.ndarray) -> np.ndarray:
block_sums = np.add.reduceat(col_sums, edges)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Low (latent edge case): If width % block != 0, np.add.reduceat produces a partial last block with fewer than block columns. Any sliding window that overlaps that partial block gets a short sum → incorrect k2 statistic (silently wrong, not a crash).

With default config (all powers of 2, coarse channel widths are powers of 2) this never triggers — the concern is a user-configured window_size/step_size whose gcd doesn't divide the coarse channel width.

Consider either asserting width % block == 0 (fail loud on unsupported geometry) or truncating col_sums to (width // block) * block before reduceat (silently exclude trailing columns that can't form a full window anyway — n_windows already excludes incomplete windows from the right edge).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 146845b, using the truncation option you suggested. _window_sums now slices the reduceat output down to the full blocks:

n_full_blocks = width // block
...
block_sums = np.add.reduceat(col_sums, edges)[:n_full_blocks]

width // block equals len(edges) when block divides width (so it's a no-op on power-of-2 geometries) and drops exactly the ragged short block otherwise. It's behavior-preserving even on the geometries that currently hit the short block — no in-range window ever reaches it, because every window ends at a multiple of block that is < width (hence <= edges[-1]), and n_windows already excludes the incomplete right-edge windows. The slice just makes that invariant explicit so a partial sum can never silently leak into a k2.

Added test_non_power_of_2_width_short_block_matches_scipy, parametrized over two non-power-of-2 widths — (1234, 100, 30) and (1000, 96, 36) — each of which assert-guards width % block != 0 (so the geometry can't be refactored into a no-short-block case and pass vacuously) and pins k2 to scipy.stats.normaltest at rtol=1e-9. Verified the pure-NumPy/SciPy math against scipy directly (the existing (100, 30)@4096 combo already exercised this path and still matches).

np.testing.assert_allclose(stat_val, expected[idx], rtol=1e-9)
np.testing.assert_allclose(pval, stats.chi2.sf(stat_val, 2), rtol=1e-9)

def test_absolute_indices_offset_by_channel_start(self, make_h5_observation):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit (test robustness): With stat_threshold=0.0, the all(s >= ch_start ...) assertion at the bottom passes vacuously if hits is empty (Python's all([])True). In practice chi-square noise at threshold 0 always produces hits, but the other tests in this class guard against this with an explicit assert len(hits) > 0 — adding one here would close the theoretical gap.

Not blocking, just for consistency with the pattern used a few lines above in test_fused_worker_matches_composed_pipeline.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Fixed in 146845b: added assert len(starts) > 0 right before the all(...) check, so the offset assertion can no longer pass vacuously on an empty hit list. This matches the assert len(hits) > 0 pattern already used a few lines above in test_fused_worker_matches_composed_pipeline.

@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Second-pass code review

Reviewed the full diff across all 10 changed files (preprocessing.py, inference.py, main.py, config.py, cli.py, and the three test files). Focused on correctness bugs, security issues, and significant quality problems.

Verdict: Clean — no blocking issues found.

The vectorized energy detection, fused per-channel workers, per-cadence streaming, downsample-at-extraction, and the tail-drop/latent-vector bug fixes are all correct. The numerical implementation in _sliding_normality_k2 faithfully transcribes scipy's skewtest/kurtosistest Z transforms; zero-variance and degenerate-window handling is sound (NaN propagation, m2 <= 0 guard). The RF classification pipeline is correctly wired end-to-end: predict_verbose internally calls prepare_latent_features which reshapes (n_samples * num_observations, latent_dim)(n_samples, num_observations * latent_dim), so predictions/confidences are per-snippet as _write_inference_results expects. The prefetch-depth-1 streaming pattern is thread-safe (futures synchronize, no shared mutable state between threads). Resource cleanup is properly guarded by try/finally in both find_hits and _run_streaming_csv_inference.

Low-severity findings (non-blocking, noted inline or below)

  1. Partial trailing block in _sliding_normality_k2 (inline comment on preprocessing.py:306-311): If coarse_channel_width % gcd(window_size, step_size) != 0, np.add.reduceat produces a short last block, silently corrupting k2 for any window that overlaps it. Default config (powers of 2) is safe. Consider an assertion or truncation guard for non-standard geometries.

  2. close() does not clean up _ed_pool (preprocessing.py:655): DataPreprocessor.close() doesn't call stop_energy_detection_pool(). Both current callers handle this via their own finally blocks, so this is defense-in-depth, not a live bug. Adding self.stop_energy_detection_pool() to close() would be a one-line safety net (it's a no-op when _ed_pool is None).

  3. Vacuous assertion guard in test (inline comment on test_preprocessing.py:703): test_absolute_indices_offset_by_channel_start uses all(...) on the hits list without an explicit assert len(starts) > 0all([]) is True in Python. Practically impossible to trigger (chi-square noise at threshold 0.0 always produces hits), but the other tests in the same class guard against this; adding one here would be consistent.

Items verified clean

  • Tail-drop fix: Padding logic in prepare_distributed_inf_dataset is correct (cyclic pad to next global-batch multiple, truncate latents back to n_samples * num_observations). Edge cases (cadence smaller than one batch, exact multiples) are covered by regression tests.
  • Latent vector fix: _write_inference_results correctly indexes latents[idx * num_observations : (idx + 1) * num_observations] per snippet, matching the (n_samples,) shape of predictions/confidence_scores.
  • Per-cadence streaming: ThreadPoolExecutor with depth-1 prefetch is correctly synchronized. Exceptions from the background thread propagate via future.result(). Orphaned prefetch on error is handled by the executor's __exit__.
  • Shared memory / pool safety: Fused workers eliminate SHM for energy detection entirely. Stamp extraction uses memmap with disjoint write regions. No shm.unlink() in workers. Pool lifecycle is idempotent (start is no-op if pool exists, stop sets _ed_pool = None).
  • Config / CLI: store_downsampled_stamps correctly wired (BooleanOptionalAction, three-way semantics, to_dict serialization). Validation stamp_width % downsample_factor == 0 is properly guarded.
  • Repo rules: from __future__ import annotations present in all modified modules; no bare print() outside utils; no logging in signal handlers; no bare mutable defaults; singleton accessors used throughout; model loading inside strategy.scope().
  • Security: No secrets in log messages, no command injection vectors, file paths are infrastructure-only.
  • Test quality: k2 equivalence tests are rigorous (6 distributions × 4 geometries, rtol=1e-9). Real-data fixture test guards against vacuousness (assert len(scipy_hits) > 0). Tail-drop regressions would genuinely fail under the old drop_remainder=True behavior.

🤖 Generated with Claude Code

…acuous test

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@zachtheyek

Copy link
Copy Markdown
Owner Author

Applied the three non-blocking hardening items from the review in 146845b:

  1. Partial trailing block in _sliding_normality_k2 (preprocessing.py) — when block doesn't divide the coarse channel width, np.add.reduceat emitted a short trailing block. Went with the truncation option: _window_sums now slices the reduceat output to width // block full blocks, dropping the ragged tail. Behavior-preserving on all currently-tested geometries (no in-range window ever reaches the short block — every window ends at a multiple of block that is < width <= edges[-1]), but it makes the invariant explicit so a partial sum can't silently corrupt a k2. Added test_non_power_of_2_width_short_block_matches_scipy, parametrized over two non-power-of-2 widths that guard width % block != 0 and pin k2 to scipy.stats.normaltest at rtol=1e-9.

  2. DataPreprocessor.close() didn't stop the ED pool (preprocessing.py) — close() now calls self.stop_energy_detection_pool() as a defense-in-depth net for callers that forget to tear the pool down. stop_energy_detection_pool() is idempotent (guards if self._ed_pool is not None), so it's a no-op when no pool was started.

  3. Vacuous assertion in test_absolute_indices_offset_by_channel_start (tests/unit/test_preprocessing.py) — added assert len(starts) > 0 before the all(...) offset check so it can't pass on an empty hit list, matching the pattern in test_fused_worker_matches_composed_pipeline.

Verification: the ED math (_sliding_normality_k2) is pure NumPy/SciPy — verified the edited function against scipy.stats.normaltest at rtol=1e-9 across the fast path, the existing short-block combos, and the two new non-power-of-2 geometries (all exact). ruff check / ruff format and pre-commit run --all-files pass. The full pytest collection pulls the heavy TF/h5py/setigen stack, which isn't available in this environment; the two package-dependent edits (items 2 and 3) are trivial and low-risk.

@zachtheyek

Copy link
Copy Markdown
Owner Author

@claude the finding from your previous review has been addressed in a new commit. Please re-review the current diff once more and confirm whether it is now clean, or report any remaining issues.

@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Re-review: all three findings addressed — diff is clean

Verified all three fixes from 146845b against the current diff:

1. Partial trailing block in _sliding_normality_k2Fixed

_window_sums now slices the np.add.reduceat output to width // block full blocks, discarding the ragged tail. New test test_non_power_of_2_width_short_block_matches_scipy covers two non-power-of-2 geometries where width % block != 0, pinned to scipy.stats.normaltest at rtol=1e-9.

2. DataPreprocessor.close() ED pool cleanup — Fixed

close() now calls self.stop_energy_detection_pool() as defense-in-depth. The method is idempotent (if self._ed_pool is not None), so double-calls are safe.

3. Vacuous assertion in test_absolute_indices_offset_by_channel_startFixed

assert len(starts) > 0 now precedes the all(...) check, preventing all([]) is True from masking a no-hits regression.

Final sweep

Re-reviewed the full diff (preprocessing.py, inference.py, main.py, config.py, cli.py, all test files). No new correctness bugs, security issues, or significant quality problems found. The padding/truncation logic, prefetch-depth-1 streaming, fused per-channel workers, downsample-at-extraction, dual-path loading, NonRetryableInferenceError retry semantics, and resource cleanup are all correct.

Verdict: clean — no remaining issues.

🤖 Generated with Claude Code

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

Labels

needs-issue PR is missing a linked issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant