diff --git a/docs/design.md b/docs/design.md index 2a21f6c..7b0c583 100644 --- a/docs/design.md +++ b/docs/design.md @@ -164,6 +164,25 @@ 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. +That divergence note is informational only, not a warning of an impending +recompute. The *display* path (`_stored_fft_image`) never asks +`cached_rf_image` whether a stored image matches the window's live +bg-sub/pad/row-averaging controls — it asks whether the image matches its +*own* recorded settings (`sras.precomputed_bg_sub`/`precomputed_pad_factor`/ +`precomputed_row_avg_n`), which is always true whenever a stored image +exists. So presence alone decides whether it's shown; the live controls +never gate it. They still matter for two things: a genuinely never-computed +angle's first live compute, and an explicit batch recompute — both of which +read the live controls and produce new stored data, at which point it's the +new data's *own* settings that get self-matched from then on. This is what +keeps a view switch (angle, channel, or flipping bg-sub/pad) from ever +discarding precomputed data — only an explicit batch recompute does, and it +already reloads the file afterward so the new data displays immediately. +Row-averaging has no live control to diverge from in the first place (it's +only ever set inside the batch dialog), so it never appears in the +divergence note — the "Cached images" line's own `row-averaged n=…` phrase +already covers it. + ### Two caches, in cost order `_refresh_display` consults this window's in-session `_fft_cache`/`_dc_cache` diff --git a/sras_viewer/main_window.py b/sras_viewer/main_window.py index 7345064..fa6ceed 100644 --- a/sras_viewer/main_window.py +++ b/sras_viewer/main_window.py @@ -90,10 +90,13 @@ class SrasViewerWindow(QMainWindow): # computed lazily (with a progress popup) the first time an # angle/threshold combination is viewed — using the cached DC4 # image to skip the FFT entirely for masked-out pixels — and - # cached per (angle, bg_sub, n_fft, threshold) so revisiting the - # same combination is free. + # cached per (angle, threshold) so revisiting the same combination + # is free. bg-sub/pad are deliberately not part of the key: once an + # angle has any FFT image (live or from the file's own stored + # cache), it stays displayed regardless of those controls — see + # _fft_cache_key. self._dc_cache: dict[tuple[int, int], np.ndarray] = {} - self._fft_cache: dict[tuple[int, bool, int | None, float], np.ndarray] = {} + self._fft_cache: dict[tuple[int, float], np.ndarray] = {} self._dc_generation: int = 0 # Angle alignment ("Fusion" menu) @@ -275,7 +278,9 @@ class SrasViewerWindow(QMainWindow): self.chk_bg_sub.setEnabled(False) self.chk_bg_sub.setToolTip( "Subtract the stored background waveform from each CH1 frame\n" - "before computing the FFT (v4+ files only)." + "before computing the FFT (v4+ files only). Applies to angles\n" + "not yet computed and to future batch recomputes — it does not\n" + "change an image already shown or already stored in the file." ) self.chk_bg_sub.toggled.connect(self._on_bg_sub_toggled) vl.addWidget(self.chk_bg_sub) @@ -622,16 +627,20 @@ class SrasViewerWindow(QMainWindow): 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. + """Informational only: whether the file's stored FFT cache was + computed under different bg-sub/pad settings than these controls + currently say. The display always shows the stored image as-is + regardless (see _stored_fft_image) — these controls only affect a + future live compute for an angle with nothing cached yet, or an + explicit batch recompute, never what's already on screen. + + row_avg_n is compared against the file's own recorded value (a + self-match), so it never contributes a reason here — there's no + live control for it to diverge from, and the "Cached images" line + above already reports it. Asks compute for the reasons rather than restating the accept rule, - so a new provenance field can only be added in one place. The display - always asks for a raw per-pixel image, since row-averaging is a batch - option with no display control. + so a new provenance field can only be added in one place. """ s = self._sras if s is None or all(x is None for x in s.precomputed_freq_mhz): @@ -639,11 +648,13 @@ class SrasViewerWindow(QMainWindow): reasons = compute.cache_mismatch_reasons( s, n_fft=self._current_n_fft(), - apply_bg_sub=self.chk_bg_sub.isChecked(), row_avg_n=0) + apply_bg_sub=self.chk_bg_sub.isChecked(), + row_avg_n=s.precomputed_row_avg_n) if not reasons: return [] - return ["! Cached FFT unusable for this view — " + "; ".join(reasons) - + ". FFT angles will recompute."] + return ["Note: current bg-sub/pad controls differ from the stored " + "cache — " + "; ".join(reasons) + ". Shown as stored; use " + "Batch Compute to recompute with these settings."] # ------------------------------------------------------------------ # Controls @@ -691,11 +702,12 @@ class SrasViewerWindow(QMainWindow): self._on_view_changed() def _on_bg_sub_toggled(self): - # Background subtraction changes the FFT input, so it genuinely - # invalidates the cached raw FFT (the cache key includes it) — - # _refresh_display() recomputes only on a miss for the new state. + # bg-sub no longer gates the display: it only affects a future live + # compute for an angle with nothing cached yet, or an explicit batch + # recompute — never what's already shown. Just keep the info panel's + # divergence note current. if self._is_fft_mode(): - self._refresh_display() + self._update_scan_info_labels() def _on_grating_changed(self): # Grating is a pure post-multiply on the cached frequency image — @@ -889,8 +901,15 @@ class SrasViewerWindow(QMainWindow): return freq_mhz def _fft_cache_key(self, angle_idx: int) -> tuple: - return (angle_idx, self.chk_bg_sub.isChecked(), self._current_n_fft(), - self.spin_threshold_mv.value()) + """Keyed by angle and DC threshold only. Once any FFT image exists + for an angle this session — live-computed or pulled from the file's + own stored cache — it stays the displayed image for that angle + regardless of later bg-sub/pad toggles; those only affect a future + live compute for an angle with nothing cached yet, or an explicit + batch recompute (see _stored_fft_image). Threshold stays in the key + because re-masking against it is free and meant to stay interactive + (see _on_threshold_changed).""" + return (angle_idx, self.spin_threshold_mv.value()) def _aligned_cache_key(self, angle_idx: int, ch_idx: int) -> tuple: """Mirrors _fft_cache's key granularity so a stale aligned image is @@ -916,20 +935,35 @@ class SrasViewerWindow(QMainWindow): 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. + """The open file's own stored peak-frequency image for this angle, + masked and ready to display, or None if the file has nothing stored + for it. + + Asks for the image at the settings it was actually computed under + (sras.precomputed_bg_sub / precomputed_pad_factor / + precomputed_row_avg_n) rather than the window's live bg-sub/pad + controls, so a stored image is always shown once present — those + controls never gate whether it's used, only what a *future* compute + produces. See _cache_mismatch_notes for the informational (non- + blocking) note when the live controls diverge from what's shown. + Only the DC threshold is taken live: re-masking a stored image + against it is free, unlike bg-sub/pad/row-averaging which are baked + irreversibly into the stored numbers. 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. """ + s = self._sras + n_fft = (s.samples_per_frame * s.precomputed_pad_factor + if s.precomputed_pad_factor > 1 else None) return compute.cached_rf_image( - self._sras, angle_idx, + s, angle_idx, dc_threshold_mv=self.spin_threshold_mv.value(), - apply_bg_sub=self.chk_bg_sub.isChecked(), - n_fft=self._current_n_fft(), + apply_bg_sub=s.precomputed_bg_sub, + n_fft=n_fft, + row_avg_n=s.precomputed_row_avg_n, dc4_mv=self._dc_cache.get((angle_idx, CH4_IDX)), allow_dc_recompute=False) @@ -1109,8 +1143,7 @@ class SrasViewerWindow(QMainWindow): ch_idx = self._pending_ch if ch_idx in CH1_DERIVED_MODES: - self._fft_cache[(angle_idx, self._pending_bg_sub, - self._current_n_fft(), self._pending_threshold)] = result + self._fft_cache[(angle_idx, self._pending_threshold)] = result img = self._scale_for_display(result, ch_idx) else: img = result @@ -1427,11 +1460,12 @@ class SrasViewerWindow(QMainWindow): self._fft_pad_factor = dlg.get_pad_factor() self._settings.setValue("fft/backend", compute.get_fft_backend()) self._settings.setValue("fft/pad_factor", self._fft_pad_factor) - # Pad factor changes the FFT bin count, so it genuinely invalidates - # the cached raw FFT (part of the cache key) — _refresh_display() - # recomputes only on a cache miss. + # Pad factor no longer gates the display: it only affects a future + # live compute for an angle with nothing cached yet, or an explicit + # batch recompute — never what's already shown. Just keep the info + # panel's divergence note current. if self._is_fft_mode(): - self._refresh_display() + self._update_scan_info_labels() # ------------------------------------------------------------------ diff --git a/tests/test_gui.py b/tests/test_gui.py index 5624e6a..3a26abc 100644 --- a/tests/test_gui.py +++ b/tests/test_gui.py @@ -222,16 +222,25 @@ def test_threshold_change_recomputes(ctx): def test_bg_sub_toggle(ctx): + """bg-sub no longer gates the display: it only affects a future live + compute for an angle with nothing cached yet, or an explicit batch + recompute. Toggling it on an angle that already has an FFT image must + leave that image on screen, untouched.""" win = ctx.win n_before = len(win._fft_cache) + img_before = win._current_image win.chk_bg_sub.setChecked(False) - assert wait_until( - lambda: not win._job_running("compute") and len(win._fft_cache) > n_before), \ - "recomputed without bg-sub" - win.chk_bg_sub.setChecked(True) pump(200) assert not win._job_running("compute"), \ - "returning to bg-sub was a cache hit (no recompute)" + "toggling bg-sub alone must not dispatch a recompute" + assert len(win._fft_cache) == n_before, "no new cache entry from the toggle" + assert np.array_equal(win._current_image, img_before), \ + "displayed image unchanged by the bg-sub toggle" + win.chk_bg_sub.setChecked(True) + pump(200) + assert not win._job_running("compute") + assert len(win._fft_cache) == n_before + assert np.array_equal(win._current_image, img_before) def test_roi_and_csv_export(ctx): diff --git a/tests/test_stored_cache.py b/tests/test_stored_cache.py index 03a8793..f802e53 100644 --- a/tests/test_stored_cache.py +++ b/tests/test_stored_cache.py @@ -256,11 +256,16 @@ def test_viewer_shows_stored_angles_without_computing(rig, no_fft, monkeypatch): rig.fresh[0] * win.spin_grating_um.value(), atol=1e-3) assert dispatched == [] and not no_fft - # ...but a setting the stored image cannot serve must still recompute, - # or the fast path would be showing the wrong picture. + # ...and a live control that no longer matches the stored image's own + # provenance must NOT force a recompute either — the stored image is + # shown as-is; only an explicit batch recompute changes what's shown. + win.combo_channel.setCurrentIndex(CH1_IDX) + pump(60) win.chk_bg_sub.setChecked(False) - assert wait_until(lambda: not win._job_running("compute") and bool(no_fft)), \ - "bg-sub off falls through to a real FFT" + pump(200) + assert not win._job_running("compute") and not no_fft, \ + "bg-sub off still shows the stored image, no real FFT" + assert np.allclose(win._current_image, rig.fresh[0], atol=1e-3) finally: win.close() pump(300) @@ -322,16 +327,21 @@ def test_batch_caches_at_the_viewers_pad_factor(tmp_path, no_fft, monkeypatch): f"no recompute at pad 10 (jobs={dispatched}, fft={no_fft})" assert "unusable" not in win.lbl_frame_warn.text() - # Change the pad and the cache legitimately stops applying — and the - # info panel has to say so rather than leave it a mystery. + # Change the live pad control so it no longer matches the stored + # image's own provenance — the info panel has to say so rather than + # leave it a mystery, but the stored pad-10 image keeps displaying; + # only an explicit batch recompute would ever produce a pad-4 one. win._fft_pad_factor = 4 win._update_scan_info_labels() - assert "Cached FFT unusable" in win.lbl_frame_warn.text(), \ + assert "differ from the stored cache" in win.lbl_frame_warn.text(), \ win.lbl_frame_warn.text() assert "pad 10x" in win.lbl_frame_warn.text() + last_angle = win._current_angle win._refresh_display() - assert wait_until(lambda: not win._job_running("compute") and bool(no_fft)), \ - "pad 4 recomputes rather than reusing the pad-10 cache" + assert wait_until(lambda: not win._job_running("compute")), "settled" + assert not no_fft, "no real FFT ran — the pad-10 cache still served the view" + assert np.allclose(win._current_image, expected[last_angle], atol=1e-3), \ + "pad-10 cache still shown after the live pad control diverged" finally: win.close() pump(300) @@ -500,16 +510,17 @@ def test_cach_v1_backward_compat_defaults_row_avg_n_zero(tmp_path): "a v1 tail (predating this feature) can never satisfy a row-averaged request" -def test_viewer_batch_row_average_dispatch(tmp_path, monkeypatch): +def test_viewer_batch_row_average_dispatch(tmp_path, monkeypatch, no_fft): """Driving the new 'Batch Compute Row-Averaged FFT and Store' action end-to-end through the real menu handler: dialog values reach the worker, the worker reaches cache_file, and the written file is - self-describing afterward. Deliberately does not assert anything about - whether viewing an angle afterward dispatches a compute job -- that is - a separate, pre-existing gap in _refresh_display shared with the plain - DC/FFT batch actions (see test_viewer_shows_stored_angles_without_computing - / test_viewer_shows_stored_dc_without_computing above), not something - row-averaging introduces or is responsible for fixing.""" + self-describing afterward. Also the exact scenario the row-averaged-FFT + recompute bug reported: before the fix, the display always asked for + row_avg_n=0 regardless of what the file actually had stored, so viewing + an angle after this batch action saw a phantom mismatch and launched a + full raw recompute on every view switch. This asserts that no longer + happens -- the stored row-averaged image is shown directly, with no + dispatched compute job and no real FFT.""" path = tmp_path / "rowavg_gui.sras" gen.write(path, n_angles=2, seed=26, samples_per_frame=128) @@ -531,6 +542,12 @@ def test_viewer_batch_row_average_dispatch(tmp_path, monkeypatch): app = QApplication.instance() or QApplication([]) # noqa: F841 win = SrasViewerWindow() win.show() + + dispatched = [] + original_start = type(win)._start_compute + monkeypatch.setattr(type(win), "_start_compute", + lambda self: (dispatched.append(self.spin_angle.value()), + original_start(self))[1]) try: win._load_file(str(path)) assert wait_until(lambda: win._sras is not None), "file loaded" @@ -553,6 +570,26 @@ def test_viewer_batch_row_average_dispatch(tmp_path, monkeypatch): apply_bg_sub=win.chk_bg_sub.isChecked(), row_avg_n=6) assert expected is not None, "the batch write left a readable row-averaged cache" + + # The regression this batch action used to leave unfixed: viewing an + # angle afterward must show the stored row-averaged image directly, + # never fall through to a real (raw) recompute. + no_fft.clear() + dispatched.clear() + win.combo_channel.setCurrentIndex(CH1_IDX) + assert wait_until(lambda: win._current_ch == CH1_IDX), "CH1 displayed" + for a in range(win._sras.n_angles): + win.spin_angle.setValue(a) + pump(60) + assert np.allclose(win._current_image, + compute.cached_rf_image( + win._sras, a, + dc_threshold_mv=win.spin_threshold_mv.value(), + apply_bg_sub=win.chk_bg_sub.isChecked(), + row_avg_n=6), atol=1e-3), \ + f"angle {a} shows the stored row-averaged image" + assert dispatched == [] and not no_fft, \ + f"no recompute for row-averaged angles (jobs={dispatched}, fft={no_fft})" finally: win.close() pump(300)