Make both batch image exports reproduce the view on screen
Two strands, committed together because they touch the same four files and answer the same question: an exported PNG must be the image the user was looking at when they triggered the export. Dropped view settings (bug fix): - BatchExportWorker forwarded the DC threshold, bg-sub, pad and row- averaging to compute_rf_image but never min_freq_mhz, so every RF and Velocity map was exported with no min peak frequency floor. The pixels the floor exists to reject came back at their pre-floor peaks. - That also decided *which source* the image came from: requesting floor 0 makes a stored v7 cache look like a match (cache_mismatch_reasons only rejects a higher stored floor), so a batch-computed file exported the cached peak-frequency block verbatim - numbers from an earlier Batch Compute run, while the screen showed that same cache re-masked against the live floor. The symptom was a newly loaded file exporting a stale-looking velocity map, since the floor is a display control that survives a load while the window's own FFT cache does not. - export_view_image had the same class of defect with a different parameter: it dropped row_avg_n, so a file whose stored cache is row- averaged displayed from the store but exported as a full raw recompute - a different, and much slower, image. It now reads sras.precomputed_row_avg_n per file, mirroring _stored_fft_image and _start_compute, and derived per file for the same reason n_fft is. Export at the live canvas size: - Both exporters now take the canvas's current figure size instead of a hard-coded 7x5. draw_view_image uses aspect="auto", so the figure box is what sets the map's proportions - a view sized wide on screen was being squeezed into a different shape on disk. Read at trigger time, in inches, so a resize mid-batch cannot change images later in the same run and a HiDPI display exports like a standard one. - sanitize_figsize clamps a degenerate size (collapsed splitter pane, minimized window): a wrong-looking aspect ratio must never be the reason a batch loses an image. Tests: BatchExportWorker had no coverage at all. Four new tests cover the floor being applied, a stored cache never being served unfloored, the menu-to-worker wiring, and the row-averaged cache case; each was verified to fail against the unfixed code. 166 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Thomas Ales [M S E]
+297
-4
@@ -1,7 +1,18 @@
|
||||
"""Batch Export View as Images: does the exported PNG actually match what
|
||||
"""Both batch image exports -- Convert -> Batch Export View as Images (one
|
||||
PNG per *file*, via sras_render.export_view_image) and Export -> Batch
|
||||
Export Images (one PNG per *angle x channel* of the open file, via
|
||||
sras_workers.BatchExportWorker). Does the exported PNG actually match what
|
||||
the live view would show, and does the batch dispatch (menu action ->
|
||||
worker -> per-file render) behave like Batch Compute's proven pattern?
|
||||
|
||||
The recurring hazard both halves guard is a dropped view setting: every
|
||||
parameter that decides what an FFT-derived pixel *is* (bg-sub, pad,
|
||||
row-averaging, the min peak frequency floor) defaults to "off" in
|
||||
compute_rf_image, so an argument the export forgets to forward does not
|
||||
degrade gracefully -- it silently renders a different image than the screen,
|
||||
and worse, makes this file's stored cache look like a match so the export
|
||||
hands back peaks from an earlier compute.
|
||||
|
||||
sras_render.export_view_image is tested directly (no Qt) for the plumbing
|
||||
that decides *what* gets rendered -- pad_factor derived per file, masked-
|
||||
pixel NaN fill, per-file auto-scale, velocity scaling -- via a
|
||||
@@ -23,11 +34,14 @@ from PyQt6.QtCore import QEventLoop, QTimer
|
||||
from PyQt6.QtWidgets import QApplication
|
||||
|
||||
import sras_render
|
||||
from sras_compute import compute_rf_image, dc_image_mv
|
||||
import sras_workers
|
||||
from sras_compute import cache_file, compute_rf_image, dc_image_mv
|
||||
from sras_format import CH1_IDX, CH3_IDX, CH4_IDX, CH_NAMES, SrasFile
|
||||
from sras_render import export_view_image
|
||||
from sras_render import DEFAULT_FIGSIZE, export_view_image, sanitize_figsize
|
||||
from sras_viewer import SrasViewerWindow, VELOCITY_MODE_IDX
|
||||
from sras_workers import BatchExportImagesWorker
|
||||
from sras_workers import (
|
||||
BatchExportImagesWorker, BatchExportWorker, ExportChannel,
|
||||
)
|
||||
import tools.make_test_sras as gen
|
||||
|
||||
_THRESHOLD_MV = 50.0
|
||||
@@ -67,6 +81,16 @@ def _kw(**overrides):
|
||||
return kw
|
||||
|
||||
|
||||
def _png_size(path) -> tuple[int, int]:
|
||||
"""(width, height) in pixels, read straight out of the PNG's IHDR chunk
|
||||
(two big-endian uint32s at byte 16) -- no image library needed just to
|
||||
check the shape of an export."""
|
||||
raw = Path(path).read_bytes()
|
||||
assert raw[:8] == b"\x89PNG\r\n\x1a\n", "not a valid PNG"
|
||||
return (int.from_bytes(raw[16:20], "big"),
|
||||
int.from_bytes(raw[20:24], "big"))
|
||||
|
||||
|
||||
def _spy_draw(monkeypatch):
|
||||
"""Patches sras_render.draw_view_image to record the image array and
|
||||
vmin/vmax/bad_color it was called with, then delegates to the real
|
||||
@@ -248,6 +272,60 @@ def test_auto_scale_uses_per_file_min_max(tmp_path, monkeypatch):
|
||||
assert captured2["vmax"] == 5.0
|
||||
|
||||
|
||||
@pytest.mark.parametrize("figsize", [(12.0, 4.0), (4.0, 9.0)])
|
||||
def test_export_matches_requested_figsize(tmp_path, figsize):
|
||||
"""The PNG comes out at the caller's figure size, so a view the user has
|
||||
sized wide (or tall) doesn't get squeezed into a fixed 7x5 -- the image
|
||||
is drawn with aspect="auto", so the figure box *is* the map's shape."""
|
||||
path = tmp_path / "shape.sras"
|
||||
gen.write(path, n_angles=1, seed=70, samples_per_frame=64)
|
||||
out_dir = tmp_path / "out"
|
||||
out_dir.mkdir()
|
||||
|
||||
dpi = 100
|
||||
err, out_name = export_view_image(
|
||||
str(path), out_dir=str(out_dir), angle_idx=0, ch_idx=CH4_IDX,
|
||||
figsize=figsize, dpi=dpi, **_kw())
|
||||
assert err == ""
|
||||
|
||||
w_px, h_px = _png_size(out_dir / out_name)
|
||||
# Agg truncates inches*dpi to whole pixels; a pixel of slack, not an
|
||||
# aspect-ratio tolerance, is what's being allowed for here.
|
||||
assert abs(w_px - figsize[0] * dpi) <= 1
|
||||
assert abs(h_px - figsize[1] * dpi) <= 1
|
||||
|
||||
|
||||
def test_export_default_figsize_when_unspecified(tmp_path):
|
||||
"""No figsize (a caller with no live canvas to match) still renders at
|
||||
the viewer's starting size rather than failing or guessing."""
|
||||
path = tmp_path / "default_shape.sras"
|
||||
gen.write(path, n_angles=1, seed=71, samples_per_frame=64)
|
||||
out_dir = tmp_path / "out"
|
||||
out_dir.mkdir()
|
||||
|
||||
err, out_name = export_view_image(
|
||||
str(path), out_dir=str(out_dir), angle_idx=0, ch_idx=CH4_IDX,
|
||||
dpi=100, **_kw())
|
||||
assert err == ""
|
||||
w_px, h_px = _png_size(out_dir / out_name)
|
||||
assert abs(w_px - DEFAULT_FIGSIZE[0] * 100) <= 1
|
||||
assert abs(h_px - DEFAULT_FIGSIZE[1] * 100) <= 1
|
||||
|
||||
|
||||
@pytest.mark.parametrize("bad", [None, (0.0, 0.0), (-3.0, 5.0), (np.nan, 5.0),
|
||||
(float("inf"), 5.0), (7.0,), "7x5"])
|
||||
def test_sanitize_figsize_never_yields_an_unrenderable_size(bad):
|
||||
"""A degenerate canvas size (collapsed pane, minimized window) must cost
|
||||
at most a wrong-looking image, never a failed export."""
|
||||
w, h = sanitize_figsize(bad)
|
||||
assert np.isfinite(w) and np.isfinite(h)
|
||||
assert w >= 1.0 and h >= 1.0
|
||||
|
||||
|
||||
def test_sanitize_figsize_passes_through_a_normal_canvas_size():
|
||||
assert sanitize_figsize(np.array([12.8, 6.4])) == pytest.approx((12.8, 6.4))
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# GUI dispatch: SrasViewerWindow._on_batch_export_images end-to-end
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -375,6 +453,47 @@ def test_batch_export_ignores_aligned_view_toggle(tmp_path):
|
||||
pump(200)
|
||||
|
||||
|
||||
def test_batch_export_uses_the_live_canvas_aspect_ratio(tmp_path):
|
||||
"""The exported PNG has the shape of the view on screen, not a fixed
|
||||
7x5 -- resize the window and the export follows it."""
|
||||
path = tmp_path / "aspect.sras"
|
||||
gen.write(path, n_angles=1, seed=45, samples_per_frame=64)
|
||||
out_dir = tmp_path / "images"
|
||||
out_dir.mkdir()
|
||||
|
||||
win = _make_window(path)
|
||||
try:
|
||||
win.resize(1400, 700)
|
||||
pump(300) # let the canvas' resizeEvent reach the figure
|
||||
canvas_w, canvas_h = win.image_canvas.figure.get_size_inches()
|
||||
|
||||
captured_kwargs = []
|
||||
orig_init = BatchExportImagesWorker.__init__
|
||||
|
||||
def spy_init(self, paths, **kw):
|
||||
captured_kwargs.append(kw)
|
||||
return orig_init(self, paths, **kw)
|
||||
|
||||
with patch.object(BatchExportImagesWorker, "__init__", spy_init), \
|
||||
patch("sras_viewer.main_window.QFileDialog.getOpenFileNames",
|
||||
return_value=([str(path)], "")), \
|
||||
patch("sras_viewer.main_window.QFileDialog.getExistingDirectory",
|
||||
return_value=str(out_dir)):
|
||||
win._on_batch_export_images()
|
||||
assert wait_until(lambda: not win._job_running("batch"), 60000), "batch ran"
|
||||
|
||||
assert captured_kwargs[0]["figsize"] == pytest.approx(
|
||||
(canvas_w, canvas_h)), "the live canvas size must travel to the worker"
|
||||
|
||||
out_files = list(out_dir.iterdir())
|
||||
assert len(out_files) == 1
|
||||
w_px, h_px = _png_size(out_files[0])
|
||||
assert w_px / h_px == pytest.approx(canvas_w / canvas_h, rel=0.01)
|
||||
finally:
|
||||
win.close()
|
||||
pump(200)
|
||||
|
||||
|
||||
def test_batch_export_filename_collision_note(tmp_path):
|
||||
dir_a, dir_b = tmp_path / "dir_a", tmp_path / "dir_b"
|
||||
dir_a.mkdir()
|
||||
@@ -424,3 +543,177 @@ def test_batch_export_does_not_modify_source_files(tmp_path):
|
||||
pump(200)
|
||||
|
||||
assert path.read_bytes() == before, "export must never write to the source file"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Export menu: BatchExportWorker (every angle of the open file)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
# High enough to sit above several of the generator's synthetic peak bins
|
||||
# (bin spacing is 6.25 GS/s / 64 = 97.66 MHz, peaks land at bins 3-19), so a
|
||||
# floor at this value genuinely changes which peak each pixel resolves to --
|
||||
# without that, a test that the floor is honored would pass either way.
|
||||
_FLOOR_MHZ = 1000.0
|
||||
_GRATING_UM = 2.0
|
||||
|
||||
|
||||
def _velocity_channel() -> ExportChannel:
|
||||
return ExportChannel(ch_idx=CH1_IDX, is_velocity=True, vmin=0.0, vmax=1e5,
|
||||
label="Velocity", unit="m/s", tag="VEL")
|
||||
|
||||
|
||||
def _run_export_worker(monkeypatch, sras, out_dir, **overrides) -> list[np.ndarray]:
|
||||
"""Runs BatchExportWorker to completion on the calling thread (its run()
|
||||
is a plain loop; the QThread in _run_worker is a GUI concern) and returns
|
||||
the image arrays it rendered, captured at the _render_map_png seam."""
|
||||
captured = []
|
||||
orig = sras_workers._render_map_png
|
||||
|
||||
def spy(img, extent, **kw):
|
||||
captured.append(np.array(img, copy=True))
|
||||
return orig(img, extent, **kw)
|
||||
|
||||
monkeypatch.setattr(sras_workers, "_render_map_png", spy)
|
||||
kw = dict(cmap="viridis", apply_bg_sub=True, dc_threshold_mv=_THRESHOLD_MV,
|
||||
n_fft=None, grating_um=_GRATING_UM, min_freq_mhz=_FLOOR_MHZ)
|
||||
kw.update(overrides)
|
||||
BatchExportWorker(sras, [_velocity_channel()], str(out_dir), "vel", **kw).run()
|
||||
return captured
|
||||
|
||||
|
||||
def test_batch_export_applies_the_min_peak_freq_floor(tmp_path, monkeypatch):
|
||||
"""The exported Velocity map is the floored one the viewer shows, not the
|
||||
unfloored peaks the floor was raised to reject."""
|
||||
path = tmp_path / "floor.sras"
|
||||
gen.write(path, n_angles=1, seed=80, samples_per_frame=64, geometry=[(6, 9)])
|
||||
out_dir = tmp_path / "out"
|
||||
out_dir.mkdir()
|
||||
sras = SrasFile(str(path))
|
||||
|
||||
def expected(floor):
|
||||
return compute_rf_image(sras, 0, dc_threshold_mv=_THRESHOLD_MV,
|
||||
apply_bg_sub=True, n_fft=None,
|
||||
min_freq_mhz=floor) * _GRATING_UM
|
||||
|
||||
floored, unfloored = expected(_FLOOR_MHZ), expected(0.0)
|
||||
assert not np.allclose(floored, unfloored), \
|
||||
"fixture must be one where the floor changes the image"
|
||||
|
||||
exported = _run_export_worker(monkeypatch, sras, out_dir)
|
||||
assert len(exported) == 1
|
||||
assert np.allclose(exported[0], floored)
|
||||
|
||||
|
||||
def test_batch_export_does_not_serve_an_unfloored_stored_cache(tmp_path,
|
||||
monkeypatch):
|
||||
"""A file batch-computed before the floor existed has a stored cache with
|
||||
no floor recorded. Asking for that cache at floor 0 (rather than the live
|
||||
floor) makes it a match, so the export would render the stored pre-floor
|
||||
peaks -- the map the user raised the floor to get rid of."""
|
||||
path = tmp_path / "stored_floor.sras"
|
||||
gen.write(path, n_angles=1, seed=81, samples_per_frame=64, geometry=[(6, 9)])
|
||||
assert cache_file(str(path), "fft", apply_bg_sub=True) == ""
|
||||
sras = SrasFile(str(path))
|
||||
assert sras.precomputed_freq_mhz[0] is not None, "stored cache written"
|
||||
assert sras.precomputed_min_freq_mhz == 0.0
|
||||
|
||||
out_dir = tmp_path / "out"
|
||||
out_dir.mkdir()
|
||||
exported = _run_export_worker(monkeypatch, sras, out_dir)[0]
|
||||
|
||||
# 0.0 is the "no valid peak" sentinel; anything else below the floor is a
|
||||
# peak that only an unfloored search could have reported.
|
||||
below = (exported > 0) & (exported < _FLOOR_MHZ * _GRATING_UM)
|
||||
assert not below.any(), \
|
||||
f"{below.sum()} sub-floor pixel(s) survived the export"
|
||||
|
||||
|
||||
def test_batch_export_dispatch_passes_the_live_view_settings(tmp_path):
|
||||
"""The Export menu hands the worker what the panel currently says --
|
||||
the floor in particular, which is a display control and so persists
|
||||
across file loads while the window's own FFT cache does not."""
|
||||
path = tmp_path / "dispatch.sras"
|
||||
gen.write(path, n_angles=1, seed=82, samples_per_frame=64)
|
||||
out_dir = tmp_path / "out"
|
||||
out_dir.mkdir()
|
||||
|
||||
win = _make_window(path)
|
||||
try:
|
||||
win.spin_min_freq_mhz.setValue(_FLOOR_MHZ)
|
||||
win.spin_grating_um.setValue(_GRATING_UM)
|
||||
win.spin_threshold_mv.setValue(_THRESHOLD_MV)
|
||||
|
||||
class StubDialog:
|
||||
def __init__(self, parent, **kw):
|
||||
pass
|
||||
|
||||
def exec(self):
|
||||
from PyQt6.QtWidgets import QDialog
|
||||
return QDialog.DialogCode.Accepted
|
||||
|
||||
def get_output_dir(self):
|
||||
return str(out_dir)
|
||||
|
||||
def get_prefix(self):
|
||||
return "vel"
|
||||
|
||||
def get_selected_channels(self):
|
||||
return [_velocity_channel()]
|
||||
|
||||
captured_kwargs = []
|
||||
orig_init = BatchExportWorker.__init__
|
||||
|
||||
def spy_init(self, sras, channels, output_dir, prefix, **kw):
|
||||
captured_kwargs.append(kw)
|
||||
return orig_init(self, sras, channels, output_dir, prefix, **kw)
|
||||
|
||||
with patch.object(BatchExportWorker, "__init__", spy_init), \
|
||||
patch("sras_viewer.main_window.BatchExportDialog", StubDialog):
|
||||
win._on_batch_export()
|
||||
assert wait_until(lambda: not win._job_running("export"), 60000), "export ran"
|
||||
|
||||
kw = captured_kwargs[0]
|
||||
assert kw["min_freq_mhz"] == _FLOOR_MHZ
|
||||
assert kw["grating_um"] == _GRATING_UM
|
||||
assert kw["dc_threshold_mv"] == _THRESHOLD_MV
|
||||
finally:
|
||||
win.close()
|
||||
pump(200)
|
||||
|
||||
|
||||
def test_export_view_image_serves_a_row_averaged_stored_cache(tmp_path,
|
||||
monkeypatch):
|
||||
"""Batch Export View as Images must ask this file's cache the question the
|
||||
viewer asks it. _stored_fft_image reads at the file's own
|
||||
precomputed_row_avg_n; requesting raw per-pixel instead makes a
|
||||
row-averaged cache a mismatch, and the export silently renders a full raw
|
||||
recompute where the screen shows the smoothed stored image."""
|
||||
path = tmp_path / "rowavg_cache.sras"
|
||||
gen.write(path, n_angles=1, seed=83, samples_per_frame=64, geometry=[(6, 9)])
|
||||
|
||||
# A planted stored image rather than a real row-averaged compute: the
|
||||
# question here is *which* source the export reads, and a distinctive
|
||||
# array answers it without depending on the synthetic waveforms being
|
||||
# smooth enough for averaging to move the numbers.
|
||||
sras = SrasFile(str(path))
|
||||
shape = sras.image_shape(0)
|
||||
stored = (100.0 + 10.0 * np.arange(shape[0] * shape[1], dtype=np.float32)
|
||||
).reshape(shape)
|
||||
sras.write_v7_cache(new_freq_mhz=[stored], new_row_avg_n=5,
|
||||
new_bg_sub=True, new_pad_factor=1)
|
||||
|
||||
reread = SrasFile(str(path))
|
||||
raw = compute_rf_image(reread, 0, dc_threshold_mv=None, apply_bg_sub=True,
|
||||
n_fft=None, row_avg_n=0, use_stored=False)
|
||||
assert not np.allclose(stored, raw), "planted cache must be distinguishable"
|
||||
|
||||
out_dir = tmp_path / "out"
|
||||
out_dir.mkdir()
|
||||
captured = _spy_draw(monkeypatch)
|
||||
err, _out_name = export_view_image(
|
||||
str(path), out_dir=str(out_dir), angle_idx=0, ch_idx=CH1_IDX,
|
||||
**_kw(is_fft_mode=True, dc_threshold_mv=-1e9, colorbar_label="MHz",
|
||||
mode_str="RF"))
|
||||
assert err == ""
|
||||
assert np.allclose(captured["img"], stored), \
|
||||
"export rendered a recompute instead of the stored image on screen"
|
||||
|
||||
Reference in New Issue
Block a user