From 8348ad313cf941d742f28bcce9b7f9f2e1e488e0 Mon Sep 17 00:00:00 2001 From: Thomas Ales Date: Sun, 9 Aug 2026 13:50:37 -0500 Subject: [PATCH] Implement FFT pad-factor caching and the stored-cache display fast path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tests/test_stored_cache.py exercised two features that were never built, so six of its tests had been failing on main. Both are now implemented. Pad-factor caching. A padded view could not use a stored FFT cache at all: the store was pad 1 by definition and cached_rf_image rejected any n_fft outright, so a user working at a pad factor got nothing from batch-computing a file. CACH tail v3 records the pad the images were resolved at, cache_file computes at a requested pad, and the batch actions pass the viewer's own pad down — while still refusing a store resolved at a different pad, since a padded FFT interpolates between the natural bins and so resolves genuinely different peak frequencies. v1/v2 tails read as pad 1 and keep working. Stored-cache dispatch. _refresh_display only ever consulted this window's in-session dicts, so after a batch every angle change still queued a worker and a progress popup for an image already on disk — the exact cost the batch was run to avoid. It now checks the file's own DC/FFT blocks first, asking with allow_dc_recompute=False so the GUI thread never touches I/O. When the stored cache genuinely cannot serve the view, the scan info panel says why rather than leaving the silent recompute a mystery. Two bugs surfaced on the way: - The angle spinbox was wired on editingFinished, which QAbstractSpinBox emits only on Return or focus-out — never on a step. Clicking its arrows, the ordinary way to walk a scan, moved the number and left the image behind. Now valueChanged with keyboard tracking off, which fires on a step and once on commit, but not per keystroke mid-typing. Every existing GUI test called _on_view_changed() by hand and so could not have caught this; two new tests pin it and fail against the old wiring. - A plain "fft" batch over a file previously cached with fft_rowavg carried the old row_avg_n forward, labelling raw images as row-averaged. It now writes row_avg_n=0 explicitly. Verified: 111 passed (was 103 passed / 6 failed), and tools/check_equivalence.py is byte-identical to the pre-change baseline. Co-Authored-By: Claude Opus 5 --- docs/design.md | 46 +++++++++++++++++++ scan_format.md | 44 +++++++++++++----- sras_compute.py | 84 +++++++++++++++++++++++++--------- sras_format.py | 76 ++++++++++++++++++++++-------- sras_viewer/main_window.py | 94 +++++++++++++++++++++++++++++++++++--- sras_workers.py | 10 +++- tests/test_gui.py | 65 ++++++++++++++++++++++++++ 7 files changed, 358 insertions(+), 61 deletions(-) diff --git a/docs/design.md b/docs/design.md index 74cec99..2a21f6c 100644 --- a/docs/design.md +++ b/docs/design.md @@ -134,6 +134,52 @@ at one window size can never be served a cache at another — see `scan_format.md`'s Cache Tail / CACH tail version history sections for the on-disk `row_avg_n` field this depends on. +## Serving a stored cache: provenance, not just presence + +A stored `peak_freq_mhz` image is only interchangeable with a live compute +for the *exact* settings it was computed under. Three of them are baked +irreversibly into the numbers — background subtraction, row-averaging window, +and zero-padding — so all three are recorded in the `SFFT` block and checked +by `cached_rf_image` before it hands the image back. Getting this wrong is +not a slow display, it is a *wrong* display, which is why the check is a +single predicate in one place rather than spread across callers. + +Padding is the subtlest of the three, because a padded FFT looks like it +should be a refinement of the unpadded one. It isn't: zero-padding +interpolates between the natural bins, so it resolves a different peak +frequency for the same waveform. `precomputed_pad_factor` exists so a padded +view can be served from a cache computed at *its* pad while still refusing +one computed at any other, including pad 1. Before it existed the store was +pad 1 by definition and any `n_fft` was rejected outright — correct, but it +meant a user working at a pad factor got nothing at all from batch-computing +a file, which is most of the point of the feature. `pad_factor_for` maps an +`n_fft` request onto the integer factor a store could have recorded, and +returns 0 for a request that is not a whole multiple of `samples_per_frame` +— unmatchable by construction, since only integer factors are representable. + +The batch actions therefore have to cache at the *viewer's* current pad +factor, not a fixed one: a cache stored at a pad nobody is viewing at is +dead weight. When the two do diverge (the user changes the pad after +batching), `_cache_mismatch_notes` says so in the scan info panel, because +the symptom otherwise is just "the file I pre-computed got slow again" with +no visible cause. + +### Two caches, in cost order + +`_refresh_display` consults this window's in-session `_fft_cache`/`_dc_cache` +dicts first, then the open file's stored v5/v7 blocks, and only then +dispatches a `ComputeWorker`. The second tier is what makes a batch-computed +file worth having; without it every angle change queued a worker and a +progress popup for an image already sitting on disk — precisely the cost the +batch was run to avoid. (The stored image *was* reachable before, but only +from inside `ComputeWorker`, i.e. after paying for the thread and the popup.) + +The file tier asks with `allow_dc_recompute=False`. If applying the DC4 mask +would mean reading a whole CH4 channel, it declines rather than blocking the +GUI thread, and the fall-through worker reaches the same stored image via +`compute_rf_image` and pays for the mask off-thread. So the GUI thread never +does I/O, and the slow path is still a fast path. + ## Angle alignment coordinate frames (`sras_compute.py`) Alignment puts every angle's images onto one shared, zero-padded pixel grid diff --git a/scan_format.md b/scan_format.md index b94c0e1..6181c02 100644 --- a/scan_format.md +++ b/scan_format.md @@ -255,7 +255,7 @@ actions. | Offset | Size | Type | Field | Description | |--------|------|------|-------|-------------| | 0 | 4 | `char[4]` | `cach_magic` | `CACH` (ASCII). Missing/wrong magic → treat file as having no cache. | -| 4 | 1 | `u8` | `cach_version` | Cache format version. Currently `2`; readers also accept `1` (a `1` tail predates row-averaged FFT caching — see the `SFFT` block below and [CACH tail version history](#cach-tail-version-history)). Any other value → treat the file as uncached (unlike v5's `PREC` section, which read but never validated its version byte). | +| 4 | 1 | `u8` | `cach_version` | Cache format version. Currently `3`; readers also accept `1` and `2` (each older tail simply lacks the fields added since — see the `SFFT` block below and [CACH tail version history](#cach-tail-version-history)). Any other value → treat the file as uncached (unlike v5's `PREC` section, which read but never validated its version byte). | | 5 | 1 | `u8` | `block_flags` | Bit 0 = DC block (`SDCB`) follows. Bit 1 = FFT block (`SFFT`) follows, immediately after the DC block if both are present. Bits 2–7 reserved, must be zero on write. | ### DC block `SDCB` (present iff `block_flags & 0x01`) @@ -286,18 +286,27 @@ value, same as v5's `PREC` section. Block header layout depends on `cach_version`: +Each `cach_version` appended one trailing field, so the header grows but +never shifts an existing offset: + - **`cach_version` 1**: 7 bytes, format `">4sBH"` — magic, flags, n_stored. -- **`cach_version` 2**: 8 bytes, format `">4sBHB"` — magic, flags, n_stored, - `row_avg_n`. Always written by current code; a `cach_version` 1 tail (no - trailing byte) is still read, with `row_avg_n` taken as `0` for every - entry it stores. +- **`cach_version` 2**: 8 bytes, format `">4sBHB"` — + `row_avg_n`. +- **`cach_version` 3**: 10 bytes, format `">4sBHBH"` — + `pad_factor`. + Always written by current code. + +An older tail is read with its absent fields taken as the only value such a +tail can describe: `row_avg_n = 0` for a `cach_version` 1 tail, which +predates row-averaged FFT caching, and `pad_factor = 1` for `cach_version` +1 or 2, which predate padded caching and are therefore natural-resolution. +Files cached before either change keep working with no recompute. | Offset (rel) | Size | Type | Field | Description | |--------------|------|------|-------|-------------| | 0 | 4 | `char[4]` | `magic` | `SFFT` | | 4 | 1 | `u8` | `flags` | Bit 0 = `bg_sub_applied` — background waveform was subtracted from CH1 before the FFT when these images were computed. Bit 1 = `row_averaged` — `peak_freq_mhz` came from same-row, distance-weighted averaged CH1 waveforms rather than raw per-pixel ones; `row_avg_n` (below) is the neighbor half-width used. Bits 2–7 reserved. | | 5 | 2 | `u16` | `n_stored` | Number of angle entries that follow | -| 7 | 1 | `u8` | `row_avg_n` | *`cach_version` 2 only.* Same-row neighbor half-width, in pixels, that `peak_freq_mhz` was averaged over before its FFT; `0` = raw (unaveraged). Meaningful only when `flags` bit 1 is set — a `cach_version` 1 tail has no such byte and is always `row_avg_n = 0`. | +| 7 | 1 | `u8` | `row_avg_n` | *`cach_version` ≥ 2 only.* Same-row neighbor half-width, in pixels, that `peak_freq_mhz` was averaged over before its FFT; `0` = raw (unaveraged). Meaningful only when `flags` bit 1 is set — a `cach_version` 1 tail has no such byte and is always `row_avg_n = 0`. | +| 8 | 2 | `u16` | `pad_factor` | *`cach_version` 3 only.* Zero-padding factor the stored `peak_freq_mhz` was resolved at: `n_fft = pad_factor × samples_per_frame`, so `1` = natural resolution. Never `0`; a `cach_version` 1 or 2 tail has no such field and is always `pad_factor = 1`. | followed by `n_stored` entries, each: @@ -328,13 +337,18 @@ size. Readers still apply their own live DC4 threshold at display time exactly as for a raw store, using whatever mask they currently have. Readers must fall back to real-time FFT computation (ignoring stored -`peak_freq_mhz`) under the same conditions as v5's PREC fast path: time-domain -gating is active, zero-padding (`n_fft ≠ samples_per_frame`) is requested, -the reader's background-subtraction setting doesn't match +`peak_freq_mhz`) whenever the store's recorded provenance doesn't match what +the reader is asking for: time-domain gating is active, the reader's +requested `n_fft` doesn't equal `pad_factor × samples_per_frame`, the +reader's background-subtraction setting doesn't match `flags.bg_sub_applied`, or the reader's requested `row_avg_n` doesn't match -the stored value exactly — a raw request must never be served a -row-averaged store, or vice versa, and a request at one window size must -never be served a store at another. +the stored value exactly. A raw request must never be served a row-averaged +store, or vice versa; a request at one row-averaging window size must never +be served a store at another; and a request at one padding must never be +served a store at another, since a padded FFT interpolates between the +natural bins and so resolves genuinely different peak frequencies. An +`n_fft` that is not a whole multiple of `samples_per_frame` can never match +any store, because only an integer `pad_factor` is representable. ### In-place write ordering @@ -359,6 +373,12 @@ which has stayed `7` since the Cache Tail was introduced — this is the inner |--------------|--------| | 1 | Initial Cache Tail: `SDCB` (DC) and `SFFT` (FFT, 7-byte header) blocks. | | 2 | `SFFT` header grows one byte, `row_avg_n` — the same-row neighbor half-width the stored `peak_freq_mhz` was averaged over before its FFT, `0` = raw. Readers still accept a `cach_version` 1 tail, treated as `row_avg_n = 0` for every angle it stores, so files cached before this change keep working without a recompute. | +| 3 | `SFFT` header grows a `u16` `pad_factor` — the zero-padding factor the stored `peak_freq_mhz` was resolved at, `1` = natural resolution. Before this, a padded view could never use a stored cache at all (the store was pad 1 by definition and readers rejected any `n_fft ≠ samples_per_frame`), so a user working at a pad factor got no benefit from batch-computing a file. Recording the factor lets such a view be served, while still refusing a store resolved at a *different* pad. Readers accept `cach_version` 1 and 2 tails as `pad_factor = 1`. | + +A reader that does not know a `cach_version` must treat the file as +uncached — not attempt a partial parse — and the file still reads as an +ordinary v7 (byte-identical to v6) scan, so a forward-dated tail costs a +recompute and never correctness. --- diff --git a/sras_compute.py b/sras_compute.py index 37e1696..b9893b7 100644 --- a/sras_compute.py +++ b/sras_compute.py @@ -18,7 +18,8 @@ import numpy as np import scipy.fft as scipy_fft import scipy.ndimage as scipy_ndimage -from sras_format import CH1_IDX, CH3_IDX, CH4_IDX, SrasFile, adc_to_mv +from sras_format import (CH1_IDX, CH3_IDX, CH4_IDX, MAX_PAD_FACTOR, SrasFile, + adc_to_mv) # --------------------------------------------------------------------------- # FFT backend @@ -482,6 +483,23 @@ def _row_average_waveforms(masked_waves: np.ndarray, valid: np.ndarray, return num / den_safe[:, None] +def pad_factor_for(sras: SrasFile, n_fft: int | None) -> int: + """The integer zero-padding factor an *n_fft* request represents, or 0 + if it represents none — i.e. it is not a whole multiple of this file's + samples_per_frame, so no stored image (which only ever records an + integer factor) can answer it. + + 0 rather than None so callers can compare it straight against + ``sras.precomputed_pad_factor``, which is never 0. + """ + if n_fft is None: + return 1 + spf = sras.samples_per_frame + if spf <= 0 or n_fft % spf: + return 0 + return max(1, n_fft // spf) + + def cached_rf_image(sras: SrasFile, angle_idx: int, dc_threshold_mv: float | None, apply_bg_sub: bool = True, @@ -495,13 +513,21 @@ def cached_rf_image(sras: SrasFile, angle_idx: int, real FFT). A stored image stands in only when *all* hold: this angle actually has - a stored image (v5 PREC or v7 CACH); no custom zero-padding is - requested (n_fft is None) — the store is always natural-resolution; - the stored bg-sub flag matches what the caller wants; and - sras.precomputed_row_avg_n == row_avg_n exactly (0 == "raw") — this - last check is what stops a raw request from ever being silently served - a row-averaged image, or vice versa, or a request at one window size - being served a cache stored at a different one. + a stored image (v5 PREC or v7 CACH); the requested zero-padding matches + what the store was computed at (see below); the stored bg-sub flag + matches what the caller wants; and sras.precomputed_row_avg_n == + row_avg_n exactly (0 == "raw") — this last check is what stops a raw + request from ever being silently served a row-averaged image, or vice + versa, or a request at one window size being served a cache stored at a + different one. + + Padding is checked the same way, and for the same reason: a padded FFT + interpolates between the natural bins, so it resolves genuinely + different peak frequencies. *n_fft* of None means natural resolution, + i.e. pad 1; anything else must be an exact whole multiple of + samples_per_frame equal to sras.precomputed_pad_factor. A ragged n_fft + that is not such a multiple can never match a stored image, since the + store only ever records an integer pad factor. If *allow_dc_recompute* is False and no DC4 image is already cached or supplied via *dc4_mv*, applying the mask would mean reading a whole @@ -511,7 +537,7 @@ def cached_rf_image(sras: SrasFile, angle_idx: int, """ cached_freq = sras.precomputed_freq_mhz[angle_idx] if (cached_freq is None - or n_fft is not None + or pad_factor_for(sras, n_fft) != sras.precomputed_pad_factor or sras.precomputed_bg_sub != (apply_bg_sub and sras.background is not None) or sras.precomputed_row_avg_n != row_avg_n): return None @@ -723,6 +749,7 @@ def compute_rf_image(sras: SrasFile, angle_idx: int, def cache_file(path: str, mode: str, apply_bg_sub: bool, fft_backend: str = "scipy", max_workers: int = 0, + pad_factor: int = 1, dc_threshold_mv: float | None = None, row_avg_n: int = 0) -> str: """Compute and store DC or FFT images for every angle of one file, @@ -732,21 +759,27 @@ def cache_file(path: str, mode: str, apply_bg_sub: bool, FFT backend and worker cap are passed explicitly because module globals do not survive a spawn. - mode is "dc", "fft" (raw per-pixel FFT, natural-resolution, unmasked — - masking applied at display time), or "fft_rowavg" (same-row, - distance-weighted CH1 averaging before the FFT — see compute_rf_image's - row_avg_n). fft_rowavg needs *dc_threshold_mv* up front, unlike plain - "fft": neighbor validity is baked into the stored numbers, so it can't - be deferred to display time the way plain masking can. + mode is "dc", "fft" (raw per-pixel FFT, unmasked — masking applied at + display time), or "fft_rowavg" (same-row, distance-weighted CH1 + averaging before the FFT — see compute_rf_image's row_avg_n). + fft_rowavg needs *dc_threshold_mv* up front, unlike plain "fft": + neighbor validity is baked into the stored numbers, so it can't be + deferred to display time the way plain masking can. - The stored FFT cache is always natural-resolution (pad 1): the v7 SFFT - block records no pad factor, and padded views compute live fast enough - (see _peak_bins_zoom) that caching them is not worth a format change. + *pad_factor* is the zero-padding factor to resolve the peaks at: 1 (the + default) is natural resolution, n_fft == samples_per_frame. It is + recorded in the SFFT block so a reader knows which views the stored + numbers answer for — and it has to be the pad the viewer is *actually* + using, since a pad-1 cache is dead weight to a padded view and vice + versa (cached_rf_image refuses the mismatch rather than showing peaks + resolved at the wrong resolution). """ global _MAX_WORKERS try: if mode not in ("dc", "fft", "fft_rowavg"): return f"unknown cache mode {mode!r} (expected 'dc', 'fft', or 'fft_rowavg')" + if not (1 <= pad_factor <= MAX_PAD_FACTOR): + return f"pad_factor must be 1-{MAX_PAD_FACTOR}, got {pad_factor}" set_fft_backend(fft_backend) if max_workers: _MAX_WORKERS = max_workers @@ -757,6 +790,7 @@ def cache_file(path: str, mode: str, apply_bg_sub: bool, "can be batch-cached") n = sras.n_angles + n_fft = sras.samples_per_frame * pad_factor if pad_factor > 1 else None n_workers, angle_budget = plan_angle_level(sras) if mode == "dc": dc3 = _parallel_map( @@ -778,9 +812,13 @@ def cache_file(path: str, mode: str, apply_bg_sub: bool, # internally over blocks, so angles run one at a time with the # full budget. freq = [compute_rf_image(sras, a, dc_threshold_mv=None, - apply_bg_sub=effective_bg) + apply_bg_sub=effective_bg, n_fft=n_fft) for a in range(n)] - sras.write_v7_cache(new_freq_mhz=freq, new_bg_sub=effective_bg) + # new_row_avg_n=0 explicitly: these images are raw per-pixel FFTs, + # and carrying forward a row_avg_n left by an earlier fft_rowavg + # write would label them as something they are not. + sras.write_v7_cache(new_freq_mhz=freq, new_bg_sub=effective_bg, + new_row_avg_n=0, new_pad_factor=pad_factor) else: # "fft_rowavg" if row_avg_n <= 0: return "row_avg_n must be a positive neighbor half-width for fft_rowavg mode" @@ -789,10 +827,12 @@ def cache_file(path: str, mode: str, apply_bg_sub: bool, "(neighbor validity depends on it)") effective_bg = apply_bg_sub and sras.background is not None freq = [compute_rf_image(sras, a, dc_threshold_mv=dc_threshold_mv, - apply_bg_sub=effective_bg, row_avg_n=row_avg_n) + apply_bg_sub=effective_bg, n_fft=n_fft, + row_avg_n=row_avg_n) for a in range(n)] sras.write_v7_cache(new_freq_mhz=freq, new_bg_sub=effective_bg, - new_row_avg_n=row_avg_n) + new_row_avg_n=row_avg_n, + new_pad_factor=pad_factor) return "" except Exception as exc: return str(exc) diff --git a/sras_format.py b/sras_format.py index 9ce7ab8..96ef358 100644 --- a/sras_format.py +++ b/sras_format.py @@ -60,11 +60,15 @@ PREC_FLAG_BG_SUB = 0x01 CACH_MAGIC = b"CACH" CACH_HDR_FMT = ">4sBB" # magic, cach_version, block_flags CACH_HDR_SIZE = struct.calcsize(CACH_HDR_FMT) -CACH_VERSION = 2 # written on every fresh write -CACH_VERSIONS_READABLE = (1, 2) # accepted on read — see - # _read_sfft_block: a v1 tail - # predates row-averaged FFT - # caching and reads as row_avg_n=0 +CACH_VERSION = 3 # written on every fresh write +CACH_VERSIONS_READABLE = (1, 2, 3) # accepted on read — see + # _read_sfft_block. Each bump only + # appended a field, and every older + # tail has a well-defined reading: + # v1 predates row-averaged FFT + # caching (row_avg_n=0) and v1/v2 + # predate padded caching, so both + # are natural-resolution (pad 1). CACH_FLAG_DC = 0x01 CACH_FLAG_FFT = 0x02 @@ -74,9 +78,12 @@ SDCB_HDR_SIZE = struct.calcsize(SDCB_HDR_FMT) SFFT_MAGIC = b"SFFT" SFFT_HDR_FMT_V1 = ">4sBH" # magic, flags, n_stored (cach_version 1) -SFFT_HDR_FMT = ">4sBHB" # + row_avg_n (cach_version 2) +SFFT_HDR_FMT_V2 = ">4sBHB" # + row_avg_n (cach_version 2) +SFFT_HDR_FMT = ">4sBHBH" # + pad_factor (cach_version 3) SFFT_HDR_SIZE_V1 = struct.calcsize(SFFT_HDR_FMT_V1) +SFFT_HDR_SIZE_V2 = struct.calcsize(SFFT_HDR_FMT_V2) SFFT_HDR_SIZE = struct.calcsize(SFFT_HDR_FMT) +MAX_PAD_FACTOR = 0xFFFF # the H field above SFFT_FLAG_BG_SUB = 0x01 SFFT_FLAG_ROW_AVG = 0x02 # peak_freq_mhz came from same-row, # distance-weighted averaged CH1 @@ -180,7 +187,10 @@ class SrasFile: ``precomputed_dc3_mv`` / ``precomputed_dc4_mv`` / ``precomputed_freq_mhz``, always as ragged per-angle lists (``list[np.ndarray | None]``, one entry per angle, ``None`` where that angle was never stored) regardless of - source version. + source version. The scalars ``precomputed_bg_sub`` / + ``precomputed_row_avg_n`` / ``precomputed_pad_factor`` record the + settings the stored FFT images were computed under, so a reader can tell + whether they answer the question it is actually asking. """ def __init__(self, path: str): @@ -238,6 +248,11 @@ class SrasFile: self.precomputed_dc3_mv: list[np.ndarray | None] = [None] * n_angles self.precomputed_bg_sub: bool = False self.precomputed_row_avg_n: int = 0 + # Zero-padding factor the stored peak_freq_mhz images were resolved + # at: 1 = natural resolution (n_fft == samples_per_frame). A padded + # FFT resolves peaks a padded view would, and only such a view can + # be served from it — see sras_compute.cached_rf_image. + self.precomputed_pad_factor: int = 1 def cached_dc_mv(self, angle_idx: int, ch_idx: int) -> np.ndarray | None: """A stored DC image (already in mV) for (angle, channel), or None.""" @@ -529,24 +544,30 @@ class SrasFile: store[angle_idx] = _read_f32_image(f, shape) return flags - def _read_sfft_block(self, f, cach_version: int) -> tuple[int, int] | None: + def _read_sfft_block(self, f, cach_version: int) -> tuple[int, int, int] | None: """Read the SFFT block header — its layout depends on cach_version, - since v2 appended a trailing row_avg_n byte — then n_stored per-angle - peak_freq_mhz entries (unchanged across versions). + since each bump appended a trailing field (v2 row_avg_n, v3 + pad_factor) — then n_stored per-angle peak_freq_mhz entries + (unchanged across versions). - Returns (flags, row_avg_n), or None if the block is malformed. - row_avg_n is always 0 for a v1 tail, which predates row-averaged FFT - caching entirely. + Returns (flags, row_avg_n, pad_factor), or None if the block is + malformed. The absent fields of an older tail take the value that + describes what such a tail can only have been: row_avg_n=0 for v1, + which predates row-averaged FFT caching, and pad_factor=1 for v1/v2, + which predate padded caching and so are natural-resolution. """ - hdr_fmt = SFFT_HDR_FMT_V1 if cach_version == 1 else SFFT_HDR_FMT + hdr_fmt = {1: SFFT_HDR_FMT_V1, 2: SFFT_HDR_FMT_V2}.get( + cach_version, SFFT_HDR_FMT) raw = f.read(struct.calcsize(hdr_fmt)) if len(raw) < struct.calcsize(hdr_fmt): return None + row_avg_n, pad_factor = 0, 1 if cach_version == 1: magic, flags, n_stored = struct.unpack(hdr_fmt, raw) - row_avg_n = 0 - else: + elif cach_version == 2: magic, flags, n_stored, row_avg_n = struct.unpack(hdr_fmt, raw) + else: + magic, flags, n_stored, row_avg_n, pad_factor = struct.unpack(hdr_fmt, raw) if magic != SFFT_MAGIC: return None for _ in range(n_stored): @@ -555,7 +576,7 @@ class SrasFile: break self.precomputed_freq_mhz[angle_idx] = _read_f32_image( f, self.image_shape(angle_idx)) - return flags, row_avg_n + return flags, row_avg_n, pad_factor def _parse_cach_section(self, offset: int): """Parse the v7 CACH tail that holds precomputed DC/FFT images.""" @@ -578,16 +599,18 @@ class SrasFile: result = self._read_sfft_block(f, cach_version) if result is None: return - flags, row_avg_n = result + flags, row_avg_n, pad_factor = result self.precomputed_bg_sub = bool(flags & SFFT_FLAG_BG_SUB) self.precomputed_row_avg_n = row_avg_n if (flags & SFFT_FLAG_ROW_AVG) else 0 + self.precomputed_pad_factor = max(1, pad_factor) def write_v7_cache(self, *, new_dc3_mv: list[np.ndarray | None] | None = None, new_dc4_mv: list[np.ndarray | None] | None = None, new_freq_mhz: list[np.ndarray | None] | None = None, new_bg_sub: bool | None = None, - new_row_avg_n: int | None = None): + new_row_avg_n: int | None = None, + new_pad_factor: int | None = None): """Store computed DC and/or FFT images into this file's CACH tail, in place, converting a v6 source to v7 (or updating an existing v7 file). Only the block(s) passed in are recomputed; whichever block @@ -601,6 +624,12 @@ class SrasFile: It describes the whole stored FFT block, not per-angle, mirroring how bg-sub has never been tracked per-angle either. + *new_pad_factor* is the zero-padding factor the passed *new_freq_mhz* + was resolved at (1 = natural resolution), carried forward the same + way. Like row_avg_n it is provenance, not a hint: a view at a + different pad resolves different peaks, so recording it is what lets + a reader refuse the cache instead of showing the wrong numbers. + The waveform data itself is never touched: the cache tail always starts at ``_cache_tail_offset()``, a fixed offset derived from the header and geometry table alone. @@ -615,8 +644,13 @@ class SrasFile: final_bg_sub = new_bg_sub if new_bg_sub is not None else self.precomputed_bg_sub final_row_avg_n = (new_row_avg_n if new_row_avg_n is not None else self.precomputed_row_avg_n) + final_pad_factor = (new_pad_factor if new_pad_factor is not None + else self.precomputed_pad_factor) if not (0 <= final_row_avg_n <= 255): raise ValueError(f"row_avg_n must fit in a byte (0-255), got {final_row_avg_n}") + if not (1 <= final_pad_factor <= MAX_PAD_FACTOR): + raise ValueError( + f"pad_factor must be 1-{MAX_PAD_FACTOR}, got {final_pad_factor}") dc_entries = [a for a in range(self.n_angles) if final_dc3[a] is not None] fft_entries = [a for a in range(self.n_angles) if final_freq[a] is not None] @@ -638,7 +672,8 @@ class SrasFile: fft_flags = SFFT_FLAG_BG_SUB if final_bg_sub else 0 fft_flags |= SFFT_FLAG_ROW_AVG if final_row_avg_n else 0 payload += struct.pack(SFFT_HDR_FMT, SFFT_MAGIC, fft_flags, - len(fft_entries), final_row_avg_n) + len(fft_entries), final_row_avg_n, + final_pad_factor) for a in fft_entries: payload += struct.pack(">H", a) payload += final_freq[a].astype(">f4").tobytes() @@ -667,6 +702,7 @@ class SrasFile: self.precomputed_freq_mhz = final_freq self.precomputed_bg_sub = final_bg_sub self.precomputed_row_avg_n = final_row_avg_n + self.precomputed_pad_factor = final_pad_factor # ------------------------------------------------------------------ # Axes helpers diff --git a/sras_viewer/main_window.py b/sras_viewer/main_window.py index 6cee70c..d37f4f9 100644 --- a/sras_viewer/main_window.py +++ b/sras_viewer/main_window.py @@ -224,7 +224,14 @@ class SrasViewerWindow(QMainWindow): self.spin_angle.setRange(0, 0) self.spin_angle.setEnabled(False) self.spin_angle.setMinimumWidth(64) - self.spin_angle.editingFinished.connect(self._on_view_changed) + # valueChanged with keyboard tracking off, not editingFinished: the + # latter fires only on Return or focus-out, so stepping the angle (an + # arrow click or Up/Down, the ordinary way to walk a scan) changed the + # number and left the image behind. Tracking off is what keeps + # valueChanged from also firing per keystroke mid-typing, which on a + # large scan would launch a compute for every intermediate angle. + self.spin_angle.setKeyboardTracking(False) + self.spin_angle.valueChanged.connect(self._on_view_changed) self.lbl_angle_deg = QLabel("—") angle_field = QWidget() ar = QHBoxLayout(angle_field) @@ -606,15 +613,49 @@ class SrasViewerWindow(QMainWindow): bg_note = " (bg-sub)" if s.precomputed_bg_sub else " (no bg-sub)" avg_note = (f", row-averaged n={s.precomputed_row_avg_n}" if s.precomputed_row_avg_n else "") + pad_note = (f", pad {s.precomputed_pad_factor}x" + if s.precomputed_pad_factor > 1 else "") notes.append( f"Cached images: DC {n_dc}/{s.n_angles} angles, " f"FFT {n_fft}/{s.n_angles} angles{bg_note if n_fft else ''}" - f"{avg_note if n_fft else ''} " + f"{avg_note if n_fft else ''}{pad_note if n_fft else ''} " "— display is instant for cached angles") + notes += self._cache_mismatch_notes() elif s.version == 7: notes.append("v7 format: no cache blocks stored yet") self.lbl_frame_warn.setText("\n".join(notes)) + def _cache_mismatch_notes(self) -> list[str]: + """Why the file's stored FFT images can't serve the current view, if + they can't. Padding, bg-sub and row-averaging are all baked into the + stored numbers, so changing any of them silently sends every angle + back through a real FFT — worth saying out loud rather than leaving + the user to wonder why a file they batch-computed got slow. + + Deliberately mirrors cached_rf_image's accept rule; the display + always asks for a raw per-pixel image, since row-averaging is a batch + option with no display control. + """ + s = self._sras + if s is None or all(x is None for x in s.precomputed_freq_mhz): + return [] + + reasons = [] + if compute.pad_factor_for(s, self._current_n_fft()) != s.precomputed_pad_factor: + reasons.append(f"stored at pad {s.precomputed_pad_factor}x, " + f"viewing at pad {self._fft_pad_factor}x") + if s.precomputed_bg_sub != (self.chk_bg_sub.isChecked() + and s.background is not None): + reasons.append("stored with background subtraction " + f"{'on' if s.precomputed_bg_sub else 'off'}") + if s.precomputed_row_avg_n: + reasons.append(f"stored row-averaged (n={s.precomputed_row_avg_n}), " + "the display shows raw per-pixel FFTs") + if not reasons: + return [] + return ["! Cached FFT unusable for this view — " + "; ".join(reasons) + + ". FFT angles will recompute."] + # ------------------------------------------------------------------ # Controls # ------------------------------------------------------------------ @@ -885,23 +926,60 @@ class SrasViewerWindow(QMainWindow): self._aligned_cache[key] = cached return cached + def _stored_fft_image(self, angle_idx: int) -> np.ndarray | None: + """The open file's own stored peak-frequency image for the current + view, masked and ready to display, or None if the file has nothing + that answers this exact view. + + allow_dc_recompute=False keeps this off the I/O path: if the mask + would mean reading a whole CH4 channel, this declines and the caller + falls through to the background worker, which reaches the same stored + image via compute_rf_image and pays for the mask off the GUI thread. + """ + return compute.cached_rf_image( + self._sras, angle_idx, + dc_threshold_mv=self.spin_threshold_mv.value(), + apply_bg_sub=self.chk_bg_sub.isChecked(), + n_fft=self._current_n_fft(), + dc4_mv=self._dc_cache.get((angle_idx, CH4_IDX)), + allow_dc_recompute=False) + def _refresh_display(self): """Show the image for the current angle/channel/threshold, using cached data whenever possible and only falling back to a background - compute (with progress popup) when genuinely nothing is cached yet.""" + compute (with progress popup) when genuinely nothing is cached yet. + + Two caches are consulted, in cost order: this window's own in-session + dicts, then the file's stored v5/v7 cache blocks. The second is what + makes a batch-computed file worth having — without it every angle + change queued a worker and a progress popup for an image already on + disk, which is exactly the cost the batch was run to avoid. + """ if self._sras is None: return angle_idx = self.spin_angle.value() ch_idx = self.combo_channel.currentIndex() if ch_idx in CH1_DERIVED_MODES: - raw = self._fft_cache.get(self._fft_cache_key(angle_idx)) + key = self._fft_cache_key(angle_idx) + raw = self._fft_cache.get(key) + if raw is None: + raw = self._stored_fft_image(angle_idx) + if raw is not None: + # Masking the stored image is cheap but not free; keep the + # result so revisiting this angle costs nothing at all. + self._fft_cache[key] = raw if raw is not None: self._show_image_now(self._scale_for_display(raw, ch_idx), angle_idx, ch_idx) return else: + # A stored DC image needs no post-processing, so the file's own + # parsed array is served directly — as the compute path already + # does, _current_image is treated as read-only by every consumer. cached = self._dc_cache.get((angle_idx, ch_idx)) + if cached is None: + cached = self._sras.cached_dc_mv(angle_idx, ch_idx) if cached is not None: self._show_image_now(cached, angle_idx, ch_idx) return @@ -1171,7 +1249,10 @@ class SrasViewerWindow(QMainWindow): return self._batch_errors = [] - worker = BatchCacheWorker(paths, mode, self.chk_bg_sub.isChecked()) + # Cache the FFT at the pad the viewer is actually displaying at, + # otherwise the batch stores images this window can never use. + worker = BatchCacheWorker(paths, mode, self.chk_bg_sub.isChecked(), + pad_factor=self._fft_pad_factor) started = self._run_worker( Jobs.BATCH, worker, connect=( @@ -1212,7 +1293,8 @@ class SrasViewerWindow(QMainWindow): self._batch_errors = [] worker = BatchCacheWorker(paths, "fft_rowavg", self.chk_bg_sub.isChecked(), - dc_threshold_mv=threshold_mv, row_avg_n=n) + dc_threshold_mv=threshold_mv, row_avg_n=n, + pad_factor=self._fft_pad_factor) started = self._run_worker( Jobs.BATCH, worker, connect=( diff --git a/sras_workers.py b/sras_workers.py index 0931eab..bacf896 100644 --- a/sras_workers.py +++ b/sras_workers.py @@ -192,6 +192,10 @@ class BatchCacheWorker(QObject): averaging before the FFT — needs *dc_threshold_mv* and a positive *row_avg_n*; see ``sras_compute.cache_file``). + Both FFT modes cache at *pad_factor*, which the caller sets from the + viewer's own padding — a cache stored at a pad the user is not viewing + at is one the display can never use. + Files are processed one per subprocess: they are fully independent, each opens its own memmap and writes only its own bytes, and only path strings and scalars cross the process boundary. Emits ``progress(int)`` (0–100 by @@ -203,13 +207,15 @@ class BatchCacheWorker(QObject): finished = pyqtSignal() def __init__(self, paths: list[str], mode: str, apply_bg_sub: bool, - dc_threshold_mv: float | None = None, row_avg_n: int = 0): + dc_threshold_mv: float | None = None, row_avg_n: int = 0, + pad_factor: int = 1): super().__init__() self._paths = paths self._mode = mode self._apply_bg_sub = apply_bg_sub self._dc_threshold = dc_threshold_mv self._row_avg_n = row_avg_n + self._pad_factor = pad_factor def _report(self, path: str, err: str, done: int, total: int): self.file_done.emit(path, err) @@ -235,6 +241,7 @@ class BatchCacheWorker(QObject): futures = { executor.submit(cache_file, p, self._mode, self._apply_bg_sub, compute.get_fft_backend(), per_proc_workers, + pad_factor=self._pad_factor, dc_threshold_mv=self._dc_threshold, row_avg_n=self._row_avg_n): p for p in paths @@ -261,6 +268,7 @@ class BatchCacheWorker(QObject): err = cache_file(path, self._mode, self._apply_bg_sub, compute.get_fft_backend(), compute.default_max_workers(), + pad_factor=self._pad_factor, dc_threshold_mv=self._dc_threshold, row_avg_n=self._row_avg_n) except Exception as exc: diff --git a/tests/test_gui.py b/tests/test_gui.py index 599936c..c6f0c57 100644 --- a/tests/test_gui.py +++ b/tests/test_gui.py @@ -106,6 +106,71 @@ def test_angle_switching_from_cache(ctx): "no compute job needed for cached DC angles" +def test_stepping_the_angle_spinbox_redraws(ctx): + """Clicking the angle spinbox's arrows (or pressing Up/Down in it) must + move the display, not just the number. + + This is the ordinary way to walk a scan, and it used to do nothing: the + spinbox was wired on editingFinished, which QAbstractSpinBox emits only + on Return or focus-out — never on a step. Every other test in this module + called _on_view_changed() by hand and so could not have caught it. + """ + win, s = ctx.win, ctx.s + assert s.n_angles >= 3, "need room to step in both directions" + + win.spin_angle.setValue(0) + assert wait_until(lambda: win._current_angle == 0), "settled on angle 0" + + for expected in range(1, s.n_angles): + win.spin_angle.stepUp() + assert wait_until(lambda e=expected: win._current_angle == e), \ + f"stepping up to angle {expected} redrew the display" + + win.spin_angle.stepDown() + assert wait_until(lambda: win._current_angle == s.n_angles - 2), \ + "stepping down redraws too" + + # Keyboard stepping goes through the same signal, so it must work as well. + QTest.keyClick(win.spin_angle, Qt.Key.Key_Down) + assert wait_until(lambda: win._current_angle == s.n_angles - 3), \ + "Key_Down redraws" + + win.spin_angle.setValue(0) + assert wait_until(lambda: win._current_angle == 0), "back to angle 0" + + +def test_typing_an_angle_does_not_compute_intermediate_angles(ctx): + """Keyboard tracking must stay off: with it on, valueChanged fires per + keystroke, so typing "12" would dispatch a compute for angle 1 first — + on a real scan, a whole wasted FFT for an angle the user never asked for. + """ + win, s = ctx.win, ctx.s + assert not win.spin_angle.keyboardTracking(), \ + "keyboard tracking off is what makes valueChanged safe to connect" + + target = s.n_angles - 1 + assert target >= 2, "need a multi-digit-ish range to make the point" + win.spin_angle.setValue(0) + wait_until(lambda: win._current_angle == 0) + + seen = [] + win.spin_angle.valueChanged.connect(seen.append) + try: + win.spin_angle.lineEdit().selectAll() + QTest.keyClicks(win.spin_angle, str(target)) + pump(60) + assert seen == [], f"no signal while typing, got {seen}" + QTest.keyClick(win.spin_angle, Qt.Key.Key_Return) + pump(60) + assert seen == [target], f"one signal on commit, got {seen}" + finally: + win.spin_angle.valueChanged.disconnect(seen.append) + assert wait_until(lambda: win._current_angle == target), "committed angle shown" + + win.spin_angle.setValue(0) + assert wait_until(lambda: win._current_angle == 0), "back to angle 0" + + def test_channel_switching(ctx): win = ctx.win win.spin_angle.setValue(0)