From 15efdf8318e9cbf696a56defc61d04076b7e80d8 Mon Sep 17 00:00:00 2001 From: Carter Francis Date: Thu, 27 Aug 2026 08:06:46 -0500 Subject: [PATCH 1/5] fix(dpc): the field map tracks the beam region, and the pass is cancellable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dragging the beam region did nothing until the pass before it had run all the way through. Measured in the real app on a 48x48x64x64 lazy scan: ZERO map repaints across a 6.4 s drag. Three separate causes, and the first is the one that matters. THE MAP DID NOT FOLLOW THE REGION. `_on_region_drag` armed a debounce on `pointer_up` only, so the map was frozen for the whole gesture. The region is now a real BaseSelector — the same CircleSelector/AnnularSelector a virtual image puts on the same plot — so it inherits the navigator's serial latest-wins dispatcher and its trailing settle instead of a hand-rolled debounce. The re-measure hangs off `index_hooks`, the seam the vector overlays already use. That is what "it should be identical to the virtual image" actually means, and it deletes more than it adds: the settle timer, the live timer, the pending flags and the paint gate are all gone. THE PASS COULD NOT BE STOPPED. It ran through `signal.map`, which has no interruption point anywhere inside it, so a superseded pass finished and only THEN had its result discarded. It is now one dask graph and one `client.compute` future held on `_measure_future` — the cancellation `virtual_image.reduce` already uses. Chunks paint into the display array as they land (`compute_with_live_buffer`, the virtual-image stream's own call), and the map BLANKS when a new pass starts, so superseding is something you can see rather than something you have to trust. THE PASS WAS SLOW ENOUGH THAT ALL OF THIS SHOWED. The centre of mass ran a Python function per frame with a `scipy.ndimage` call inside it; at tens of thousands of frames that overhead IS the runtime. It is now one contraction per block against stacked detector-sized weights: 64x64 scan of 64x64 frames before 1429.4 ms after 38.3 ms (37x) the virtual image, for scale 15.5 ms Stacking the three moments into one `(3, sy, sx)` weight array reads the block ONCE instead of three times (73 ms -> 38 ms). Bit-identical to the per-frame reference, which is what keeps pyxem equivalence: a test asserts `array_equal` frame by frame rather than `allclose`. Also fixed, found while testing the above: - Opening the wizard fired TWO passes. Placing the selector writes its geometry, and the widget reports that write as a move, so an identical pass immediately superseded the opening one — a whole scan's work and the progressive fill with it. `measure` now records the region it ran with and a move to that same geometry does not re-measure. - The brightness readout did a dask frame read on the event loop at `pointer_up`. It runs on a worker now and refreshes when a pass lands. - The beam region has no "off". It IS what the centre of mass is taken over, so switching it off only meant taking the whole frame with no handle to grab. - The default radius went from a quarter of the short detector axis to HALF — the inscribed circle. With the region always on, the default is what an unattended scan is measured with, and the old one CLIPPED the beam: on the synthetic disc it under-read true shifts of 1.5/3.0/4.5 px by 0.30/1.00/1.76 px. The inscribed circle reproduces the whole-frame answer exactly. `dpc_live_region.spec.ts` pins the user-visible claim by sampling the map's own CANVAS pixels across a real drag: 0 changed frames on the old behaviour, 12-13 on this one. Deliberately not a window screenshot — the "Calculating…" chip pulses on a CSS animation, and hashing that scored the BROKEN build 22 of 24 and passed. --- .../src/renderer/src/components/DpcWizard.tsx | 13 +- electron/tests/dpc_live_region.spec.ts | 222 ++++++ electron/tests/dpc_workflow.spec.ts | 26 +- spyde/actions/dpc.py | 146 +++- spyde/actions/dpc_action.py | 708 +++++++++++------- spyde/tests/migrated/test_dpc.py | 117 ++- spyde/tests/migrated/test_dpc_action.py | 242 ++++-- 7 files changed, 1050 insertions(+), 424 deletions(-) create mode 100644 electron/tests/dpc_live_region.spec.ts diff --git a/electron/src/renderer/src/components/DpcWizard.tsx b/electron/src/renderer/src/components/DpcWizard.tsx index 6b396b57..dbb38b72 100644 --- a/electron/src/renderer/src/components/DpcWizard.tsx +++ b/electron/src/renderer/src/components/DpcWizard.tsx @@ -65,10 +65,10 @@ const VIEW_OPTIONS: readonly { value: View; label: string }[] = [ { value: 'curl', label: 'Curl' }, ] -/** `dpc.BEAM_SHAPES` — the beam region's shape, or off. */ -type BeamShape = 'off' | 'circle' | 'ring' +/** `dpc.BEAM_SHAPES`. There is no "off": the region is always on the pattern, + * because it IS what the centre of mass is taken over. */ +type BeamShape = 'circle' | 'ring' const BEAM_SHAPES: readonly { value: BeamShape; label: string }[] = [ - { value: 'off', label: 'Off' }, { value: 'circle', label: 'Circle' }, { value: 'ring', label: 'Ring' }, ] @@ -99,7 +99,7 @@ const DEFAULTS: DpcSaved = { halfSquareWidth: 0, centerMode: 'corners', cornerFraction: 0.05, - beamShape: 'off', + beamShape: 'circle', beamCx: 0.0, beamCy: 0.0, beamR: 0.0, @@ -267,7 +267,7 @@ export function DpcWizard({ caretPos, windowId, sendAction, onClose }: Props) { // it. Same shape as the Crop / Center-Zero-Beam drag→field round trip. useWizardEvent('spyde:dpc_region', windowId, (d) => { const r: Region = { - shape: String(d.shape ?? 'off') as BeamShape, + shape: String(d.shape ?? 'circle') as BeamShape, cx: Number(d.cx ?? 0), cy: Number(d.cy ?? 0), r: Number(d.r ?? 0), r_inner: Number(d.r_inner ?? 0), brightness: d.brightness == null ? null : Number(d.brightness), @@ -433,7 +433,8 @@ export function DpcWizard({ caretPos, windowId, sendAction, onClose }: Props) { ))} - {beamShape !== 'off' && ( + {/* Always rendered: there is no shape that hides these. */} + {( <> { + test.setTimeout(600_000) + fs.mkdirSync(SHOTS, { recursive: true }) + + const ctx = await launchApp({ dask: true, env: { SPYDE_LOG_LEVEL: 'INFO' } }) + const { page, backend, assertNoJsErrors } = ctx + + try { + // LAZY + chunked, so the pass streams through the real cluster and takes + // long enough for "does it restart mid-drag?" to be answerable at all. On + // eager data every pass finishes instantly and the question is vacuous. + await backendAction(page, 'load_test_data_dpc', + { nav: 48, sig: 64, lazy: true, nav_chunk: 8 }) + await waitForSubwindowCount(page, 2, 180_000) + await page.waitForTimeout(3_000) + await page.screenshot({ path: `${SHOTS}/01-loaded.png` }) + + const sig = page.getByTestId('subwindow') + .filter({ has: page.getByTestId('subwindow-title').filter({ hasText: /S-Synthetic DPC$/ }) }) + .last() + await sig.getByTestId('subwindow-title').click() + await sig.getByTestId('subwindow-titlebar').hover() + await sig.getByTestId('action-btn-DPC').click() + await expect(sig.getByTestId('dpc-wizard')).toBeVisible({ timeout: 60_000 }) + + // The first pass has to land before we drag, or "the map changed" would be + // measuring the opening pass filling in rather than the drag. + await expect.poll( + () => sig.getByTestId('dpc-centering').getAttribute('data-worst'), + { timeout: 180_000, message: 'the opening pass never produced a field' }) + .toMatch(/\d/) + await page.screenshot({ path: `${SHOTS}/02-first-pass.png` }) + + await sig.getByTestId('dpc-tab-Center').click() + await sig.getByTestId('dpc-beam-circle').click() + await expect(sig.getByTestId('dpc-beam-r')).toBeVisible() + await page.waitForTimeout(4_000) + await page.screenshot({ path: `${SHOTS}/03-beam-on.png` }) + + const dpcWin = page.getByTestId('subwindow') + .filter({ has: page.getByTestId('subwindow-title').filter({ hasText: DPC_TITLE }) }) + .first() + await expect(dpcWin).toBeVisible({ timeout: 60_000 }) + + /** + * A fingerprint of the MAP'S OWN pixels, read off its canvases. + * + * NOT a screenshot of the window. The "Calculating…" chip sits on top of + * the figure and pulses on a 1.6 s CSS animation, so a screenshot hash + * changes on almost every sample while a pass is running — which is most of + * a drag. Measured with a window screenshot, the RELEASE-ONLY behaviour this + * spec exists to catch scored 22 of 24 "live frames" and the spec passed. A + * canvas readback sees only what the figure drew. + */ + const mapHash = async (): Promise => { + const host = await dpcWin.elementHandle() + if (!host) return 'no-window' + const parts: string[] = [] + for (const frame of page.frames()) { + const el = await frame.frameElement().catch(() => null) + if (!el) continue + const inside = await host.evaluate( + (w, f) => w.contains(f as Node), el).catch(() => false) + if (!inside) continue + parts.push(await frame.evaluate(() => { + let h = 0, n = 0 + for (const c of Array.from(document.querySelectorAll('canvas'))) { + const cv = c as HTMLCanvasElement + const g = cv.getContext('2d') + if (!g || !cv.width) continue + const d = g.getImageData(0, 0, cv.width, cv.height).data + for (let i = 0; i < d.length; i += 41) { h = (h * 31 + d[i]) | 0; n++ } + } + return `${n}:${h}` + }).catch(() => 'err')) + } + return parts.join('|') + } + + /** + * Where the beam circle is on screen, from its colour (#94e2d5). + * + * The ring is symmetric, so the centroid of its teal pixels IS its centre — + * which is where the drag handle sits. Guessing a point inside the circle + * instead grabs nothing: the cursor readout still tracks, so the figure + * looks driven while the circle stays put, and the run reports "0 frames" + * for a drag that never happened. That is indistinguishable from the bug, + * which is why the grab below is verified rather than assumed. + */ + const beamCentre = async () => { + const host = await sig.elementHandle() + if (!host) return null + for (const frame of page.frames()) { + const el = await frame.frameElement().catch(() => null) + if (!el) continue + const inside = await host.evaluate( + (w, f) => w.contains(f as Node), el).catch(() => false) + if (!inside) continue + const hit = await frame.evaluate(() => { + let sx = 0, sy = 0, n = 0 + for (const c of Array.from(document.querySelectorAll('canvas'))) { + const cv = c as HTMLCanvasElement + const g = cv.getContext('2d') + if (!g || !cv.width) continue + const d = g.getImageData(0, 0, cv.width, cv.height).data + const box = cv.getBoundingClientRect() + const kx = box.width / cv.width, ky = box.height / cv.height + for (let i = 0; i < d.length; i += 4) { + const r = d[i], gg = d[i + 1], b = d[i + 2] + if (!(r > 110 && r < 175 && gg > 200 && b > 185 && b < 240)) continue + const px = (i / 4) % cv.width, py = Math.floor((i / 4) / cv.width) + sx += box.left + px * kx; sy += box.top + py * ky; n++ + } + } + return n > 8 ? { x: sx / n, y: sy / n } : null + }).catch(() => null) + if (hit) { + const fb = await el.boundingBox() + return { x: (fb?.x ?? 0) + hit.x, y: (fb?.y ?? 0) + hit.y } + } + } + return null + } + + const sigBox = await sig.boundingBox() + const start = await beamCentre() + if (!sigBox || !start) throw new Error('could not find the beam circle') + + // Prove the grab took before measuring anything (see beamCentre). + let grabbed = false + for (let attempt = 0; attempt < 4 && !grabbed; attempt++) { + await page.mouse.move(start.x, start.y) + await page.mouse.down() + await page.mouse.move(start.x, start.y + 12) + await page.waitForTimeout(250) + const now = await beamCentre() + grabbed = !!now && Math.abs(now.y - start.y) > 3 + if (!grabbed) { + await page.mouse.up() + await page.waitForTimeout(400) + } + } + expect(grabbed, 'never managed to grab the beam circle').toBe(true) + + const seen: string[] = [await mapHash()] + for (let i = 1; i <= STEPS; i++) { + const t = i / STEPS + await page.mouse.move( + start.x + Math.sin(t * Math.PI * 2) * sigBox.width * 0.12, + start.y + t * sigBox.height * 0.18) + await page.waitForTimeout(120) + const h = await mapHash() + if (h !== seen[seen.length - 1]) seen.push(h) + } + await page.mouse.up() + const liveFrames = seen.length - 1 + await page.screenshot({ path: `${SHOTS}/04-drag-end.png` }) + + console.log(`[dpc-live] distinct map frames across ${STEPS} drag steps: ${liveFrames}`) + expect(liveFrames, + 'the field map did not repaint while the beam region was dragged — ' + + 'the re-measure is waiting for pointer_up again') + .toBeGreaterThanOrEqual(MIN_LIVE_FRAMES) + + // The region moved, so the RESTING field must differ from the one the map + // opened with. Without the trailing settle the map would be left showing + // whichever superseded partial happened to be up when the pointer stopped. + await expect.poll(mapHash, { + timeout: 90_000, + message: 'the settle never measured the resting region', + }).not.toBe(seen[seen.length - 1]) + await page.screenshot({ path: `${SHOTS}/05-after-settle.png` }) + + expect(backendErrorLines(backend), 'the backend reported an error') + .toEqual([]) + } finally { + assertNoJsErrors() + await ctx.app?.close() + } +}) diff --git a/electron/tests/dpc_workflow.spec.ts b/electron/tests/dpc_workflow.spec.ts index 29776d23..17833a01 100644 --- a/electron/tests/dpc_workflow.spec.ts +++ b/electron/tests/dpc_workflow.spec.ts @@ -366,20 +366,22 @@ test('the Center tab offers all three references, each with its own furniture', // The caret is still open from the previous test, parked on its Map tab. await page.getByTestId('dpc-tab-Center').click() - // The beam region FIRST — Manual below depends on it being on, since the - // region's centre is what Manual adopts. + // The beam region FIRST — Manual below depends on it, since the region's + // centre is what Manual adopts. // // One draggable shape that BOTH masks the centre of mass and marks the zero - // beam. Toggling Circle→Ring swaps the anyplotlib widget type, so this - // checks the shape actually changed on the PATTERN, not just in the caret. + // beam. It is ALWAYS on the pattern: there is no "off", because the region + // is what the centre of mass is taken over, and switching it off only meant + // taking the whole frame with no handle to grab. Toggling Circle→Ring swaps + // the anyplotlib widget type, so this checks the shape actually changed on + // the PATTERN, not just in the caret. const beamPixels = () => colorPixelsIn(page, sourceWindow(page), IS_BEAM) - expect(await beamPixels(), 'the region should start off').toBe(0) + await expect.poll(beamPixels, { + timeout: 30_000, message: 'the beam region was not on the pattern at open', + }).toBeGreaterThan(0) await page.getByTestId('dpc-beam-circle').click() await expect(page.getByTestId('dpc-beam-r')).toBeVisible() - await expect.poll(beamPixels, { - timeout: 30_000, message: 'the beam circle never appeared on the pattern', - }).toBeGreaterThan(0) await expect(page.getByTestId('dpc-beam-readout')) .toHaveAttribute('data-brightness', /\d/, { timeout: 30_000 }) await shot('10-beam-circle') @@ -401,14 +403,8 @@ test('the Center tab offers all three references, each with its own furniture', await page.getByTestId('dpc-info-beam').click() await expect(page.getByTestId('dpc-info-beam-text')).toHaveCount(0) - await page.getByTestId('dpc-beam-off').click() - await expect.poll(beamPixels, { - timeout: 30_000, message: 'turning the region off left its widget behind', - }).toBe(0) - // Manual — the beam region IS the marker, so this is one click, not a - // separate crosshair to place. Turn the region back on so there is a centre - // to adopt. + // separate crosshair to place. Back to a circle for it. await page.getByTestId('dpc-beam-circle').click() await expect.poll(beamPixels, { timeout: 30_000 }).toBeGreaterThan(0) await page.getByTestId('dpc-center-mode').click() diff --git a/spyde/actions/dpc.py b/spyde/actions/dpc.py index 9a4f63d1..12f2e80f 100644 --- a/spyde/actions/dpc.py +++ b/spyde/actions/dpc.py @@ -101,7 +101,7 @@ # ───────────────────────────────────────────────────────────────────────────── #: Shapes the beam region can take. ``off`` searches the whole frame. -BEAM_SHAPES: tuple[str, ...] = ("off", "circle", "ring") +BEAM_SHAPES: tuple[str, ...] = ("circle", "ring") @dataclass @@ -127,7 +127,7 @@ class BeamRegion: widgets report and the frame ``measure_beam_shifts`` works in. Do not add a scale conversion. """ - shape: str = "off" + shape: str = "circle" cx: float = 0.0 cy: float = 0.0 r: float = 0.0 @@ -139,7 +139,15 @@ def center(self) -> tuple[float, float]: @property def active(self) -> bool: - return self.shape in ("circle", "ring") and self.r > 0 + """Whether this region can actually be integrated over. + + There is no "off" shape — the region is always on the pattern. What this + still guards is the moment BEFORE a radius exists: a sensible default is + a fraction of the detector, whose size is unknown until a dataset is + open, so a freshly constructed region carries ``r = 0`` until + :func:`default_beam_region` fills it in. + """ + return self.shape in BEAM_SHAPES and self.r > 0 def mask(self, sig_shape: tuple[int, int]) -> np.ndarray | None: """Boolean keep-mask over the detector, or ``None`` when inactive.""" @@ -170,7 +178,7 @@ def as_dict(self) -> dict: def from_dict(cls, d: dict | None) -> "BeamRegion": d = d or {} shape = str(d.get("shape", "off")) - return cls(shape=shape if shape in BEAM_SHAPES else "off", + return cls(shape=shape if shape in BEAM_SHAPES else "circle", cx=float(d.get("cx") or 0.0), cy=float(d.get("cy") or 0.0), r=float(d.get("r") or 0.0), r_inner=float(d.get("r_inner") or 0.0)) @@ -178,16 +186,30 @@ def from_dict(cls, d: dict | None) -> "BeamRegion": def default_beam_region(sig_shape: tuple[int, int], shape: str = "circle" ) -> BeamRegion: - """A region centred on the detector, generous enough to open on. - - A quarter of the shorter detector axis: comfortably larger than a typical - direct-beam disc, small enough to exclude the first-order discs that would - otherwise drag the centroid. It is a STARTING point to drag, not a - calibration — the caret shows what fraction of the frame's intensity it - captures so an obviously wrong one is visible. + """The LARGEST region that fits on the detector — the inscribed circle. + + The region is always on the pattern, so its default is what every scan is + measured with until someone drags it. That makes "generous" a correctness + requirement, not a nicety: a region that CLIPS the beam pulls the centroid + back towards its own centre and under-reads the shift. Measured on the + synthetic disc, with the region centred on the frame: + + ====================== ========================================== + radius error on a true shift of 1.5 / 3.0 / 4.5 px + ====================== ========================================== + ``0.25 * min(sy, sx)`` −0.30 / −1.00 / −1.76 px + ``0.375 * min(sy, sx)`` −0.00 / −0.00 / −0.02 px + ``0.5 * min(sy, sx)`` 0 / 0 / 0 + ====================== ========================================== + + So a quarter-frame default — chosen when the region opened OFF and only + ever appeared because someone asked for it — silently under-reads every + field by up to 40%. The inscribed circle reproduces the whole-frame answer + exactly, and shrinking it onto the beam (which is what excludes the + first-order discs) is then a deliberate act with a readout to judge it by. """ sy, sx = int(sig_shape[0]), int(sig_shape[1]) - r = max(2.0, 0.25 * min(sy, sx)) + r = max(2.0, 0.5 * min(sy, sx)) return BeamRegion(shape=shape, cx=sx / 2.0, cy=sy / 2.0, r=r, r_inner=r * 0.35) @@ -213,6 +235,54 @@ def _com_shift_frame(frame, keep, cx0: float, cy0: float): return np.array([cx0 - com_x, cy0 - com_y], dtype=np.float64) +def _region_weights(keep, sig_shape: tuple[int, int]) -> np.ndarray: + """``(3, sy, sx)`` float64: the mask, and the mask weighted by x and by y. + + STACKED, so the centre of mass is one contraction over the data rather than + three. All three quantities read the same block, and a block is the big + thing — three separate ``einsum`` calls read it three times, which measured + 73 ms against 38 ms for the stacked form on a 64x64x64x64 scan. + + Built ONCE per pass, never per frame. They are detector-sized (a few hundred + KB at most), while the data they weight is the whole scan. + """ + sy, sx = int(sig_shape[0]), int(sig_shape[1]) + mask = (np.ones((sy, sx), dtype=np.float64) if keep is None + else np.asarray(keep, dtype=np.float64)) + yy, xx = np.mgrid[0:sy, 0:sx].astype(np.float64) + return np.stack([mask, mask * xx, mask * yy]) + + +def _com_shift_blocks(block, weights, cx0: float, cy0: float) -> np.ndarray: + """Every frame in a block's ``centre − beam`` shift → ``(..., 2)``. + + The vectorised twin of :func:`_com_shift_frame`, and the one production + uses. Same arithmetic, same result — ``test_dpc.py`` asserts they agree + element for element — but as three ``einsum`` contractions over the whole + block instead of a Python call per frame with a ``scipy`` call inside it. + A scan is tens of thousands of frames, so per-frame overhead IS the runtime; + this is the same shape ``virtual_image`` reduces with, for the same reason. + + ``einsum`` also accumulates IN PLACE against detector-sized weights, so a + block's only intermediates are the nav-sized outputs. Casting the block to + float64 first would allocate four times the chunk on uint16 data — the + "spills GiBs" pathology virtual_image documents. + + Do NOT pass ``optimize=True``: it plans a contraction that materialises an + intermediate and measured 136 ms against 38 ms for the plain call. + """ + moments = np.einsum("...ij,kij->...k", block, weights) + total = moments[..., 0] + # Nothing inside the region — a NaN says "no measurement here" and + # propagates visibly, where a silent 0 would read as a centred beam. + empty = ~np.isfinite(total) | (total <= 0) + with np.errstate(invalid="ignore", divide="ignore"): + out = np.stack([cx0 - moments[..., 1] / total, + cy0 - moments[..., 2] / total], axis=-1) + out[empty] = np.nan + return out + + def private_view(signal): """A signal object the caller owns exclusively, over the SAME data buffer. @@ -260,9 +330,9 @@ def beam_shift_graph(signal, *, method: str = "center_of_mass", region: "BeamRegion | None" = None): """The LAZY ``(ny, nx, 2)`` beam-shift array, or ``None`` on eager data. - Split out from :func:`measure_beam_shifts` so a caller that wants the pass - to arrive progressively can hand the graph to - ``ComputeBackend.compute_chunks_progressive`` instead of blocking on it. + Split out from :func:`measure_beam_shifts` so an interactive caller can hand + the graph to ``client.compute`` and hold a CANCELLABLE future, instead of + blocking on a pass a newer beam region has already superseded. The nav chunking is preserved end to end (no rechunk layer), which is what makes per-nav-chunk streaming line up with the storage — see Live-Display §1 in CLAUDE.md and ``test_dpc.py::TestLazy``. @@ -274,12 +344,24 @@ def beam_shift_graph(signal, *, method: str = "center_of_mass", except Exception as e: # pragma: no cover log.debug("set_signal_type(electron_diffraction) failed: %s", e) if region is not None and region.active and str(method) == "center_of_mass": + import dask.array as da sy, sx = _sig_shape(signal) - out = signal.map(_com_shift_frame, keep=region.mask((sy, sx)), - cx0=sx / 2.0, cy0=sy / 2.0, - inplace=False, ragged=False, lazy_output=True, - output_signal_size=(2,), output_dtype=float, - show_progressbar=False) + data = signal.data + weights = _region_weights(region.mask((sy, sx)), (sy, sx)) + # map_blocks, not hyperspy `map`: one vectorised contraction per chunk + # rather than a Python call per frame. The nav chunking is preserved + # (no rechunk layer), which is what keeps per-nav-chunk streaming lined + # up with the storage — Live-Display §1. + return da.map_blocks( + _com_shift_blocks, data, weights=weights, + cx0=sx / 2.0, cy0=sy / 2.0, dtype=np.float64, + drop_axis=(data.ndim - 2, data.ndim - 1), + new_axis=data.ndim - 2, + chunks=data.chunks[:-2] + ((2,),), + # Declared, not probed: without it dask calls the kernel an extra + # time on a zero-sized block just to learn the output type. + meta=np.empty((0,) * (data.ndim - 1), dtype=np.float64), + ) else: kw: dict = {"method": str(method), "lazy_output": True} hw = int(half_square_width or 0) @@ -311,6 +393,7 @@ def measure_beam_shifts(signal, *, method: str = "center_of_mass", This is a REDUCTION over the dataset, not a materialisation: a lazy signal streams through ``map`` and only the ``(ny, nx, 2)`` result is computed — the memory-safety rule in CLAUDE.md is respected. + """ try: signal.set_signal_type("electron_diffraction") @@ -319,13 +402,22 @@ def measure_beam_shifts(signal, *, method: str = "center_of_mass", if region is not None and region.active and str(method) == "center_of_mass": sig_shape = _sig_shape(signal) - keep = region.mask(sig_shape) sy, sx = sig_shape - shifts = signal.map(_com_shift_frame, keep=keep, - cx0=sx / 2.0, cy0=sy / 2.0, - inplace=False, ragged=False, lazy_output=False, - output_signal_size=(2,), output_dtype=float, - show_progressbar=False) + weights = _region_weights(region.mask(sig_shape), sig_shape) + # Straight through the vectorised path — hyperspy's `map` would add a + # Python call per frame around arithmetic that is already one einsum + # over the whole array. + # + # A LAZY signal goes through `beam_shift_graph` rather than contracting + # the dask array here: this kernel's subscript is a per-BLOCK one, and + # dask's own einsum does not reproduce it. One code path for lazy data, + # and this function's contract stays a materialised (ny, nx, 2). + graph = beam_shift_graph(signal, method=method, + half_square_width=half_square_width, + region=region) + if graph is not None: + return np.asarray(graph.compute(), dtype=np.float64) + return _com_shift_blocks(signal.data, weights, sx / 2.0, sy / 2.0) else: kw: dict = {"method": str(method), "lazy_output": False} hw = int(half_square_width or 0) @@ -1191,8 +1283,6 @@ def component_titles(mode: str, units: str) -> dict[str, str]: "magnitude": f"|{sym}| ({units})", "phase": "Direction (rad)", "divergence": "Divergence", "curl": "Curl", } - - def compute_dpc(signal, *, mode: str = "magnetic", method: str = "center_of_mass", half_square_width: int = 0, center_mode: str = "corners", diff --git a/spyde/actions/dpc_action.py b/spyde/actions/dpc_action.py index c13f1955..c0399ed0 100644 --- a/spyde/actions/dpc_action.py +++ b/spyde/actions/dpc_action.py @@ -27,7 +27,7 @@ instead of a click-and-wait. Do not move the measure into ``dpc_tune``. **That one pass STREAMS on lazy data.** It is dispatched per navigation chunk -through ``ComputeBackend.compute_chunks_progressive`` and the map repaints as +as ONE cancellable ``client.compute`` future, and the map repaints as each chunk lands, so a scan that takes minutes shows a field filling in rather than a spinner. Two properties make that work and are worth not breaking: @@ -88,6 +88,8 @@ import concurrent.futures import logging +import os +import time import numpy as np @@ -109,11 +111,12 @@ "half_square_width": 0, "center_mode": "corners", "corner_fraction": 0.05, - # The beam region (see BeamRegion): "off" | "circle" | "ring". It opens OFF - # so the measurement is byte-for-byte what it was before this control - # existed; the radii are filled in from the detector size the first time it - # is switched on, because a default in pixels cannot know the frame size. - "beam_shape": "off", + # The beam region (see BeamRegion): "circle" | "ring". There is no "off" — + # the region is what the centre of mass is taken over, so a pattern without + # one is just a region the size of the whole frame, drawn nowhere the user + # can grab it. The radii are filled in from the detector size once a dataset + # is open, because a default in pixels cannot know the frame size. + "beam_shape": "circle", "beam_cx": 0.0, "beam_cy": 0.0, "beam_r": 0.0, @@ -140,12 +143,6 @@ #: heavier edge than the other furniture to stay findable against it. _CORNER_LINEWIDTH = 3.0 -#: Re-measuring the whole scan is the one expensive step, so a DRAG must not -#: trigger it per frame. The widget and the readouts follow the pointer live; -#: the re-measure waits this long after motion stops. Same shape as the drift -#: caret's ROI settle, for the same reason. -_REGION_SETTLE_S = 0.45 - #: Bare-figure window geometry. A bare figure never receives ``resize_figure``, #: so its initial px size is the one it keeps and anything drawn outside is #: CLIPPED by the subwindow — see the same note in ``drift_action``. @@ -274,12 +271,12 @@ def __init__(self, session, tree, src_plot, *, params: dict | None = None): self.clim: tuple[float, float] | None = None self.cmap: str | None = None self._corner_mg = None # navigator corner boxes - self._beam_widget = None # the circle/ring on the DP - self._beam_handler = None # kept alive (weak callback) - self._beam_dragging = False # re-entrancy guard - self._settle_timer = None # drag → debounced re-measure + self._beam_selector = None # the circle/ring on the DP + self._lane = None # the serial pass-setup lane self._measure_stop: list | None = None # in-flight pass's cancel token self._measure_future = None # …and its future, if any + self._measured_region = None # the region a pass last ran + self._measure_event = None # …and what the stream waits on self._last_brightness = None # re-sent during a drag # ── the source signal ──────────────────────────────────────────────────── @@ -303,7 +300,7 @@ def _sig_shape(self) -> tuple[int, int]: def region(self) -> _dpc.BeamRegion: """The beam region the caret's parameters describe.""" p = self.params - return _dpc.BeamRegion(shape=str(p.get("beam_shape", "off")), + return _dpc.BeamRegion(shape=str(p.get("beam_shape", "circle")), cx=float(p.get("beam_cx") or 0.0), cy=float(p.get("beam_cy") or 0.0), r=float(p.get("beam_r") or 0.0), @@ -316,6 +313,15 @@ def measure(self, *, on_done=None) -> None: the map is repainted as each lands, so a multi-minute scan shows a field filling in rather than a spinner. On eager data (already in RAM) there is nothing to stream and it runs in one go. + + Only the BOOKKEEPING happens on the caller's thread: cancel the pass + this one replaces, take the parameters it will run with, and adopt its + cancel token so a supersede arriving a moment later still stops it. + Everything after that — copying the signal, building the graph, handing + the chunks to the backend — runs on :meth:`_pass_lane`. Measured, that + part costs 13-33 ms (hyperspy's ``map`` / ``get_direct_beam_position`` + graph build dominates), and a dragged region runs it several times a + second: inline, it stuttered the very gesture it exists to serve. """ if self.signal is None: emit_error("DPC: no active dataset") @@ -325,25 +331,23 @@ def measure(self, *, on_done=None) -> None: # waiting for costs the cluster the entire dataset. Same contract as # virtual_image (cancel prior → unregister → register new). self._cancel_measure() - # A pass gets its OWN signal object, taken here on the dispatch thread. - # The worker below runs hyperspy `map` on it, and a new pass can still - # overlap the TAIL of the one it cancels: a queued future cancels - # cleanly, one already inside `map` does not. See dpc.private_view. - signal = _dpc.private_view(self.signal) method = str(self.params["method"]) hw = int(self.params["half_square_width"] or 0) region = self.region() sig_shape = self._sig_shape() gen = self.guard() - # Only the EAGER branch below uses this one; the progressive branch owns - # its own token, because that is where a pass can actually be stopped - # part-way (see _measure_progressive). - eager_stop: list = [False] + # Adopted BEFORE the lane picks the pass up, so a supersede that lands + # while it is still queued stops it before it costs anything: the lane + # checks the token first and drops the job. During a drag that is most + # of them. + stop: list = [False] + self._track_measure(stop) + self._measured_region = region.as_dict() emit_status("DPC: locating the direct beam…") def _finish(shifts): - self._retire_measure(eager_stop) - if eager_stop[0] or not self.still(gen) or self._closed: + self._retire_measure(stop) + if stop[0] or not self.still(gen) or self._closed: return self.shifts = np.asarray(shifts, dtype=np.float64) self.report = _dpc.centering_report(self.shifts) @@ -352,31 +356,212 @@ def _finish(shifts): emit_progress(1, 1, "DPC") self.emit_state() self.refresh() + # Now, not per pointer frame: this reads a frame off the dataset. + self.emit_region(with_brightness=True) if on_done is not None: on_done() - if self._measure_progressive(signal, method, hw, region, gen, _finish): + try: + self._pass_lane().submit(self._begin_pass, stop, gen, method, hw, + region, _finish) + except RuntimeError as e: # lane shut down under a closing wizard + log.debug("DPC pass lane refused the job: %s", e) + self._retire_measure(stop) + + def _pass_lane(self): + """The ONE thread every beam-shift pass is set up on. + + Serial for the reason :func:`dpc.private_view` gives: hyperspy parks a + length-1 placeholder on ``data`` for the width of the copy, so two + threads setting a pass up on the same signal read each other's + placeholder. One lane means the copy, and the graph build after it, can + never overlap another pass's — however fast the region is dragged. + + A superseded job drops itself at the top of :meth:`_begin_pass`, so what + queues behind a slow set-up is no-ops, not stacked passes. + """ + if self._lane is None: + self._lane = concurrent.futures.ThreadPoolExecutor( + max_workers=1, thread_name_prefix="dpc-pass") + return self._lane + + def _begin_pass(self, stop, gen, method, hw, region, on_finish) -> None: + """Set the pass up and hand it to the cluster — on the pass lane. + + ONE handle, kept on ``_measure_future`` so the next move can cancel it, + and the chunks paint into the display array as they land. Both come from + ``compute_with_live_buffer``, which is what the virtual-image stream + uses: a superseded whole-scan reduction has to be CANCELLED (its + ``cancel`` stops outstanding work through the dispatcher's ``on_start`` + hook, not on a 0.5 s poll), and a scan big enough to be worth cancelling + is big enough that the user should watch it fill in. + + Data already in RAM has no graph and so no handle, and runs to the end + uninterrupted — the token can only drop its result. That is fine now + and was not before: the centre of mass is one vectorised contraction + per block rather than a Python call per frame, which measured 1429 ms → + 38 ms on a 64x64 scan of 64x64 frames. A pass that finishes in the time + of two pointer frames does not need stopping. + """ + if stop[0] or self._closed: + return + live = self.signal + if live is None: + return + try: + # Its OWN signal object: the graph runs hyperspy `map` on it, and a + # new pass can still overlap the TAIL of the one it cancels. + signal = _dpc.private_view(live) + graph = _dpc.beam_shift_graph(signal, method=method, + half_square_width=hw, region=region) + except Exception as e: + self._retire_measure(stop) + self._measure_failed(gen, e) + self._set_computing(False) + return + if stop[0] or self._closed: return - # Eager data: ONE hyperspy `map` over an array already in RAM. There is - # no interruption point inside it, so the token can only stop the pass - # BEFORE it starts and drop the result if it lands late — which is also - # all `virtual_image` does for eager data. Registering it still matters: - # closing the tree flips it, so a pass in the queue never begins. - self._track_measure(eager_stop) + client = getattr(self.tree, "client", None) + backend = getattr(self.session, "compute_backend", None) + if graph is None or (client is None and backend is None): + # Nothing to chunk and so nothing to cancel: the data is already in + # RAM, or there is neither a cluster nor a compute backend to hand + # the chunks to. The token can only drop the result. + try: + shifts = _dpc.measure_beam_shifts(signal, method=method, + half_square_width=hw, + region=region) + except Exception as e: + self._retire_measure(stop) + self._measure_failed(gen, e) + self._set_computing(False) + return + self._dispatch(lambda: on_finish(shifts)) + return - def _work(): - if eager_stop[0]: - return None - return _dpc.measure_beam_shifts(signal, method=method, - half_square_width=hw, region=region) + ny, nx = int(graph.shape[0]), int(graph.shape[1]) + field = self._display_field(signal, (ny, nx), method, hw, region, gen) + if stop[0] or self._closed: + return + self.shifts = field + self._set_computing(True) + total = max(1, len(graph.chunks[0]) * len(graph.chunks[1])) + landed = [0] + # The cluster stream waits on an Event; the backend and the tree's + # cancel registry work in `[False]` tokens. Both are set by the same + # _cancel_measure, so there is one decision and two ways of hearing it. + import threading + event = threading.Event() - def _done(shifts): - if shifts is not None: - _finish(shifts) + def _on_chunk(chunk, slices): + """A dask callback thread — store, then marshal the repaint.""" + if stop[0] or self._closed or not self.still(gen): + return + try: + field[slices] = np.asarray(chunk, dtype=np.float64) + except Exception as e: # pragma: no cover + log.debug("storing a DPC chunk failed: %s", e) + return + landed[0] += 1 + n = landed[0] + if n >= total: + # The last chunk IS the completed pass. Counting them is how the + # finish is known: the handle's value is assembled client-side + # from these same chunks, so waiting on it as well would only + # add a hop. + self._dispatch(lambda: on_finish(field)) + return + self._dispatch(lambda: self._on_partial(gen, n, total)) + + try: + if client is not None: + # The cluster path, and the virtual-image stream's own call: + # ONE handle, per-chunk callbacks, and a `cancel` that stops + # outstanding work through the dispatcher's `on_start` hook + # rather than on its next 0.5 s poll. + from spyde.drawing.update_functions import ( + compute_with_live_buffer) + handle = compute_with_live_buffer( + graph, (ny, nx), client, "", on_chunk_done=_on_chunk, + windowed=True, stop_event=event) + else: + # No cluster (it takes ~10 s to come up, and a scan can be open + # sooner; tests run with SPYDE_NO_DASK). The backend still + # chunks and still stops — the threaded mode checks the token + # before each submit AND inside each chunk task. Routing this + # through a plain blocking compute instead is what made a + # superseded pass run to the end. + handle = backend.compute_chunks_progressive( + graph, 2, _on_chunk, stopped_flag=stop) + except Exception as e: + self._retire_measure(stop) + self._measure_failed(gen, e) + self._set_computing(False) + return + if self._measure_stop is stop: + self._measure_event = event + self._attach_future(stop, handle) + + def _display_field(self, signal, nav_shape, method, hw, region, gen): + """The ``(ny, nx, 2)`` array the pass paints into. + + BLANK, deliberately, and this is the visible half of cancellation: the + map goes dark the instant a new pass starts and fills back in as its + chunks land, so stopping one pass and starting another is something you + can SEE rather than something you have to trust. It is what the virtual + image does, and it is the reason a superseded compute there is never in + doubt. Carrying the previous field over instead looks smoother and + hides exactly the thing worth showing. + + The FIRST pass after the caret opens is the exception: there is nothing + to supersede and nothing on screen, so it seeds from the CORNERS. Those + are a few percent of the scan, so they measure in a fraction of the time + the full pass takes, and the plane through them is the descan ramp — a + real map immediately, rather than an empty window with a spinner. + """ + ny, nx = int(nav_shape[0]), int(nav_shape[1]) + field = np.full((ny, nx, 2), np.nan, dtype=np.float64) + if self.shifts is not None: + return field # a supersede: go dark and refill + try: + corners = self._measure_corners(signal, (ny, nx), method, hw, region) + except Exception as e: + log.debug("the DPC corner seed failed: %s", e) + return field + if corners is not None and self.still(gen): + field[:] = corners + return field + + def _measure_corners(self, signal, nav_shape, method, hw, region): + """A whole-field plane fitted through the four scan corners only. + + The corners carry the instrument descan and (by assumption) none of the + sample's field, which is what makes them both cheap to measure and worth + showing on their own. + """ + fraction = float(self.params["corner_fraction"]) + sparse = np.full((int(nav_shape[0]), int(nav_shape[1]), 2), np.nan, + dtype=np.float64) + for rows, cols in _dpc.corner_slices(nav_shape, fraction): + block = _dpc.measure_beam_shifts(signal.inav[cols, rows], + method=method, + half_square_width=hw, region=region) + sparse[rows, cols] = block + return _dpc.corner_reference(sparse, fraction) + + def _dispatch(self, fn) -> None: + """Run *fn* on the event loop — figures and IPC belong there.""" + dispatch = getattr(self.session, "_dispatch_to_main", None) + dispatch(fn) if dispatch is not None else fn() + + def _on_partial(self, gen: int, done: int, total: int) -> None: + """Repaint from what has landed so far (event loop).""" + if self._closed or not self.still(gen): + return + emit_progress(done, total, "DPC: locating the direct beam") + self.refresh() - self.run_on_worker(_work, name="dpc-measure", on_done=_done, - on_error=lambda e: self._measure_failed(gen, e)) def _track_measure(self, stop: list, future=None) -> None: """Adopt *stop*/*future* as the in-flight pass and register them on the @@ -387,13 +572,34 @@ def _track_measure(self, stop: list, future=None) -> None: if reg is not None: reg(flag=stop, future=future) + def _attach_future(self, stop: list, future) -> None: + """Add the backend future to an ALREADY-adopted token. + + The token is adopted on the caller's thread; the future only exists once + the pass has been set up on the lane, by which time a newer pass may own + the slot. Attaching to a token that is no longer current would make the + next ``_cancel_measure`` cancel the WRONG pass — so if this one has been + superseded, cancel its future here instead. + """ + if self._measure_stop is not stop: + try: + if not future.done(): + future.cancel() + except Exception as e: # pragma: no cover + log.debug("cancelling a superseded DPC future failed: %s", e) + return + self._measure_future = future + reg = getattr(self.tree, "register_cancel", None) + if reg is not None: + reg(flag=stop, future=future) + def _retire_measure(self, stop: list) -> None: """Drop a FINISHED pass's token. Without this the tree's cancel registry gains an entry per measure — and every drag settle is a measure.""" if self._measure_stop is not stop: return # already superseded; not ours to drop future = self._measure_future - self._measure_stop = self._measure_future = None + self._measure_stop = self._measure_future = self._measure_event = None unreg = getattr(self.tree, "unregister_cancel", None) if unreg is not None: try: @@ -404,19 +610,30 @@ def _retire_measure(self, stop: list) -> None: def _cancel_measure(self) -> None: """Stop the in-flight beam-shift pass, if any. - Setting the flag is what actually stops it: the progressive path checks - it before each chunk submit and inside each chunk task, so a superseded - pass stops dispatching instead of computing a result nobody reads. - Cancelling the future kills one still queued; one already running ends - at its next flag check. Then unregister both, or the tree's cancel list - grows by one entry per drag. + **Cancelling the future is what stops it.** The pass is ONE + ``client.compute`` graph, so this is the same cancellation + ``virtual_image`` uses and the scheduler drops the tasks. The token + beside it covers only the window BEFORE that future exists — the graph + is built on the pass lane, and a move can land while it is — and stops a + late result being painted. Unregister both afterwards, or the tree's + cancel list grows by one entry per drag. """ stop, future = self._measure_stop, self._measure_future - self._measure_stop = self._measure_future = None + event = self._measure_event + self._measure_stop = self._measure_future = self._measure_event = None if stop is None and future is None: return if stop is not None: stop[0] = True + # The stream waits on the Event, so setting it is what stops the + # dispatcher NOW rather than on its next 0.5 s poll. + if event is not None: + event.set() + # At INFO because "is it actually cancelling, or just restarting after + # it finishes?" is the one question this path raises and the one the + # code cannot answer by inspection. + log.info("DPC: cancelled the in-flight beam-shift pass") + if future is not None: try: if not future.done(): @@ -443,100 +660,11 @@ def _measure_failed(self, gen: int, exc: Exception) -> None: if self._closed or not self.still(gen): log.debug("DPC measure abandoned after close/supersede: %s", exc) return + # With the traceback: the message alone names a symptom, and the pass + # runs on a worker, so the frames are gone by the time anyone looks. + log.exception("DPC beam-shift pass failed", exc_info=exc) emit_error(f"DPC: locating the direct beam failed: {exc}") - def _measure_progressive(self, signal, method, hw, region, gen, on_finish - ) -> bool: - """Stream the beam-shift pass per nav chunk. False → not applicable. - - Uses ``ComputeBackend.compute_chunks_progressive``, which dispatches one - task per nav chunk and calls back from a worker thread as each lands. - The lazy graph keeps the dataset's own nav chunking (no rechunk layer), - so a "chunk" here is a storage chunk — the streaming granularity matches - what the reader actually reads (Live-Display §1). - - Partial state is NaN, which every stage downstream already tolerates: - the plane fits mask on ``isfinite``, the rotation estimator drops - non-finite gradients, and the display paints non-finite black rather - than letting one poison the contrast. So the map genuinely fills in. - """ - backend = getattr(self.session, "compute_backend", None) - if backend is None or not hasattr(backend, "compute_chunks_progressive"): - return False - try: - graph = _dpc.beam_shift_graph(signal, method=method, - half_square_width=hw, region=region) - except Exception as e: - log.debug("building the DPC beam-shift graph failed: %s", e) - return False - if graph is None: # eager data — nothing to stream - return False - - ny, nx = int(graph.shape[0]), int(graph.shape[1]) - partial = np.full((ny, nx, 2), np.nan, dtype=np.float64) - total = max(1, len(graph.chunks[0]) * len(graph.chunks[1])) - done = [0] - # The cancel token travels WITH the dispatch: the backend checks it - # before each chunk submit and inside each chunk task, so superseding - # this pass stops it rather than letting it finish unread. - stop: list = [False] - self.shifts = partial - self._set_computing(True) - - def _on_chunk(chunk, slices): - """Worker thread — marshal the paint onto the main loop.""" - if stop[0] or self._closed or not self.still(gen): - return - try: - partial[slices] = np.asarray(chunk, dtype=np.float64) - except Exception as e: # pragma: no cover - log.debug("storing a DPC chunk failed: %s", e) - return - done[0] += 1 - n = done[0] - dispatch = getattr(self.session, "_dispatch_to_main", None) - paint = lambda: self._on_partial(gen, n, total) # noqa: E731 - dispatch(paint) if dispatch is not None else paint() - - try: - future = backend.compute_chunks_progressive(graph, 2, _on_chunk, - stopped_flag=stop) - except Exception as e: - log.debug("progressive DPC dispatch failed (%s) — running in one " - "pass instead", e) - return False - self._track_measure(stop, future) - - def _settled(fut): - self._retire_measure(stop) - try: - result = fut.result() - except concurrent.futures.CancelledError: - # Cancelled deliberately (superseded, or the caret/tree closed). - # Not a failure and not the user's problem — say nothing. - self._set_computing(False) - return - except Exception as e: - if stop[0]: # torn down mid-pass; same story - self._set_computing(False) - return - self._measure_failed(gen, e) - self._set_computing(False) - return - dispatch = getattr(self.session, "_dispatch_to_main", None) - finish = lambda: on_finish(result) # noqa: E731 - dispatch(finish) if dispatch is not None else finish() - - future.add_done_callback(_settled) - return True - - def _on_partial(self, gen: int, done: int, total: int) -> None: - """Repaint from what has landed so far (main thread).""" - if self._closed or not self.still(gen): - return - emit_progress(done, total, "DPC: locating the direct beam") - self.refresh() - def _set_computing(self, computing: bool) -> None: """Drive the window's "Calculating…" overlay. Every True is paired.""" try: @@ -794,160 +922,169 @@ def hide_corner_boxes(self) -> None: # ── the beam region (one shape, two jobs) ──────────────────────────────── def ensure_region_defaults(self) -> None: - """Fill in radii the first time the region is switched on. + """Fill in the radii the first time a dataset is open. They cannot be declared in ``DEFAULTS`` because a sensible radius is a - fraction of the DETECTOR, whose size is not known until a dataset is - open. + fraction of the DETECTOR, whose size is not known until then — which is + also why ``BeamRegion.active`` still exists with the "off" shape gone. """ p = self.params - if str(p.get("beam_shape", "off")) == "off" or float(p.get("beam_r") or 0) > 0: + if float(p.get("beam_r") or 0) > 0: return d = _dpc.default_beam_region(self._sig_shape(), str(p["beam_shape"])) p["beam_cx"], p["beam_cy"] = d.cx, d.cy p["beam_r"], p["beam_r_inner"] = d.r, d.r_inner - def show_beam_region(self) -> None: - """Draw (or reshape) the draggable circle / ring on the pattern. + def sync_beam_region(self) -> None: + """Put the selector for the current shape on the pattern. - The SAME widget answers both of the Center step's questions: its area is - the centre-of-mass mask, and its centre is the Manual reference. Two - separate controls for one physical thing (where is the beam?) is what - this replaces. + The beam region is a :class:`~spyde.drawing.selectors.CircleSelector` / + :class:`~spyde.drawing.selectors.AnnularSelector` — the same selectors a + virtual image puts on the same plot — so it inherits the whole live + path: every pointer frame submits to the one serial navigator + dispatcher, a newer position replaces a still-queued older one, and a + trailing settle re-fires once motion stops. It used to be a raw widget + with a hand-rolled debounce, which is the only reason the map did not + track the region the way a virtual image tracks its detector ROI. - Switching shape rebuilds the widget — a circle and an annulus are - different anyplotlib widget types, so there is nothing to mutate. + Switching shape REBUILDS it — a circle and an annulus are different + anyplotlib widget types, so there is nothing to mutate. """ - plot2d = getattr(self.src_plot, "_plot2d", None) - shape = str(self.params.get("beam_shape", "off")) - if plot2d is None or shape == "off": - self.hide_beam_region() + from spyde.drawing.selectors import AnnularSelector, CircleSelector + + if self._closed or getattr(self.src_plot, "_plot2d", None) is None: return self.ensure_region_defaults() - r = self.region() - if self._beam_widget is not None: - if getattr(self._beam_widget, "_dpc_shape", None) == shape: - try: - kw = ({"cx": r.cx, "cy": r.cy, "r_outer": r.r, - "r_inner": r.r_inner} if shape == "ring" - else {"cx": r.cx, "cy": r.cy, "r": r.r}) - self._beam_widget.set(**kw) - return - except Exception as e: # pragma: no cover - log.debug("resizing the DPC beam region failed: %s", e) + region = self.region() + shape = region.shape + selector = self._beam_selector + if selector is not None and getattr(selector, "_dpc_shape", "") != shape: self.hide_beam_region() + selector = None + if selector is None: + cls = AnnularSelector if shape == "ring" else CircleSelector + try: + # No children: this selector drives a whole-scan re-measure, not + # a sliced child plot, so the work hangs off `index_hooks` — the + # same seam the vector overlays use. `_run_update`'s child loop + # is then a no-op and its geometry de-duplication still applies. + selector = cls(parent=self.src_plot, children=[], + update_function=[], color=_BEAM_COLOR) + except Exception as e: # pragma: no cover + log.debug("building the DPC beam selector failed: %s", e) + return + selector._dpc_shape = shape + selector.index_hooks.append(self._on_region_moved) + self._beam_selector = selector + self._write_region_to_widget(region) + + def _write_region_to_widget(self, region) -> None: + """Push *region* onto the widget (a typed radius, a shape rebuild). + + Safe to call from anywhere: unlike the old raw widget this does NOT + re-enter synchronously. ``set`` fires ``pointer_move``, but the selector + answers that by submitting to the dispatcher, so the read-back lands on + another thread and one write cannot recurse into another. + """ + widget = getattr(self._beam_selector, "roi", None) + if widget is None: + return try: - if shape == "ring": - w = plot2d.add_annular_widget(cx=r.cx, cy=r.cy, r_outer=r.r, - r_inner=r.r_inner, - color=_BEAM_COLOR) - else: - w = plot2d.add_circle_widget(cx=r.cx, cy=r.cy, r=r.r, - color=_BEAM_COLOR) - w._dpc_shape = shape - from spyde.drawing.selectors.base_selector import event_handler_fn - handler = event_handler_fn(lambda event: self._on_region_drag(event)) - w.add_event_handler(handler, "pointer_move", "pointer_up") - self._beam_widget, self._beam_handler = w, handler + widget.set(**({"cx": region.cx, "cy": region.cy, + "r_outer": region.r, "r_inner": region.r_inner} + if region.shape == "ring" + else {"cx": region.cx, "cy": region.cy, "r": region.r})) except Exception as e: # pragma: no cover - log.debug("adding the DPC beam region failed: %s", e) + log.debug("writing the DPC beam region to its widget failed: %s", e) def hide_beam_region(self) -> None: - """Widgets have no ``remove()``, only ``hide()`` (same as CZB's).""" - if self._beam_widget is not None: - try: - self._beam_widget.hide() - except Exception as e: # pragma: no cover - log.debug("hiding the DPC beam region failed: %s", e) - self._beam_widget = self._beam_handler = None - - def _on_region_drag(self, event=None) -> None: - """Read the widget back, echo its geometry, and on RELEASE re-measure. - - Everything expensive waits for ``pointer_up``. A drag frame only reads - the widget and echoes numbers the caret already has in hand. - - The brightness readout is what makes this necessary. It reads a frame, - which on a lazy signal is a dask compute, and one per pointer frame - queues work faster than it drains — the caret's own radius then keeps - climbing for a while after the pointer stops, because the messages - behind it are still landing. Same reason the Fit caret sends its state - on release only (see ``background_action._on_window_drag``). - - RE-ENTRANCY GUARD: anyplotlib ``Widget.set()`` fires ``pointer_move`` - unconditionally — even on a no-change write — so anything here that - writes back to the widget re-invokes this handler synchronously. The - same recursion Crop and CZB both hit; compare-before-set is NOT enough. + """Take the selector off the pattern (shape change, or teardown).""" + selector = self._beam_selector + self._beam_selector = None + if selector is None: + return + try: + selector.close() + except Exception as e: # pragma: no cover + log.debug("closing the DPC beam selector failed: %s", e) + + def _on_region_moved(self, indices=None) -> None: + """The region moved — track it and re-measure. On the dispatcher thread. + + This is the virtual-image update function's job, in the virtual-image + update function's place: the selector fires it once per COMMITTED + position (repeats at the same geometry are de-duplicated, and a + superseded position was dropped from the pending slot before it ever + ran), and it cancels the compute it replaces and starts a new one. + There is no pacing on top of that — the dispatcher's latest-wins + coalescing IS the pacing, exactly as it is for a virtual image. """ - w = self._beam_widget - if w is None or self._beam_dragging or self._closed: + if self._closed: + return + widget = getattr(self._beam_selector, "roi", None) + if widget is None: return - self._beam_dragging = True try: - self.params["beam_cx"] = float(w.cx) - self.params["beam_cy"] = float(w.cy) + self.params["beam_cx"] = float(widget.cx) + self.params["beam_cy"] = float(widget.cy) if str(self.params.get("beam_shape")) == "ring": - self.params["beam_r"] = float(w.r_outer) - self.params["beam_r_inner"] = float(w.r_inner) + self.params["beam_r"] = float(widget.r_outer) + self.params["beam_r_inner"] = float(widget.r_inner) else: - self.params["beam_r"] = float(w.r) + self.params["beam_r"] = float(widget.r) except Exception as e: # pragma: no cover log.debug("reading the DPC beam region failed: %s", e) - finally: - self._beam_dragging = False - released = str(getattr(event, "event_type", "") or "") == "pointer_up" - self.emit_region(with_brightness=released) - if released: - self.arm_region_settle() - - def arm_region_settle(self) -> None: - """(Re)start the debounce that re-measures once the drag stops. - - The region changes the centre of mass, so it can only take effect by - re-measuring the whole scan — the one expensive step. Doing that per - drag frame would make the widget unusable on any real dataset, so the - widget and the brightness readout track the pointer and the measurement - follows once motion stops. - """ - import threading - if self._settle_timer is not None: - try: - self._settle_timer.cancel() - except Exception as e: # pragma: no cover - log.debug("cancelling the DPC settle timer failed: %s", e) - if self._closed: return - - def _fire(): - self._settle_timer = None - if self._closed: - return - dispatch = getattr(self.session, "_dispatch_to_main", None) - if dispatch is not None: - dispatch(self.measure) - else: - self.measure() - - self._settle_timer = threading.Timer(_REGION_SETTLE_S, _fire) - self._settle_timer.daemon = True - self._settle_timer.start() + # Geometry only: the brightness readout reads a FRAME, and one dask + # compute per pointer frame queues work faster than it drains however + # far off-thread it runs. It is refreshed when the pass lands. + self.emit_region(with_brightness=False) + if self.region().as_dict() == self._measured_region: + # The region the running (or last) pass already used. Reached on + # every OPEN, because placing the selector writes its geometry and + # the widget reports that write as a move — which superseded the + # opening pass with an identical one, throwing away a whole scan's + # work and the progressive fill with it. Also covers a drag that + # returns to where it started. + return + self.measure() def emit_region(self, with_brightness: bool = True) -> None: """Live region geometry + how bright it is, for the caret's readout. - ``with_brightness=False`` re-sends the last measured value instead of - reading a frame for a new one. Mid-drag the geometry is what the caret - needs to track the pointer; the brightness costs a frame read and is - recomputed on release. Re-sending the last value rather than ``None`` - keeps the readout from blanking on every drag. + The geometry goes out NOW, carrying whatever brightness was last + measured — the caret has to track the pointer, and re-sending the last + value rather than ``None`` keeps the readout from blanking on every + drag. ``with_brightness`` additionally asks for a fresh reading, which + costs a frame read (a dask compute on a lazy signal), so it happens on a + worker and lands in a second message. Doing it inline stalled the event + loop on exactly the gesture that must not stall. """ region = self.region() + self._send_region(region) signal = self.signal - if with_brightness: - brightness = (_dpc.region_brightness(signal, region) - if signal is not None else float("inf")) + if not with_brightness or signal is None: + return + + def _work(): + return _dpc.region_brightness(signal, region) + + def _done(brightness): + # Drop a reading for a region the user has already moved on from — + # a stale multiplier next to a region it does not describe reads as + # a wrong answer, not a late one. + if self._closed or self.region().as_dict() != region.as_dict(): + return self._last_brightness = (None if not np.isfinite(brightness) else float(brightness)) + self._send_region(region) + + self.run_on_worker(_work, name="dpc-brightness", on_done=_done, + on_error=lambda e: log.debug( + "the DPC brightness probe failed: %s", e)) + + def _send_region(self, region) -> None: + """One ``dpc_region`` message for *region* + the last known brightness.""" emit({"type": "dpc_region", "window_id": self.caret_window_id, "result_window_id": self.window_id, **region.as_dict(), @@ -957,15 +1094,14 @@ def sync_overlays(self) -> None: """Show exactly the furniture the current state needs. The corner boxes belong to one Center MODE; the beam region does not — - it defines the centre of mass for every mode, so it is shown whenever - it is switched on. + it defines the centre of mass for every mode, so it is always there. """ mode = str(self.params["center_mode"]) if mode == "corners": self.show_corner_boxes() else: self.hide_corner_boxes() - self.show_beam_region() + self.sync_beam_region() # ── rotation ───────────────────────────────────────────────────────────── @@ -1156,7 +1292,7 @@ def _attach_wheel(tree): return commit_result_tree( self.session, title=f"DPC ({sym})", # The primary is the RGB direction+magnitude image, so label it that - # way — calling it "Ex" put a chip next to the real "Ex (MV/cm)" + # way — calling it "Ex" put a chip next to the real "Ex (MV/cm)" # view claiming to be the same map. primary=r.rgb, primary_label=f"{sym} direction", views=[(titles[c], r.component(c)) for c in _dpc.COMPONENTS], @@ -1174,7 +1310,7 @@ def _attach_wheel(tree): on_tree=_attach_wheel, ) - # ── teardown ───────────────────────────────────────────────────────────── + # ── teardown ───────────────────────────────────────────────────────────── def remove(self) -> None: """Tear down everything the wizard added. Idempotent — re-entry through @@ -1186,14 +1322,9 @@ def remove(self) -> None: # "nobody is waiting for this any more", and a beam-shift pass reads the # whole scan — it must not keep running for a wizard that is gone. self._cancel_measure() - # Cancel the drag debounce FIRST: a timer that fires after teardown - # would re-measure a torn-down wizard on a worker thread. - if self._settle_timer is not None: - try: - self._settle_timer.cancel() - except Exception as e: # pragma: no cover - log.debug("cancelling the DPC settle timer failed: %s", e) - self._settle_timer = None + # The selector owns the only remaining drag timer (its settle re-fire) + # and cancels it in `close`; `_closed` is already True, so a hook that + # fires in the meantime bails out on its own. self.hide_corner_boxes() self.hide_beam_region() if self.window_id is not None: @@ -1211,6 +1342,12 @@ def remove(self) -> None: self.window_id = self.plot = self.wheel = None if getattr(self.tree, "_dpc_wizard", None) is self: self.tree._dpc_wizard = None + # Last, and without waiting: a job still on the lane has already been + # stopped by the cancel above and returns on its next token check, but + # `remove` runs on the event loop and must not block on it. + lane, self._lane = self._lane, None + if lane is not None: + lane.shutdown(wait=False) def _tree_title(tree) -> str: @@ -1218,8 +1355,6 @@ def _tree_title(tree) -> str: return str(tree.root.metadata.General.title) or "untitled" except Exception: # pragma: no cover return "untitled" - - # ── toolbar entry (ActionContext convention: fn(ctx, ...)) ──────────────────── def dpc(ctx, action_name: str = "DPC", **params) -> None: @@ -1320,24 +1455,21 @@ def dpc_set_center(session, plot, payload) -> None: def dpc_set_beam(session, plot, payload) -> None: - """Beam region: switch between off / circle / ring, or set its geometry. + """Beam region: switch circle/ring, or type a radius. - Changing the region changes the CENTRE OF MASS, so it can only take effect - by re-measuring — which this does immediately for a discrete change (a - shape toggle, a typed radius). A DRAG goes through the debounce instead - (``_on_region_drag``), because the whole scan cannot be re-measured per - pointer frame. + This only writes the geometry onto the widget. The re-measure follows from + the widget the same way it does for a drag — ``sync_beam_region`` pushes the + value, the selector reports the move, and ``_on_region_moved`` decides. One + path for "the region changed", whether a pointer or a number moved it; + measuring here as well would run the pass twice for every typed radius. """ ctrl = _ctrl_for(session, plot, payload) if ctrl is None: return - before = ctrl.region().as_dict() ctrl.params.update(_clean(payload)) ctrl.ensure_region_defaults() ctrl.sync_overlays() - ctrl.emit_region() - if ctrl.region().as_dict() != before: - ctrl.measure() + ctrl.emit_region(with_brightness=False) def dpc_pick_center(session, plot, payload) -> None: diff --git a/spyde/tests/migrated/test_dpc.py b/spyde/tests/migrated/test_dpc.py index 886f87fc..84bfacbd 100644 --- a/spyde/tests/migrated/test_dpc.py +++ b/spyde/tests/migrated/test_dpc.py @@ -442,16 +442,39 @@ def test_an_explicit_center_still_wins_over_the_region(self): def test_region_round_trips_through_a_dict(self): r = dpc.BeamRegion("ring", 1.5, 2.5, 9.0, 3.0) assert dpc.BeamRegion.from_dict(r.as_dict()) == r - assert dpc.BeamRegion.from_dict(None).shape == "off" - assert dpc.BeamRegion.from_dict({"shape": "nonsense"}).shape == "off" - - def test_default_region_is_a_sane_starting_point(self): + assert dpc.BeamRegion.from_dict(None).shape == "circle" + assert dpc.BeamRegion.from_dict({"shape": "nonsense"}).shape == "circle" + assert dpc.BeamRegion.from_dict({"shape": "off"}).shape == "circle", \ + "a saved report from before the region was always-on must still load" + + def test_default_region_is_the_largest_that_fits(self): + """It must not CLIP the beam. The region is always on the pattern, so + this default is what an unattended scan is measured with, and a smaller + circle pulls the centroid towards its own centre — see + ``default_beam_region`` for the measured under-read.""" d = dpc.default_beam_region((256, 512)) assert (d.cx, d.cy) == (256.0, 128.0) # centred on the DETECTOR - assert d.r == 64.0 # a quarter of the short axis + assert d.r == 128.0 # HALF the short axis assert 0 < d.r_inner < d.r assert d.active + def test_the_default_region_does_not_bias_the_shift(self): + """The claim the radius rests on, checked rather than asserted: on a + centred disc displaced by a known amount, the default region returns the + SAME shift the whole frame does.""" + from scipy import ndimage as ndi + k = 32 + gy, gx = np.mgrid[0:k, 0:k].astype(np.float32) + region = dpc.default_beam_region((k, k)) + keep = region.mask((k, k)) + for true_shift in (1.5, 3.0, 4.5): + r = np.hypot(gx - (k / 2 - true_shift), gy - k / 2) + frame = np.clip((k * 0.22 - r) / (k * 0.05) + 0.5, 0, 1) + masked = dpc._com_shift_frame(frame, keep, k / 2, k / 2) + whole_cy, whole_cx = ndi.center_of_mass(frame) + assert masked[0] == pytest.approx(k / 2 - whole_cx, abs=0.02) + assert masked[0] == pytest.approx(true_shift, abs=0.05) + def test_the_region_lands_in_provenance(self): s, _k = self._scan() region = dpc.BeamRegion("ring", 40.0, 36.0, 12.0, 3.0) @@ -899,29 +922,87 @@ def test_eager_data_has_no_graph(self): s.set_signal_type("electron_diffraction") assert dpc.beam_shift_graph(s) is None - def test_the_kernel_only_ever_sees_one_frame(self): - """Streaming is per FRAME inside a chunk, so peak memory is a frame — - not a chunk, and certainly not the dataset.""" + def test_the_kernel_sees_one_chunk_and_allocates_only_the_result(self): + """Peak memory is a chunk — never the dataset — and the kernel is + VECTORISED over that chunk rather than called per frame. + + It used to be a Python call per frame with a ``scipy.ndimage`` call + inside it, which is why a pass over a real scan took most of a minute: + at tens of thousands of frames the per-call overhead IS the runtime. + One ``einsum`` per chunk measured 37x faster on a 64x64x64x64 scan and + returns bit-identical numbers (see + ``test_the_vectorised_kernel_matches_the_per_frame_reference``). + + What still has to hold is the memory rule: the kernel must not + materialise a float64 copy of the block it is handed. It contracts the + block against detector-sized weights instead, so the only thing it + allocates is the nav-sized result. + """ s, arr = self._lazy() - seen = {"max": 0, "n": 0} - real = dpc._com_shift_frame + seen = {"blocks": [], "n": 0} + real = dpc._com_shift_blocks - def spy(frame, **kw): + def spy(block, *a, **kw): seen["n"] += 1 - seen["max"] = max(seen["max"], np.asarray(frame).nbytes) - return real(frame, **kw) + seen["blocks"].append(np.asarray(block).nbytes) + out = real(block, *a, **kw) + assert out.nbytes < np.asarray(block).nbytes, \ + "the kernel returned something the size of its input" + return out - dpc._com_shift_frame = spy + dpc._com_shift_blocks = spy try: shifts = dpc.measure_beam_shifts( s, region=dpc.BeamRegion("circle", 18.0, 14.0, 8.0)) finally: - dpc._com_shift_frame = real - one_frame = arr.shape[2] * arr.shape[3] * arr.dtype.itemsize - assert seen["n"] == arr.shape[0] * arr.shape[1] - assert seen["max"] <= one_frame * 1.01 + dpc._com_shift_blocks = real + + chunk_bytes = max( + np.prod([c[0] for c in arr.chunks]) * arr.dtype.itemsize, 1) + assert seen["n"] == len(arr.chunks[0]) * len(arr.chunks[1]), \ + "the kernel ran per frame, not per chunk" + assert max(seen["blocks"]) <= chunk_bytes * 1.01, \ + "the kernel was handed more than one chunk" assert shifts.shape == (arr.shape[0], arr.shape[1], 2) + def test_the_vectorised_kernel_matches_the_per_frame_reference(self): + """The fast path and the reference must agree EXACTLY, not closely. + + ``_com_shift_frame`` is the documented equivalence to pyxem's + ``center_of_mass_from_image`` (``TestBeamRegion`` pins that), so the + vectorised kernel inherits that guarantee only for as long as the two + return the same bits. ``allclose`` would let a real drift through. + """ + rng = np.random.default_rng(0) + k = 16 + block = (rng.random((3, 4, k, k)) * 1000).astype(np.uint16) + region = dpc.BeamRegion("circle", k / 2, k / 2, k * 0.4) + keep = region.mask((k, k)) + + fast = dpc._com_shift_blocks( + block, dpc._region_weights(keep, (k, k)), k / 2, k / 2) + for iy in range(block.shape[0]): + for ix in range(block.shape[1]): + one = dpc._com_shift_frame(block[iy, ix], keep, k / 2, k / 2) + assert np.array_equal(fast[iy, ix], one), (iy, ix) + + def test_an_empty_region_reads_as_no_measurement(self): + """A region with nothing under it yields NaN, not a centred beam. + + Zero is the dangerous answer: it is exactly what an undeflected beam + looks like, so it would show as a real measurement of no field. + """ + k = 16 + block = np.zeros((2, 2, k, k), dtype=np.uint16) + block[1, 1, k // 2, k // 2] = 500 # one frame has signal + region = dpc.BeamRegion("circle", k / 2, k / 2, k * 0.4) + out = dpc._com_shift_blocks( + block, dpc._region_weights(region.mask((k, k)), (k, k)), + k / 2, k / 2) + assert np.isnan(out[0, 0]).all(), "an empty region reported a position" + assert np.isfinite(out[1, 1]).all(), "a real frame was dropped" + + def test_a_partial_field_survives_every_downstream_stage(self): """The whole point of streaming: NaN where nothing has landed yet must flow through centering, rotation and display without poisoning them.""" diff --git a/spyde/tests/migrated/test_dpc_action.py b/spyde/tests/migrated/test_dpc_action.py index eb7ecfcf..3b376332 100644 --- a/spyde/tests/migrated/test_dpc_action.py +++ b/spyde/tests/migrated/test_dpc_action.py @@ -270,23 +270,23 @@ def test_switching_mode_takes_the_previous_furniture_away(self, window): beam_shape="circle") nav = _navigator_plot(session) assert "dpc_corners" in _markers(nav._plot2d) - assert wiz._beam_widget is not None + assert wiz._beam_selector.roi is not None dpca.dpc_set_center(session, plot, {"center_mode": "manual"}) assert wiz._corner_mg is None assert "dpc_corners" not in _markers(nav._plot2d) - assert wiz._beam_widget is not None, \ + assert wiz._beam_selector.roi is not None, \ "the beam region is not owned by a Center mode" dpca.dpc_set_center(session, plot, {"center_mode": "none"}) - assert wiz._corner_mg is None and wiz._beam_widget is not None + assert wiz._corner_mg is None and wiz._beam_selector.roi is not None def test_the_region_centre_becomes_the_manual_reference(self, window): """Drag the region onto the beam and Manual is already answered — no second marker to place, and no way for the two to disagree.""" session, plot, _tree, wiz = _opened(window, center_mode="manual", beam_shape="circle") - assert wiz._beam_widget is not None + assert wiz._beam_selector.roi is not None wiz.params.update({"beam_cx": 20.0, "beam_cy": 12.0, "beam_r": 6.0}) dpca.dpc_pick_center(session, plot, {}) assert wiz.params["cx"] == 20.0 and wiz.params["cy"] == 12.0 @@ -305,9 +305,12 @@ def test_the_reference_follows_the_region_without_an_explicit_pick(self, window) assert wiz.manual_center() == (18.0, 14.0) assert wiz.reference()[..., 0] == pytest.approx(16.0 - 18.0) - def test_picking_with_no_region_errors_instead_of_guessing(self, window): - session, plot, _tree, _wiz = _opened(window, center_mode="none", - beam_shape="off") + def test_picking_before_the_region_has_a_radius_errors(self, window): + """There is no "off" shape any more, so the only way to have no usable + region is the moment before the detector size has filled the radii in — + which is what ``BeamRegion.active`` still guards.""" + session, plot, _tree, wiz = _opened(window, center_mode="none") + wiz.params["beam_r"] = 0.0 dpca.dpc_pick_center(session, plot, {}) assert any("beam region" in e for e in _errors(window["messages"])) @@ -338,69 +341,124 @@ def test_vacuum_before_a_dataset_is_picked_is_not_an_error(self, window): assert wiz.result is not None assert not _errors(window["messages"]) - def test_the_beam_region_widget_matches_the_shape(self, window): + def test_the_beam_region_selector_matches_the_shape(self, window): """Circle and ring are different anyplotlib widget TYPES, so switching - rebuilds rather than mutates — and the widget on screen must be the one - the mask is computed from.""" + rebuilds the SELECTOR rather than mutating it — and the widget on screen + must be the one the mask is computed from. + + The region is a real ``BaseSelector`` (the same one a virtual image puts + on this plot), which is what gives it the navigator's latest-wins + dispatch and settle re-fire instead of a hand-rolled debounce. + """ + from spyde.drawing.selectors import AnnularSelector, CircleSelector + session, plot, _tree, wiz = _opened(window, beam_shape="circle") - assert type(wiz._beam_widget).__name__ == "CircleWidget" + assert isinstance(wiz._beam_selector, CircleSelector) + assert type(wiz._beam_selector.roi).__name__ == "CircleWidget" assert wiz.region().shape == "circle" and wiz.region().r > 0 dpca.dpc_set_beam(session, plot, {"beam_shape": "ring"}) - assert _wait(lambda: type(wiz._beam_widget).__name__ == "AnnularWidget") + assert _wait(lambda: isinstance(wiz._beam_selector, AnnularSelector)) + assert type(wiz._beam_selector.roi).__name__ == "AnnularWidget" assert wiz.region().shape == "ring" assert 0 < wiz.region().r_inner < wiz.region().r - dpca.dpc_set_beam(session, plot, {"beam_shape": "off"}) - assert wiz._beam_widget is None and not wiz.region().active + def test_the_region_is_always_on_the_pattern(self, window): + """There is no "off". The region IS what the centre of mass is taken + over, so switching it off only meant taking the whole frame with no + handle to grab — a control whose useful setting was "not that one".""" + session, _plot, _tree, wiz = _opened(window) + assert wiz._beam_selector is not None + assert wiz.region().active, "the region opened without a usable radius" + assert "off" not in dpca._dpc.BEAM_SHAPES + assert dpca.DEFAULTS["beam_shape"] in dpca._dpc.BEAM_SHAPES def test_radii_are_filled_in_from_the_detector_size(self, window): """They cannot be declared in DEFAULTS — a sensible radius is a fraction - of a detector whose size is unknown until a dataset is open.""" - session, plot, _tree, wiz = _opened(window, beam_shape="off") - assert wiz.params["beam_r"] == 0.0 - dpca.dpc_set_beam(session, plot, {"beam_shape": "circle"}) + of a detector whose size is unknown until a dataset is open. Opening the + wizard is therefore what fills them in, since the region is always on.""" + session, plot, _tree, wiz = _opened(window) sy, sx = wiz._sig_shape() - assert wiz.params["beam_r"] == pytest.approx(0.25 * min(sy, sx)) + # The INSCRIBED circle: the region is always on, so its default is what + # an unattended scan is measured with, and a smaller one clips the beam + # and under-reads the shift. See dpc.default_beam_region for the numbers. + assert wiz.params["beam_r"] == pytest.approx(0.5 * min(sy, sx)) assert (wiz.params["beam_cx"], wiz.params["beam_cy"]) == (sx / 2, sy / 2) - def test_a_drag_frame_costs_nothing_and_the_release_pays(self, window): - """Every cost waits for ``pointer_up``. + def test_a_moved_region_re_measures_through_the_selector(self, window): + """The map tracks the region as it moves, the way a virtual image tracks + its detector ROI — and for the same reason: the region IS a + ``BaseSelector``, so every move goes through the navigator's serial + latest-wins dispatcher and cancels the compute it replaces. - Two of them. Re-measuring the scan is the obvious one. The other is the - brightness readout, which reads a frame — a dask compute on a lazy scan - — and firing one per pointer frame queues work faster than it drains, - so the caret's own radius keeps climbing after the pointer has stopped. - A drag frame must therefore do no reading at all, only echo geometry. + Waiting for ``pointer_up`` left the map frozen for the whole gesture + (measured in the real app: zero repaints across a 6.4 s drag). """ session, plot, _tree, wiz = _opened(window, beam_shape="circle") - measures = {"n": 0} - reads = {"n": 0} + passes: list = [] + wiz.measure = lambda **kwargs: passes.append(wiz.region().r) + + # Driven by moving the WIDGET, not by calling the hook: what is under + # test is that the selector carries a move all the way to a re-measure + # on its own, which is the whole point of it being a BaseSelector. + for r in (8.0, 9.0, 10.0, 11.0): # a drag, frame by frame + before = len(passes) + wiz._beam_selector.roi.set(r=r) + assert _wait(lambda: len(passes) > before, timeout=10.0), \ + f"moving the region to r={r} never re-measured" + + assert passes[-1] == pytest.approx(11.0), \ + "the last pass measured a region the widget had already moved off" + assert wiz.params["beam_r"] == pytest.approx(11.0), \ + "the widget geometry must track the pointer every frame" + + def test_a_superseded_pass_is_cancelled_not_left_to_finish(self, window): + """Each move cancels the pass it replaces. A beam-shift pass reads the + whole scan, so one nobody is waiting for costs the cluster the entire + dataset — the same contract ``virtual_image`` keeps.""" + session, plot, _tree, wiz = _opened(window, beam_shape="circle") + first = [False] + wiz._track_measure(first) + assert wiz._measure_stop is first + + wiz._beam_selector.roi.set(r=9.0) + assert _wait(lambda: first[0], timeout=10.0), \ + "moving the region left the previous pass running" + assert wiz._measure_stop is not first, \ + "the new pass did not take the slot from the one it superseded" + + + def test_moving_the_region_does_not_read_a_frame_for_the_brightness(self, window): + """The brightness readout reads a FRAME — a dask compute on a lazy scan + — so one per pointer frame queues work faster than it drains, and the + caret's own radius keeps climbing after the pointer stops. Re-measuring + has no such problem (it dispatches and returns, and a superseded pass is + cancelled), which is why the two are not on the same cadence: the + readout refreshes when a pass LANDS. + """ + import threading + import spyde.actions.dpc as dpc_mod - real_measure = dpc_mod.measure_beam_shifts + reads: list = [] real_brightness = dpc_mod.region_brightness - dpc_mod.measure_beam_shifts = lambda *a, **k: ( - measures.__setitem__("n", measures["n"] + 1) or real_measure(*a, **k)) dpc_mod.region_brightness = lambda *a, **k: ( - reads.__setitem__("n", reads["n"] + 1) or real_brightness(*a, **k)) + reads.append(threading.current_thread()) or real_brightness(*a, **k)) try: - for r in (8.0, 9.0, 10.0, 11.0): # a drag, frame by frame - wiz._beam_widget.set(r=r) - wiz._on_region_drag(_drag_frame()) - assert measures["n"] == 0, "a drag frame re-measured the whole scan" - assert reads["n"] == 0, \ - "a drag frame read a frame for the brightness readout" - assert wiz._settle_timer is None, "a drag frame armed the re-measure" - assert wiz.params["beam_r"] == pytest.approx(11.0), \ - "the drag must still track the widget, it just must not read" - - wiz._on_region_drag(_release()) - assert reads["n"] == 1, "the release did not refresh the brightness" - assert _wait(lambda: measures["n"] >= 1, timeout=10.0), \ - "the release never fired the re-measure" - assert measures["n"] == 1, "the settle should coalesce to ONE measure" + session, plot, _tree, wiz = _opened(window, beam_shape="circle") + wiz.measure = lambda **kwargs: None # not what this test is about + reads.clear() + for r in (8.0, 9.0, 10.0, 11.0): + wiz._beam_selector.roi.set(r=r) + wiz._on_region_moved() + assert reads == [], \ + "moving the region read a frame for the brightness readout" + + wiz.emit_region(with_brightness=True) # what a landed pass does + assert _wait(lambda: len(reads) >= 1, timeout=10.0), \ + "a landed pass never refreshed the brightness" + assert reads[0] is not threading.current_thread(), \ + "the brightness probe ran on the caller's thread, stalling the drag" finally: - dpc_mod.measure_beam_shifts = real_measure dpc_mod.region_brightness = real_brightness def test_a_measure_abandoned_by_close_does_not_shout(self, window): @@ -429,18 +487,22 @@ def test_a_superseded_measure_does_not_shout(self, window): wiz._measure_failed(stale, RuntimeError("boom")) assert len(_errors(window["messages"])) == before - def test_the_drag_debounce_cannot_fire_after_teardown(self, window): + def test_the_settle_re_fire_cannot_fire_after_teardown(self, window): """A timer that survives close would re-measure a torn-down wizard on a - worker thread.""" + worker thread. The timer belongs to the SELECTOR now, so closing the + wizard has to close the selector — which is the one thing that could + quietly stop happening when the region stopped being a raw widget.""" session, plot, _tree, wiz = _opened(window, beam_shape="circle") - wiz._on_region_drag(_release()) - assert wiz._settle_timer is not None + selector = wiz._beam_selector + selector.update_data() # arms the settle re-fire + assert _wait(lambda: selector._settle_timer is not None, timeout=5.0) dpca.dpc_close(session, plot, {}) - assert wiz._settle_timer is None and wiz._closed + assert selector._settle_timer is None and wiz._closed + assert wiz._beam_selector is None def test_the_region_is_echoed_back_for_the_caret(self, window): session, plot, _tree, wiz = _opened(window, beam_shape="circle") - wiz._on_region_drag() + wiz._on_region_moved() msgs = _of_type(window["messages"], "dpc_region") assert msgs, "no dpc_region echo reached the caret" last = msgs[-1] @@ -537,9 +599,13 @@ def test_a_lazy_scan_fills_in_progressively(self, window): each lands — the difference between a filling field and a spinner on a scan that takes minutes. - Asserted on the PROGRESS stream and on partial state, not on timing: a - single final repaint would emit one progress message and never expose a - half-NaN field. + Asserted on the PROGRESS stream, which reports what each landing chunk + triggered. NOT on how much of the field was finite when a repaint ran: + the repaints are marshalled to the event loop and the pass now finishes + in milliseconds, so every one of them can legitimately see a complete + field and still have been driven by a separate chunk. That is the + compute being fast, not the stream having collapsed — measuring it that + way made this test fail on a 37x speedup. """ import dask.array as da import hyperspy.api as hs @@ -583,16 +649,21 @@ def spy(self): dpca.DpcWizard.refresh = real_refresh assert seen_partial, "the map was never repainted" - # The whole claim: at least one repaint happened while the field was - # still incomplete. - assert any(0 < n < total_positions for n in seen_partial), \ - f"no partial repaint — the map only appeared at the end: {seen_partial}" + assert seen_partial[-1] == total_positions, \ + f"the streamed pass left holes in the field: {seen_partial}" - progress = [m for m in window["messages"] + # The whole claim: the map was repainted MORE THAN ONCE, once per nav + # chunk that landed, rather than a single paint at the end. + chunks = (nav // chunk) ** 2 + streamed = [m for m in window["messages"] if isinstance(m, dict) and m.get("type") == "progress" - and "DPC" in str(m.get("label", ""))] - assert len(progress) >= 2, f"progress was not streamed: {progress}" - assert progress[0]["total"] == (nav // chunk) ** 2, \ + and "DPC" in str(m.get("label", "")) + and m.get("total") == chunks] + assert len(streamed) >= 2, \ + f"the pass did not stream — one paint at the end: {streamed}" + assert any(0 < m["done"] < chunks for m in streamed), \ + f"no chunk reported partial progress: {streamed}" + assert streamed[0]["total"] == chunks, \ "progress should count NAV CHUNKS, matching the storage layout" def test_an_eager_scan_does_not_pretend_to_stream(self, window): @@ -831,11 +902,44 @@ def test_the_worker_never_gets_the_trees_own_signal(self, window): "the worker was handed the tree's live signal — two passes would race" assert handed_data is live.data, "the view must share the data buffer" - def test_overlapping_passes_get_separate_objects(self, window): - _tree, seen = self._measured_signals(window, opens=3) - assert len(seen) >= 2 - assert len({id(signal) for signal, _data in seen}) == len(seen), \ + def test_every_pass_gets_its_own_object_and_they_cannot_overlap(self, window): + """Two guarantees, and the second is what makes the first sufficient. + + Each pass is handed its own view, so two of them cannot read each + other's state. And they are set up on a SINGLE-thread lane, so two + set-ups never run at once — which is the stronger property, because + ``private_view`` is itself the operation that is unsafe to overlap + (hyperspy parks a length-1 placeholder on ``.data`` while it copies). + + Passes are run sequentially here rather than raced: a superseded pass is + now cancelled before the lane ever picks it up, so open/close/open no + longer produces two calls to compare. + """ + session, plot, _tree, wiz = _opened(window) + seen = [] + real = dpca._dpc.measure_beam_shifts + + def spy(signal, **kw): + seen.append(signal) + return real(signal, **kw) + + dpca._dpc.measure_beam_shifts = spy + try: + for _ in range(3): + before = len(seen) + wiz.measure() + assert _wait(lambda: len(seen) > before, timeout=30.0), \ + "a measure never reached the worker" + finally: + dpca._dpc.measure_beam_shifts = real + + live = dpca._current_signal(_signal_plot(session)) + assert all(s is not live for s in seen), \ + "a pass was handed the tree's live signal — two would race" + assert len({id(s) for s in seen}) == len(seen), \ "two measures shared one signal object" + assert wiz._pass_lane()._max_workers == 1, \ + "the pass lane must be serial — private_view cannot overlap itself" @pytest.mark.usefixtures("_capture_module_emit") From 46c85c4c4e5b619be870da8ac0b578ab614a808c Mon Sep 17 00:00:00 2001 From: Carter Francis Date: Thu, 27 Aug 2026 08:06:56 -0500 Subject: [PATCH 2/5] docs(dpc): changelog fragments for the live region --- upcoming_changes/+dpc-live-region.bugfix.rst | 4 ++++ upcoming_changes/+dpc-region-always-on.api_change.rst | 4 ++++ 2 files changed, 8 insertions(+) create mode 100644 upcoming_changes/+dpc-live-region.bugfix.rst create mode 100644 upcoming_changes/+dpc-region-always-on.api_change.rst diff --git a/upcoming_changes/+dpc-live-region.bugfix.rst b/upcoming_changes/+dpc-live-region.bugfix.rst new file mode 100644 index 00000000..ea923bb0 --- /dev/null +++ b/upcoming_changes/+dpc-live-region.bugfix.rst @@ -0,0 +1,4 @@ +The DPC field map now recomputes as the beam region is dragged, instead of +staying frozen until the pass before it had finished. The beam region is a +real selector, so a superseded measurement is cancelled rather than left to +run, and the centre of mass is ~37x faster. diff --git a/upcoming_changes/+dpc-region-always-on.api_change.rst b/upcoming_changes/+dpc-region-always-on.api_change.rst new file mode 100644 index 00000000..c0ed3355 --- /dev/null +++ b/upcoming_changes/+dpc-region-always-on.api_change.rst @@ -0,0 +1,4 @@ +The DPC beam region no longer has an "off" setting — it is what the centre of +mass is taken over, so it is always on the pattern. Its default radius is now +half the shorter detector axis rather than a quarter: with the region always +on, a smaller default clipped the beam and under-read every field. From 902293635a0819c32761b4094637cc666adae81e Mon Sep 17 00:00:00 2001 From: Carter Francis Date: Mon, 31 Aug 2026 10:24:49 -0500 Subject: [PATCH 3/5] fix(dpc): the beam region kept the radius it was created with MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fixes, all found by running the drag spec this branch added. **The beam circle collapsed the moment the pointer entered the pattern.** `Widget.set` reaches the figure as a targeted message and deliberately never rewrites the panel's own state; the renderer RETAINS that state and re-applies it, so what the figure fell back to was the geometry the widget was BORN with — `image_width * 0.1`, a fifth of the radius the region opens at. The drag that followed moved that collapsed circle, and the field map sat still because a region already a few pixels wide barely changes it. `_write_region_to_widget` pushes the panel as well now, which is what anyplotlib's own `_sync_for_export` does for the same divergence before a snapshot. Measured on the drag spec: 0 map repaints before, 12-19 after. **The brightness probe measured on the tree's LIVE signal.** `inav` takes its slice by deep-copying the signal it slices, so the probe — on a worker — raced the pass lane copying that same object. The loser read a half-unset object, fell back, and handed the worker the live signal, which is exactly what `test_every_pass_gets_its_own_object_and_they_cannot_overlap` caught on the 4-core Windows runner (and reproduces here about 1 run in 30). The probe takes its own `private_view`, and `private_view` takes a lock so two copies of one signal can never overlap. **Landing chunks were tallied from several pool threads at once.** `landed[0] += 1` is three bytecodes; a lost update means the count never reaches `total` and the pass has no finish at all. `test_a_lazy_scan_fills_in_progressively` waited on the FIELD being complete, which the corner seed makes true before a single chunk has landed — so it read the progress stream three messages short of what the pass went on to emit. It waits for the pass to finish now. `dpc_live_region.spec.ts` aims with the region's own geometry (echoed to the caret as `data-cx`/`data-cy`) and anyplotlib's image->canvas transform, not the centroid of the circle's pixels: the region opens as the inscribed circle, so any nudge clips it and the centroid of what is left moves the OTHER way — a grab check built on it reads "never moved" for a drag that moved perfectly well. The harness's shared noise filter now drops the scheduler's "Couldn't gather keys ... 'forgotten'" line: that is one per superseded pass, i.e. the cancellation working. --- .../src/renderer/src/components/DpcWizard.tsx | 13 +- electron/tests/_harness.cjs | 10 +- electron/tests/dpc_live_region.spec.ts | 111 ++++++++++++------ spyde/actions/dpc.py | 24 +++- spyde/actions/dpc_action.py | 34 +++++- spyde/tests/migrated/test_dpc_action.py | 86 +++++++++++--- .../+dpc-beam-region-collapse.bugfix.rst | 5 + 7 files changed, 217 insertions(+), 66 deletions(-) create mode 100644 upcoming_changes/+dpc-beam-region-collapse.bugfix.rst diff --git a/electron/src/renderer/src/components/DpcWizard.tsx b/electron/src/renderer/src/components/DpcWizard.tsx index dbb38b72..e0b0e46f 100644 --- a/electron/src/renderer/src/components/DpcWizard.tsx +++ b/electron/src/renderer/src/components/DpcWizard.tsx @@ -642,16 +642,25 @@ function Info({ text, testid }: { text: string; testid: string }) { */ function BeamReadout({ region, shape }: { region: Region | null; shape: BeamShape }) { const b = region?.brightness + // The region's geometry, as the BACKEND currently holds it. Not shown — it is + // already drawn on the pattern — but a drag test has no other truthful source + // for it: read off the figure's pixels, a circle that runs to the edge of the + // frame is clipped, and the centroid of what is left moves the wrong way. + const geom = { + 'data-cx': region ? region.cx.toFixed(3) : undefined, + 'data-cy': region ? region.cy.toFixed(3) : undefined, + 'data-r': region ? region.r.toFixed(3) : undefined, + } if (b == null || !Number.isFinite(b)) { return ( -
+
Drag the {shape === 'ring' ? 'ring' : 'circle'} onto the direct beam.
) } const good = b >= 2 return ( -
{b.toFixed(1)}× frame average diff --git a/electron/tests/_harness.cjs b/electron/tests/_harness.cjs index d6f9aa65..ab31d32f 100644 --- a/electron/tests/_harness.cjs +++ b/electron/tests/_harness.cjs @@ -542,7 +542,15 @@ function backendErrorLines(backendOrLines) { // full-path colon form `dbus/bus.cc:405]` (format changed upstream, so an // Electron bump must not silently turn infrastructure noise back into // "backend errors"). - && !/:(ERROR|FATAL):[a-z_0-9/]+\.(cc|mm)[(:]\d+[)\]]/.test(l)) + && !/:(ERROR|FATAL):[a-z_0-9/]+\.(cc|mm)[(:]\d+[)\]]/.test(l) + // A CANCELLED compute, reported by the scheduler at ERROR. Any spec that + // supersedes a whole-scan pass mid-flight (drag a virtual detector, drag + // the DPC beam region) produces one per superseded pass: the client is + // still gathering the result of a graph whose keys the scheduler has just + // forgotten. It is the cancellation WORKING — the alternative, letting the + // pass run to the end, is the bug. Deliberately narrow: only the gather of + // a key in a released state, so a genuine worker loss still fails the spec. + && !/Couldn't gather keys.*'(forgotten|cancelled|released)'/.test(l)) } /** diff --git a/electron/tests/dpc_live_region.spec.ts b/electron/tests/dpc_live_region.spec.ts index 9ff81c79..2cda0bfa 100644 --- a/electron/tests/dpc_live_region.spec.ts +++ b/electron/tests/dpc_live_region.spec.ts @@ -119,17 +119,18 @@ test('the DPC field map tracks the beam region while it is dragged', async () => return parts.join('|') } - /** - * Where the beam circle is on screen, from its colour (#94e2d5). - * - * The ring is symmetric, so the centroid of its teal pixels IS its centre — - * which is where the drag handle sits. Guessing a point inside the circle - * instead grabs nothing: the cursor readout still tracks, so the figure - * looks driven while the circle stays put, and the run reports "0 frames" - * for a drag that never happened. That is indistinguishable from the bug, - * which is why the grab below is verified rather than assumed. - */ - const beamCentre = async () => { + /** Where the region IS, as the backend holds it — echoed to the caret on + * every committed move. */ + const regionGeometry = async () => { + const el = sig.getByTestId('dpc-beam-readout') + const [cx, cy] = await Promise.all([ + el.getAttribute('data-cx'), el.getAttribute('data-cy'), + ]) + return cx == null || cy == null + ? null : { cx: Number(cx), cy: Number(cy) } + } + + const figureFrame = async () => { const host = await sig.elementHandle() if (!host) return null for (const frame of page.frames()) { @@ -138,45 +139,77 @@ test('the DPC field map tracks the beam region while it is dragged', async () => const inside = await host.evaluate( (w, f) => w.contains(f as Node), el).catch(() => false) if (!inside) continue - const hit = await frame.evaluate(() => { - let sx = 0, sy = 0, n = 0 - for (const c of Array.from(document.querySelectorAll('canvas'))) { - const cv = c as HTMLCanvasElement - const g = cv.getContext('2d') - if (!g || !cv.width) continue - const d = g.getImageData(0, 0, cv.width, cv.height).data - const box = cv.getBoundingClientRect() - const kx = box.width / cv.width, ky = box.height / cv.height - for (let i = 0; i < d.length; i += 4) { - const r = d[i], gg = d[i + 1], b = d[i + 2] - if (!(r > 110 && r < 175 && gg > 200 && b > 185 && b < 240)) continue - const px = (i / 4) % cv.width, py = Math.floor((i / 4) / cv.width) - sx += box.left + px * kx; sy += box.top + py * ky; n++ - } - } - return n > 8 ? { x: sx / n, y: sy / n } : null - }).catch(() => null) - if (hit) { - const fb = await el.boundingBox() - return { x: (fb?.x ?? 0) + hit.x, y: (fb?.y ?? 0) + hit.y } - } + const ok = await frame.evaluate( + () => !!(window as any).__apl_imgToCanvas + && !!(window as any)._aplTiming).catch(() => false) + if (ok) return { frame, el } } return null } + /** + * The region's centre, in PAGE coordinates. + * + * Both halves come from something authoritative rather than from the + * picture: the geometry is the backend's own and the image→canvas transform + * is anyplotlib's own (`__apl_imgToCanvas`, published for exactly this). + * + * NOT the centroid of the circle's teal pixels, which is what this did + * first. The region opens as the INSCRIBED circle, so a nudge in any + * direction runs it off the frame; the visible arc is then clipped on that + * side and its centroid moves the OTHER way. A grab check built on it reads + * "never moved" for a drag that moved perfectly well — indistinguishable + * from the bug this spec exists to catch. And aiming has to be exact + * either way: the centre hot-spot is 9 px, and a miss lands on the radius + * handle or on nothing. + */ + const centrePoint = async () => { + const g = await regionGeometry() + const f = await figureFrame() + if (!g || !f) return null + const hit = await f.frame.evaluate(([x, y]: number[]) => { + const w: any = window + for (const id of Object.keys(w._aplTiming || {})) { + const c = w.__apl_imgToCanvas(id, x, y) + if (!c) continue + // The panel's own canvas — the biggest one, since the axis strips, + // the colour bar and the status line are canvases too. The image, + // overlay and marker layers share its box, which is the box + // `__apl_imgToCanvas` returns coordinates in. + const cv = Array.from(document.querySelectorAll('canvas')) + .map((c2) => (c2 as HTMLCanvasElement).getBoundingClientRect()) + .sort((a, b) => b.width * b.height - a.width * a.height)[0] + if (!cv) return null + return { x: cv.left + c[0], y: cv.top + c[1] } + } + return null + }, [g.cx, g.cy]).catch(() => null) + if (!hit) return null + const fb = await f.el.boundingBox() + return { x: (fb?.x ?? 0) + hit.x, y: (fb?.y ?? 0) + hit.y } + } + const sigBox = await sig.boundingBox() - const start = await beamCentre() - if (!sigBox || !start) throw new Error('could not find the beam circle') + const before = await regionGeometry() + // `_aplTiming` gains a panel's entry only once that panel has drawn twice, + // so on a slow machine the transform can simply not be there yet. Wait for + // it rather than reporting it as a missing circle. + let start = await centrePoint() + for (let i = 0; i < 40 && !start; i++) { + await page.waitForTimeout(500) + start = await centrePoint() + } + if (!sigBox || !before || !start) throw new Error('could not find the beam circle') - // Prove the grab took before measuring anything (see beamCentre). + // Prove the grab took before measuring anything (see centrePoint). let grabbed = false for (let attempt = 0; attempt < 4 && !grabbed; attempt++) { await page.mouse.move(start.x, start.y) await page.mouse.down() await page.mouse.move(start.x, start.y + 12) - await page.waitForTimeout(250) - const now = await beamCentre() - grabbed = !!now && Math.abs(now.y - start.y) > 3 + await page.waitForTimeout(400) + const now = await regionGeometry() + grabbed = !!now && Math.abs(now.cy - before.cy) > 0.5 if (!grabbed) { await page.mouse.up() await page.waitForTimeout(400) diff --git a/spyde/actions/dpc.py b/spyde/actions/dpc.py index 12f2e80f..cdd334a5 100644 --- a/spyde/actions/dpc.py +++ b/spyde/actions/dpc.py @@ -48,6 +48,7 @@ from __future__ import annotations import logging +import threading from dataclasses import dataclass, field as _dc_field import numpy as np @@ -283,6 +284,13 @@ def _com_shift_blocks(block, weights, cx0: float, cy0: float) -> np.ndarray: return out +#: Serialises :func:`private_view`. The copy it takes mutates the signal it is +#: copying FROM, so two threads copying one signal race — and the DPC wizard has +#: two that do: the pass lane sets a measure up while the brightness probe reads +#: a frame off the same signal. +_COPY_LOCK = threading.Lock() + + def private_view(signal): """A signal object the caller owns exclusively, over the SAME data buffer. @@ -313,12 +321,20 @@ def private_view(signal): Only the wrapper is duplicated — ``data`` is the same object, not a copy — so this stays cheap on a multi-GB lazy scan. - Call it on the DISPATCH thread, before handing the result to a worker: the - copy is itself a ``_deepcopy_with_new_data``, so it must not overlap another - operation on the signal it is copying. + Call it before handing the result to a worker, and call it for EVERY copy + of a shared signal — including the ones hyperspy takes on your behalf, which + is what ``inav`` slicing does. The copy is itself a + ``_deepcopy_with_new_data``, so two of them on one object corrupt each other + exactly as two ``map`` calls would; :data:`_COPY_LOCK` is what stops that, + and it only helps for callers that go through here. """ try: - return signal._deepcopy_with_new_data(signal.data) + # What the race looks like when it is lost: the second copy reads the + # first's half-unset object and raises `AttributeError: … has no + # attribute 'learning_results'`, which lands in the fallback below and + # hands the worker the LIVE signal — the one thing this exists to avoid. + with _COPY_LOCK: + return signal._deepcopy_with_new_data(signal.data) except Exception as e: # Worse to refuse to measure than to measure on the shared object. log.debug("private signal view failed (%s); using the live signal", e) diff --git a/spyde/actions/dpc_action.py b/spyde/actions/dpc_action.py index c0399ed0..f4a74d1c 100644 --- a/spyde/actions/dpc_action.py +++ b/spyde/actions/dpc_action.py @@ -453,6 +453,11 @@ def _begin_pass(self, stop, gen, method, hw, region, on_finish) -> None: # _cancel_measure, so there is one decision and two ways of hearing it. import threading event = threading.Event() + # Chunks land on several pool threads at once, so the tally is taken + # under a lock: `landed[0] += 1` is three bytecodes, and a lost update + # means the count never reaches `total` — the pass then has no finish at + # all, leaving the token registered and "Calculating…" up for good. + counted = threading.Lock() def _on_chunk(chunk, slices): """A dask callback thread — store, then marshal the repaint.""" @@ -463,8 +468,9 @@ def _on_chunk(chunk, slices): except Exception as e: # pragma: no cover log.debug("storing a DPC chunk failed: %s", e) return - landed[0] += 1 - n = landed[0] + with counted: + landed[0] += 1 + n = landed[0] if n >= total: # The last chunk IS the completed pass. Counting them is how the # finish is known: the handle's value is assembled client-side @@ -985,6 +991,19 @@ def _write_region_to_widget(self, region) -> None: re-enter synchronously. ``set`` fires ``pointer_move``, but the selector answers that by submitting to the dispatcher, so the read-back lands on another thread and one write cannot recurse into another. + + Then push the PANEL as well, which is not redundant. ``set`` reaches the + figure as a targeted widget message and deliberately never rewrites the + panel's own state (anyplotlib says so on ``Figure._push_widget``, and + reconciles the same divergence in ``_sync_for_export`` before any + snapshot). The renderer RETAINS the last panel state per figure and + re-applies it — so until something else repaints this plot, the geometry + the figure falls back to is the one the widget was BORN with. For the + beam region that is ``image_width * 0.1``, a fifth of the radius it + opens at: the circle silently collapsed to it the first time the pointer + entered the pattern, and the drag that followed moved that collapsed + circle. Pushing here makes the panel state agree with the widget, so + there is nothing stale to fall back to. """ widget = getattr(self._beam_selector, "roi", None) if widget is None: @@ -994,6 +1013,9 @@ def _write_region_to_widget(self, region) -> None: "r_outer": region.r, "r_inner": region.r_inner} if region.shape == "ring" else {"cx": region.cx, "cy": region.cy, "r": region.r})) + plot2d = getattr(widget, "_plot", None) + if plot2d is not None: + plot2d._push() except Exception as e: # pragma: no cover log.debug("writing the DPC beam region to its widget failed: %s", e) @@ -1065,9 +1087,15 @@ def emit_region(self, with_brightness: bool = True) -> None: signal = self.signal if not with_brightness or signal is None: return + # Its OWN view, for the reason `private_view` gives: the probe reads one + # frame with `inav`, and hyperspy takes an `inav` slice by deep-copying + # the signal it slices. Probing the LIVE signal on a worker therefore + # races the pass lane copying that same signal — and the loser of that + # race falls back to measuring on the shared object. + probe = _dpc.private_view(signal) def _work(): - return _dpc.region_brightness(signal, region) + return _dpc.region_brightness(probe, region) def _done(brightness): # Drop a reading for a region the user has already moved on from — diff --git a/spyde/tests/migrated/test_dpc_action.py b/spyde/tests/migrated/test_dpc_action.py index 3b376332..4a8f62b5 100644 --- a/spyde/tests/migrated/test_dpc_action.py +++ b/spyde/tests/migrated/test_dpc_action.py @@ -363,6 +363,37 @@ def test_the_beam_region_selector_matches_the_shape(self, window): assert wiz.region().shape == "ring" assert 0 < wiz.region().r_inner < wiz.region().r + def test_writing_the_region_also_refreshes_the_panel(self, window): + """A geometry write from Python must leave the FIGURE's own state + agreeing with the widget, not just the widget. + + ``Widget.set`` reaches the figure as a targeted message and deliberately + never rewrites the panel state — and the renderer retains that state and + re-applies it, so whatever the panel still holds is what the figure falls + back to. What it held was the geometry the widget was BORN with + (``image_width * 0.1``, a fifth of the radius the region opens at), and + the circle collapsed to it the moment the pointer entered the pattern. + Dragging the collapsed circle then measured a region a few pixels wide. + """ + _session, _plot, _tree, wiz = _opened(window, beam_shape="circle") + widget = wiz._beam_selector.roi + plot2d = widget._plot + pushes = [] + real_push = plot2d._push + + def counted_push(): + pushes.append(1) + real_push() + + plot2d._push = counted_push + try: + wiz._write_region_to_widget(wiz.region()) + finally: + del plot2d._push + assert pushes, "the panel kept the widget's birth geometry" + drawn = plot2d.to_state_dict()["overlay_widgets"] + assert [w["r"] for w in drawn] == [wiz.region().r] + def test_the_region_is_always_on_the_pattern(self, window): """There is no "off". The region IS what the centre of mass is taken over, so switching it off only meant taking the whole frame with no @@ -631,20 +662,45 @@ def spy(self): return real_refresh(self) total_positions = nav * nav + chunks = (nav // chunk) ** 2 + + def streamed(): + """The progress a LANDING CHUNK reported, one message each.""" + return [m for m in window["messages"] + if isinstance(m, dict) and m.get("type") == "progress" + and "DPC" in str(m.get("label", "")) + and m.get("total") == chunks] + + def finished(): + """`_finish` is the only thing that emits a bare ``"DPC"`` + progress, and it runs only once the LAST chunk is in.""" + return any(isinstance(m, dict) and m.get("type") == "progress" + and m.get("label") == "DPC" and m.get("total") == 1 + for m in window["messages"]) + dpca.DpcWizard.refresh = spy try: dpca.dpc_open(session, plot, {}) _wait(lambda: getattr(plot.signal_tree, "_dpc_wizard", None) is not None) - # Wait on the recorded PAINT, not on `wiz.shifts`: `_finish` assigns - # the shifts and only THEN calls `refresh`, so a wait on the array - # can return between the two — leaving the last entry in - # `seen_partial` a partial paint, and restoring the spy in `finally` - # before the full one was ever recorded. + # Wait for the PASS TO FINISH — not for the field to be complete. + # The opening pass seeds the whole field from the scan corners + # before a single chunk has landed, so "every position is finite" + # is already true at the FIRST repaint: waiting on it returned + # after one chunk of four and then read a progress stream three + # messages short of what the pass went on to emit. + assert _wait(finished, timeout=60.0), \ + f"the streamed pass never completed: {streamed()}" + # Every chunk but the last reports progress; the last one IS the + # finish. Both emits happen on their own chunk's callback thread, + # so the finish can be recorded a moment before the partial that + # precedes it — hence a wait rather than a bare read. + _wait(lambda: len(streamed()) >= chunks - 1, timeout=5.0) + # Only now is the last recorded paint the completed field. assert _wait(lambda: seen_partial and seen_partial[-1] == total_positions, - timeout=60.0), \ - f"the streamed pass never completed: {seen_partial}" + timeout=10.0), \ + f"the finished pass never repainted the whole field: {seen_partial}" finally: dpca.DpcWizard.refresh = real_refresh @@ -654,16 +710,12 @@ def spy(self): # The whole claim: the map was repainted MORE THAN ONCE, once per nav # chunk that landed, rather than a single paint at the end. - chunks = (nav // chunk) ** 2 - streamed = [m for m in window["messages"] - if isinstance(m, dict) and m.get("type") == "progress" - and "DPC" in str(m.get("label", "")) - and m.get("total") == chunks] - assert len(streamed) >= 2, \ - f"the pass did not stream — one paint at the end: {streamed}" - assert any(0 < m["done"] < chunks for m in streamed), \ - f"no chunk reported partial progress: {streamed}" - assert streamed[0]["total"] == chunks, \ + landed = streamed() + assert len(landed) >= 2, \ + f"the pass did not stream — one paint at the end: {landed}" + assert any(0 < m["done"] < chunks for m in landed), \ + f"no chunk reported partial progress: {landed}" + assert landed[0]["total"] == chunks, \ "progress should count NAV CHUNKS, matching the storage layout" def test_an_eager_scan_does_not_pretend_to_stream(self, window): diff --git a/upcoming_changes/+dpc-beam-region-collapse.bugfix.rst b/upcoming_changes/+dpc-beam-region-collapse.bugfix.rst new file mode 100644 index 00000000..6dfd5399 --- /dev/null +++ b/upcoming_changes/+dpc-beam-region-collapse.bugfix.rst @@ -0,0 +1,5 @@ +The DPC beam region no longer collapses the moment the pointer enters the +diffraction pattern. Its radius was being written to the widget alone, while +the figure's own state kept the radius the widget was created with — a fifth +as large — and reverted to it on the next redraw, so the region that was +actually measured was a few pixels wide and dragging it moved that. From 54c0b907b2ea5b47ac37abb019ecbb419b89eeaa Mon Sep 17 00:00:00 2001 From: Carter Francis Date: Mon, 31 Aug 2026 11:27:56 -0500 Subject: [PATCH 4/5] fix(dpc): a close mid-pass must not leave a window behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_finish` runs wherever the pass landed and `dpc_close` on the caller's thread, so the two interleave. A pass could clear its `_closed` check, be removed while it derived the map, and then open a window — after `remove` had been and gone, so nothing owned it and nothing ever closed it. That is `TestDoubleFire::test_open_close_open_leaves_exactly_one_wizard`, which failed on both macOS runners as "2 DPC windows left open, closed []". `remove` now claims the close and TAKES the window in one step, under the lock `_open_window` publishes through: a pass either sees the close and opens nothing, or gets in first and leaves a `window_id` to be closed. Nothing between the two. `refresh` bails on a closed wizard as well, so a removed wizard cannot repaint either. No changelog fragment: in the app `_dispatch_to_main` marshals `_finish` onto the event loop, which is the thread `dpc_close` runs on, so the two cannot interleave there. It is reachable where a pass lands on its own thread — which is what the test suite does, and what the docstring on `_finish` already claims to allow. The new test pins the timing instead of racing it: it gates `derive` until the close has run, which is the ordering that was failing on some machines and passing on others. --- spyde/actions/dpc_action.py | 49 ++++++++++++++++++------- spyde/tests/migrated/test_dpc_action.py | 38 +++++++++++++++++++ 2 files changed, 73 insertions(+), 14 deletions(-) diff --git a/spyde/actions/dpc_action.py b/spyde/actions/dpc_action.py index f4a74d1c..d6bab279 100644 --- a/spyde/actions/dpc_action.py +++ b/spyde/actions/dpc_action.py @@ -89,6 +89,7 @@ import concurrent.futures import logging import os +import threading import time import numpy as np @@ -278,6 +279,13 @@ def __init__(self, session, tree, src_plot, *, params: dict | None = None): self._measured_region = None # the region a pass last ran self._measure_event = None # …and what the stream waits on self._last_brightness = None # re-sent during a drag + # Guards the one hand-off `remove` and `_open_window` both reach for: + # whether this wizard is still open, and which window is its. They run + # on different threads (`_finish` lands wherever the pass did), so + # without it a pass that has already cleared its `_closed` check can + # publish a window a moment AFTER the close that was meant to prevent + # it — and `remove` has been and gone, so nothing ever closes it. + self._window_lock = threading.Lock() # ── the source signal ──────────────────────────────────────────────────── @@ -776,6 +784,8 @@ def derive(self) -> _dpc.DpcResult | None: def refresh(self) -> None: """Derive and repaint the map (opening the window on the first call).""" + if self._closed: + return result = self.derive() if result is None: return @@ -799,12 +809,17 @@ def _open_window(self, result: _dpc.DpcResult) -> None: emit_error(f"DPC: building the result window failed: {e}") log.exception("DPC window build failed") return - wid = int(self.session.next_window_id()) - keep_alive(wid, fig) - self.window_id, self.plot, self.wheel = wid, plot, wheel - emit({"type": "figure", "fig_id": fig_id, "window_id": wid, - "html": html, "title": self._title(), "is_navigator": False, - "aspect": _FIG_WIDTH / float(_FIG_HEIGHT)}) + # Claim and publish together: `remove` may have run while the figure + # was being built, and a window emitted after it is one nobody owns. + with self._window_lock: + if self._closed: + return + wid = int(self.session.next_window_id()) + keep_alive(wid, fig) + self.window_id, self.plot, self.wheel = wid, plot, wheel + emit({"type": "figure", "fig_id": fig_id, "window_id": wid, + "html": html, "title": self._title(), "is_navigator": False, + "aspect": _FIG_WIDTH / float(_FIG_HEIGHT)}) self.own_window(wid) #: The live window's title. Deliberately does NOT name the field type. @@ -1343,9 +1358,16 @@ def _attach_wheel(tree): def remove(self) -> None: """Tear down everything the wizard added. Idempotent — re-entry through remove → _forget_window → close → remove is a no-op.""" - if self._closed: - return - self._closed = True + # Claim the close and TAKE the window in one step, under the lock + # `_open_window` publishes through: a pass landing on another thread + # either sees the close and opens nothing, or gets in first and leaves + # a `window_id` here to be closed. Nothing between the two. + with self._window_lock: + if self._closed: + return + self._closed = True + window_id = self.window_id + self.window_id = self.plot = self.wheel = None # Stop the in-flight pass. Closing the caret is the clearest case of # "nobody is waiting for this any more", and a beam-shift pass reads the # whole scan — it must not keep running for a wizard that is gone. @@ -1355,19 +1377,18 @@ def remove(self) -> None: # fires in the meantime bails out on its own. self.hide_corner_boxes() self.hide_beam_region() - if self.window_id is not None: + if window_id is not None: forget = getattr(self.session, "_forget_window", None) if forget is not None: try: - forget(int(self.window_id)) + forget(int(window_id)) except Exception as e: # pragma: no cover log.debug("forgetting the DPC window failed: %s", e) else: # pragma: no cover - emit({"type": "window_closed", "window_id": int(self.window_id)}) + emit({"type": "window_closed", "window_id": int(window_id)}) reg = getattr(self.session, "_window_controllers", None) if isinstance(reg, dict): - reg.pop(int(self.window_id), None) - self.window_id = self.plot = self.wheel = None + reg.pop(int(window_id), None) if getattr(self.tree, "_dpc_wizard", None) is self: self.tree._dpc_wizard = None # Last, and without waiting: a job still on the lane has already been diff --git a/spyde/tests/migrated/test_dpc_action.py b/spyde/tests/migrated/test_dpc_action.py index 4a8f62b5..abe61827 100644 --- a/spyde/tests/migrated/test_dpc_action.py +++ b/spyde/tests/migrated/test_dpc_action.py @@ -32,6 +32,7 @@ """ from __future__ import annotations +import threading import time import numpy as np @@ -1023,6 +1024,43 @@ def test_open_close_open_leaves_exactly_one_wizard(self, window): f"{len(opened - closed)} DPC windows left open " f"(opened {sorted(opened)}, closed {sorted(closed)})") + def test_a_close_mid_pass_leaves_no_window_behind(self, window): + """A pass that has already cleared its ``_closed`` check must not go on + to open a window for a wizard that is gone by the time it gets there. + + ``_finish`` runs wherever the pass landed and ``dpc_close`` on the + caller's thread, so the two interleave. ``remove`` closes the window it + can SEE — and it sees none, because the pass opens it a moment later. + Nothing owns that window afterwards and nothing ever closes it. This is + the StrictMode open/close/open sequence with the timing pinned instead + of raced, which is what made it a failure on some machines only. + """ + session, plot, tree = _dataset(window) + entered, go = threading.Event(), threading.Event() + real_derive = dpca.DpcWizard.derive + + def gated_derive(self): + entered.set() + go.wait(30.0) + return real_derive(self) + + dpca.DpcWizard.derive = gated_derive + try: + dpca.dpc_open(session, plot, {}) + assert entered.wait(30.0), "the pass never reached the map" + dpca.dpc_close(session, plot, {}) + finally: + go.set() + dpca.DpcWizard.derive = real_derive + time.sleep(0.5) # let the released pass finish whatever it does + + opened = {m["window_id"] for m in _of_type(window["messages"], "figure") + if str(m.get("title", "")).startswith("DPC")} + closed = {m["window_id"] for m in _of_type(window["messages"], + "window_closed")} + assert not (opened - closed), \ + f"orphan DPC window(s) {sorted(opened - closed)} — closed already ran" + def test_re_opening_a_live_wizard_does_not_build_a_second(self, window): session, plot, tree, wiz = _opened(window) dpca.dpc_open(session, plot, {"rotation": 15.0}) From fc8555c0116b9bc0fbd4631243dd9b8d2fb401d8 Mon Sep 17 00:00:00 2001 From: Carter Francis Date: Mon, 31 Aug 2026 11:50:20 -0500 Subject: [PATCH 5/5] test(dpc): let the opening pass's brightness read land before measuring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test_moving_the_region_does_not_read_a_frame_for_the_brightness` clears its record of reads the instant `_opened` returns — and `_opened` returns from the line BEFORE the opening pass submits a brightness read of its own (`window_id` is set by the `refresh` just above it in `_finish`). Whether that read lands either side of the clear was a coin toss, and it came up tails on windows-latest-py3.12: one `dpc-brightness` thread in a list that is asserted empty, reading as a drag having read a frame. Wait for it to land and go quiet before clearing. --- spyde/tests/migrated/test_dpc_action.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spyde/tests/migrated/test_dpc_action.py b/spyde/tests/migrated/test_dpc_action.py index abe61827..b61079cc 100644 --- a/spyde/tests/migrated/test_dpc_action.py +++ b/spyde/tests/migrated/test_dpc_action.py @@ -478,6 +478,18 @@ def test_moving_the_region_does_not_read_a_frame_for_the_brightness(self, window try: session, plot, _tree, wiz = _opened(window, beam_shape="circle") wiz.measure = lambda **kwargs: None # not what this test is about + # The OPENING pass ends with a brightness read of ITS own, on a + # worker — and `_opened` returns from the line before it is + # submitted (`window_id` is set by the `refresh` just above it). So + # wait for that read to land and go quiet; clearing straight away + # leaves it to arrive inside the window measured below, where it is + # indistinguishable from a drag having read a frame. + assert _wait(lambda: reads, timeout=30.0), \ + "the opening pass never read the brightness" + settled = -1 + while settled != len(reads): + settled = len(reads) + time.sleep(0.2) reads.clear() for r in (8.0, 9.0, 10.0, 11.0): wiz._beam_selector.roi.set(r=r)