feat: per-region laser-AF offset (focus-map constant-z + laser AF)

[Alpaca233]

Summary

Lets each well/region be focused at its own offset from the single global laser-AF reference plane when laser autofocus (Reflection AF) and a constant-z focus map are used together — instead of every well being driven to that one shared reference plane.

Workflow: set the laser-AF reference once, define one focus point per well (the existing focus-map constant + Fit by Region case), and at each well record z. At capture, the system stores that region's laser-AF displacement (measure_displacement(), µm from the reference). During acquisition, laser AF at each FOV in a well drives to its stored offset (move_to_target(offset)) instead of the previously hardcoded move_to_target(0). This pins each well to its capture-time relationship with the reference plane, so laser AF maintains per-well focus against drift across a time-lapse.

Gated behind an explicit "Per-region laser AF offset" checkbox that is only active when Reflection AF is on and the focus map is constant + Fit by Region. The focus map's absolute z stays as the coarse pre-position; the existing per-channel z-offset composes additively (both unchanged).

What changed

• control/core/multi_point_worker.py — perform_autofocus targets region_laser_af_offsets.get(region_id, 0.0) instead of 0; new attribute unpacked from params.
• control/core/multi_point_controller.py — region_laser_af_offsets field + set_region_laser_af_offsets(); threaded through build_params; cleared per-run after build_params so stale offsets can't leak into a later acquisition from entry points that don't push them (fluidics / control server).
• control/core/multi_point_utils.py — AcquisitionParameters.region_laser_af_offsets: Dict[str, float] (defaults {} → current behavior).
• control/widgets.py (FocusMapWidget) — captures the offset on Add/Update-Z; the new checkbox + enable gating (Reflection AF + constant + Fit-by-Region); clears offsets when the laser-AF reference changes or the focus-point set changes; CSV export/import gains a back-compatible Offset_um column. Both multipoint widgets wire and push it at acquisition start (gated three ways); the third (fluidics) widget has no focus map and is untouched.
• control/gui_hcs.py — passes the laser-AF controller into FocusMapWidget (tolerates None when laser AF is unsupported).
• Docs — design spec + implementation plan under docs/superpowers/.

Backward compatibility

Empty region_laser_af_offsets (the default, and whenever the mode is off) reproduces the exact pre-feature behavior: every FOV targets displacement 0. move_to_target, the focus-map z-baking, and the per-channel z-offset logic are unchanged.

Testing

• New unit tests in tests/control/test_per_region_laser_af_offset.py (28): backend plumbing/apply, capture edge cases (no controller / no reference / NaN / out-of-range / disabled), reference-change invalidation, the constant-mode enable gating, and CSV round-trip + back-compat.
• Full suite: 1433 passed, 8 skipped, 1 xfailed; black --check clean.

Not yet done

• Interactive --simulation GUI smoke test is still pending (no display in the build env): live checkbox enable/disable, on-stage offset capture, reference-reset clearing, and CSV round-trip through the actual dialogs. The automated tests exercise the method logic via stubs but not the live Qt signal connections / acquisition-gate blocks.

Out of scope (noted)

MultiPointController.focus_map has the same pre-existing cross-entry-point staleness that the offsets had; only the new region_laser_af_offsets is reset per run here.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds support for per-region (well-level) laser reflection autofocus target offsets when using a constant-z focus map with “Fit by Region”, so each region can be maintained at its capture-time displacement relative to the global laser-AF reference plane.

Changes:

• Thread region_laser_af_offsets: Dict[str, float] through MultiPointController → AcquisitionParameters → MultiPointWorker, and use it as the laser-AF target in perform_autofocus.
• Extend FocusMapWidget to capture/clear/sync per-region offsets, gate the UI with a new checkbox, and persist offsets via an Offset_um CSV column (back-compatible read).
• Add unit tests covering backend apply path, capture edge cases, gating, invalidation, and CSV round-trip.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
software/control/core/multi_point_utils.py	Adds AcquisitionParameters.region_laser_af_offsets with default {}.
software/control/core/multi_point_controller.py	Stores/sets offsets, passes them into build_params, and clears controller state post-build to avoid stale reuse.
software/control/core/multi_point_worker.py	Uses per-region target offset in reflection AF path (move_to_target(target_um)).
software/control/widgets.py	Captures offsets in FocusMapWidget, adds UI gating + CSV persistence, and pushes offsets at acquisition start from both multipoint widgets.
software/control/gui_hcs.py	Passes the laser-AF controller into FocusMapWidget (supports None).
software/tests/control/test_per_region_laser_af_offset.py	New test suite for per-region offset behavior.
software/tests/control/test_MultiPointWorker_offsets.py	Updates AF stub to include region_laser_af_offsets for perform_autofocus.
software/docs/superpowers/specs/2026-06-18-per-region-laser-af-offset-design.md	Design spec documenting behavior and rationale.
software/docs/superpowers/plans/2026-06-18-per-region-laser-af-offset.md	Implementation plan and verification steps.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[hongquanli]

Code Review — PR #562: per-region laser-AF offset

Date: 2026-07-05
PR: #562 (per-region-laser-af-offset → master, author Alpaca233)
Scope: +1708/−77 across 10 files (~900 code lines; 1077 lines are in-tree planning docs)
Method: 8 parallel finder angles (~42 candidates) → dedup to 11 correctness mechanisms + 10 cleanup claims → 1-vote adversarial verification per item (12 verifiers). CI green; merges clean into master and into feat/squid-core-service.

Overview

The PR lets each well/region be focused at its own offset from the global laser-AF reference plane: offsets are captured in the Focus Map tab (constant + Fit-by-Region + a new checkbox) as live measure_displacement() readings, persisted in the focus-points CSV (Offset_um), pushed through MultiPointController.set_region_laser_af_offsets() into AcquisitionParameters, and consumed in the worker as move_to_target(offsets.get(region_id, 0.0)) (replacing the hardcoded move_to_target(0)). The concept is sound and the worker hook is exactly what the planned schema-v2 needs — but the feature as shipped is defeated by its own safety check (finding 1).

Correctness findings (most severe first)

1. multi_point_worker.py:1162 / laser_auto_focus_controller.py:372-382 — nonzero AF targets always fail spot-alignment verification; the feature is inert for typical offsets. [CONFIRMED — BLOCKER]
   _verify_spot_alignment() runs unconditionally inside move_to_target(), crops the current image centered at the reference spot position (center_x = int(x_reference), :524), and computes a zero-lag correlation against the reference crop (:536, threshold 0.7). Driving displacement to target_um puts the spot target/pixel_to_um pixels off the crop center; for a ~10 px-wide spot, correlation collapses beyond ~5 px shift. Offsets beyond ≈2–5 µm fail essentially always (5 µm ≈ 12.5 px at 0.4 µm/px → correlation ≈ 0.11). On failure the z move is fully reverted (:381) — not even to the reference — AF returns False, the FOV's per-channel z-offsets are skipped too (_apply_channel_z_offset(af_succeeded=False)), and this repeats for every FOV in the region.
   Recommended fix (proposed by Hongquan, 2026-07-05): anchor closed-loop at the reference with move_to_target(0) — where verification is valid — then apply the region offset as an open-loop relative Z move, mirroring the existing per-channel z_offset_um pattern. Captured values stay valid (same quantity); drift still nulled per FOV; errors don't accumulate. Alternative: make verification target-aware (crop at x_reference + target/pixel_to_um) or verify against the detected spot centroid.

2. multi_point_controller.py:856 + widgets.py:6504/8869 — stale offsets leak into acquisitions that never opted in. [CONFIRMED]
   Both toggle_acquisition paths push offsets before their disk-space/RAM early returns, and the controller clears them only after build_params inside run_acquisition (returns at :683 validate_acquisition_settings, :776, :836 also precede the clear). The TCP control server (microscope_control_server.py:790/:1251) and the fluidics widget (widgets.py:9637) share the same controller instance and never push offsets; do_reflection_af is sticky. Sequence: GUI start aborts on the RAM dialog → offsets stranded → a later TCP/fluidics run over the same wells silently applies them. (Acquire-current-FOV is safe: it clears the AF flag and uses region "current".)
   Fix: clear (or snapshot-and-clear) at every consumption boundary — e.g. clear in the early-return branches, or make offsets a per-run argument instead of sticky controller state.

3. widgets.py:10807 + laser_auto_focus_controller.py:108 — a benign objective/profile switch silently wipes all captured offsets. [CONFIRMED]
   initialize_manual re-emits signal_reference_changed with an unchanged reference on every config reload (the comment says "Duplicate emits are fine — the signal is idempotent"; true for the old listener, false for the new one). _on_laser_af_reference_changed clears unconditionally, ignoring the payload. Trigger: objective A→B→A or a laser-AF settings tweak → on_settings_changed → load_cached_configuration → re-emit → all offsets gone, with only a status-line message. The bool payload cannot distinguish benign re-emits from a genuine re-reference, so the fix needs a distinct "reference invalidated" signal (or a reference-identity payload), not a listener guard.

4. widgets.py:10592-10598 — edit_current_point mutates a point without invalidating its offset. [CONFIRMED]
   Unlike update_current_z (re-captures), remove_current_point (syncs), and generate_grid (clears), the Edit dialog rewrites (region_id, x, y, z) and never touches region_laser_af_offsets — so the stale offset overrides the edited z at acquisition (laser AF wins over the focus-map z). Blind re-capture would be wrong (the stage isn't at the point during the dialog); drop the region's offset and prompt for re-capture.

5. widgets.py:10729 + multi_point_worker.py:1159-1161 — a failed re-capture destroys a valid offset; nothing at run start flags missing offsets. [CONFIRMED]
   _capture_region_offset pops the existing offset before validation; the NaN path (:10752) then returns without re-storing — only a transient status label, no log (the design doc specified label + log). At acquisition start no one compares captured offsets to scan regions (the design doc's "optionally warn" item was not implemented), and the worker's get(region_id, 0.0) logs nothing for missing keys (if target_um: gates the log). Net: one well silently at the reference plane while its neighbors use offsets.

6. widgets.py:10964-10966 — import restores offsets with no reference present/matching check, and the CSV carries no reference identity. [CONFIRMED, medium]
   The capture path requires has_reference and warns on out-of-range; reference changes clear offsets. Import bypasses all three (and the status text doesn't even mention offsets were loaded). The reference persists across sessions via cache, so "restart → import yesterday's CSV → acquire" silently applies offsets measured against a plane that may since have been re-set.

7. widgets.py:10929-10969 — import failures are now silent (error-dialog regression). [CONFIRMED]
   On master the whole import flow sat in one try/except with an "Import Error" dialog. The PR guards only the parse; the region-mismatch dialog, state assignment, and display updates now run unguarded in a Qt slot — an exception there (empirically: nan/inf coordinates surviving the parse raise in get_FOV_pixel_coordinates) is caught by the app's excepthook and logged only, leaving the widget half-updated with no dialog. Regression is the lost notification (the half-update predates the PR).

8. widgets.py:10904-10908 — CSV import accepts nan/inf offsets, silently poisoning that region's laser AF. [PLAUSIBLE, downgraded]
   float() with only ValueError caught admits nan/inf; no isfinite anywhere downstream; move_to_target validates the measured displacement but never the target. The NaN travels to _move_z, where both actuator layers incidentally raise (piezo range check; round() conversion) one call before the MCU — so no hardware NaN move, but every FOV in the region fails AF with a traceback + bmp dump. Cheap fix: apply the capture path's isfinite + range check at import.

Refuted: the str-vs-numpy.int64 region-key mismatch (Load Coordinates path) — the silent-0.0 state is unreachable: the by-region flows that would mix key types are blocked loudly by a pre-existing region-set validation (fit_surface → "Region Mismatch"), and the surviving grid/Update-Z route keys offsets with the same raw ids the worker uses, so they match.

Reclassified (not bugs):

• Yaml save/replay omits offsets — deliberate scope boundary: the PR self-clears on non-GUI paths, CSV is the designed persistence, and yaml modeling is explicitly owned by the schema-v2 charter (well_z_offsets_um). At most a docstring note on the server's replay command.
• Export writes Offset_um when the checkbox is off / import re-arms — facts confirmed, but offsets are inert until the receiving session arms the triple gate; defensible "gate captures, don't destroy data" semantics. Polish: mention offsets in the import status message.

Cleanup appendix (all verified against the branch)

• widgets.py:10740 vs :4446 — sixth inline copy of the suspend-live/measure/resume pattern, without the try/except hardening the older copy gained; extract one helper (e.g. a LiveController context manager). The capture also runs synchronously on the GUI thread (stop_live → MCU waits + multi-frame average → start_live): ~0.5–2 s freeze per capture, ×N wells.
• multi_point_worker.py:1161 — per-FOV INFO log of a region-constant value (~24k lines for 96 wells × 25 FOVs × 10 t). Log once per region or demote to debug.
• widgets.py:10410/10769 — capture_laser_af_offset_enabled mirrors isChecked() (slot body is one assignment) while get_offsets_for_acquisition reads the checkbox directly; drop the flag (latent desync risk only).
• widgets.py:10783 — programmatic setChecked(False) when gating lapses forces the toggled slot's "incidental uncheck" carve-out comment; prefer setEnabled(False) + deriving active = enabled && checked.
• widgets.py:10803 — get_offsets_for_acquisition claims to be the gate but never re-verifies constant + Fit by Region; that invariant exists only as UI side-effects. Make the one method check all activation conditions.
• widgets.py:10750 — dead offset_um is None branch (measure_displacement is -> float, returns NaN on failure; same dead check exists at :4472).
• widgets.py:10754 vs laser_auto_focus_controller.py:366/:618 — the range rule is enforced in three places with one value but three reactions; expose a single predicate so the capture-time warning keeps predicting acquisition-time behavior.
• widgets.py:10786 — _clear_region_offsets wrapper vs direct dict rebinds elsewhere (mixed idiom); one reconcile-after-mutation helper would also have prevented finding 4.
• widgets.py:10874 — hand-rolled header.index() CSV parsing where the file's four other importers use pd.read_csv + column checks.
• Architecture (multi-angle convergence, not separately verified): the offsets dict shadows focus_points and is kept coherent by hand at every mutation site — widening the per-point record to (region_id, x, y, z, offset) would make findings 4 and 5 structurally impossible; likewise per-run acquisition arguments (vs sticky controller state) would eliminate finding 2's whole class.
• Conventions: software/docs/superpowers/plans/… (831 lines) and …/specs/…-design.md (246 lines) are planning documents; CLAUDE.md routes these to the AI-docs repo — they'll rot in-tree (they already contain line-number references). Move them; keep the user-facing docs/per-region-laser-af-offset.md, which is good.

Merge recommendation

Fix-then-merge — do not merge as-is. Finding 1 means the feature does not work for its intended use: any offset large enough to matter (≥ ~2–5 µm) is reverted by the spot-verification check on every FOV, and it also disables per-channel z-offsets for those FOVs as collateral. The good news: the recommended two-step fix (AF to reference, then open-loop relative offset — mirroring the existing per-channel offset pattern) is small, sidesteps the verification problem entirely instead of weakening the safety check, and keeps all captured data valid. Findings 2, 4, 5, 7, 8 are each small, localized fixes; finding 3 needs a modest signal redesign (distinct invalidation signal), and finding 6 wants at minimum an import-time status message + isfinite/range filter.

Strategic context: the feature is worth landing — its capture flow is the natural authoring tool for the per-well offsets that the acquisition-method schema-v2 (well_z_offsets_um) will declare, and its worker hook is the dependency that charter now cites. Suggested path: fix findings 1–2 (functional) + 4–5 + 7–8 (small) in this PR, spin the signal redesign (3) and import/reference identity (6) into fast follow-ups if the author prefers, move the planning docs to AI-docs, and land it before schema-v2 starts.

🤖 Generated with Claude Code

[Alpaca233]

Thanks for the thorough review — I independently re-verified every finding against the branch (adversarial per-finding checks, including a numerical simulation of the spot-alignment collapse for F1) and they all hold. Fixed in 9d33d549.

F1 (blocker) — move_to_target(nonzero) reverts z every FOV. Confirmed: _verify_spot_alignment() crops at x_reference and correlates zero-lag against the reference crop, so a deliberately-displaced target fails (~1.5–6 µm at 0.4 µm/px in simulation) → z reverted, af_succeeded=False, and the per-channel offset suppressed as collateral. Took your recommended fix: anchor closed-loop with move_to_target(0.0) (where verification is valid), then apply the region offset as an open-loop relative move (LaserAutofocusController.apply_relative_offset_um). Captured values stay valid; drift still nulled per FOV.

F2 (leak). run_acquisition now consumes-and-clears the offsets up-front (threaded explicitly into build_params), and both widgets push offsets only after all pre-flight checks pass — so a disk/RAM abort can't strand them on the shared controller for a later fluidics/TCP run.

F3. The FocusMapWidget listener now clears only when x_reference actually differs from the reference the offsets were captured against, so a benign objective/profile reload (which re-emits signal_reference_changed unchanged) no longer wipes valid offsets. Left the shared signal alone to avoid touching other listeners.

F4. edit_current_point drops the region's now-stale offset and prompts re-capture (not a blind re-capture — the stage isn't at the point).

F5. FocusMapWidget got a logger; capture failures now log (not just a transient status label), and get_offsets_for_acquisition warns at run start listing scan regions with no captured offset.

• One deliberate deviation: on a failed re-capture I drop the offset (loud log + status) rather than restore the prior value. Rationale: update_current_z has already moved the point's z, so a retained prior offset would be stale — dropping is the safe fallback (region → reference plane, never a wrong plane), and the new run-start warning makes a missing region visible. Happy to switch to restore-on-failure if you'd rather keep the last-good value.

F6 / F7 / F8 (import). Import applies offsets only when a reference is set (recording the reference they're tied to) and notes them in the status message; the whole import body is re-wrapped so any failure surfaces a dialog instead of a silent half-update; and _read_focus_points_csv now rejects non-finite coordinates and offsets.

Two small corrections from verification: edit_current_point is in control/widgets.py (not gui_hcs.py), and the capture path only warns on out-of-range offsets (still stores) — so import was simply gate-free rather than "weaker than capture's enforcement".

Not done here (per your split): the CSV reference-identity embed (F6's stronger form) and moving the planning docs out of the tree — I can spin those into follow-ups. Tests added for every fix (worker anchor+open-loop, build_params threading, reference-identity keep/clear, capture-failure logging, run-start missing-offset warning, non-finite CSV rejection). Note: a handful of test_MultiPointController / RAM-dialog tests fail in my local sandbox for environment reasons (RAM/disk/sim-config) — they fail identically on the clean branch and are unrelated to these changes; CI is green.

[hongquanli]

Re-verified the fix commit (9d33d549) — the F1 remedy is implemented exactly as intended (closed-loop anchor at the reference where verification is valid, then apply_relative_offset_um open-loop, gated on af_succeeded so a failed anchor never gets a blind offset stacked on it). CI green. Three reviewer decisions to close out:

1. Drop-on-failed-re-capture: agreed — the prior value is stale by construction once the point's z changed, so restoring it would silently target a wrong plane. But losing a previously captured offset must show a dialog, not just a log line + status text: when a capture attempt fails and an existing offset is removed as a result, pop a QMessageBox.warning telling the user which region lost its offset, why (spot not detected / no reference), and that the region will fall back to the reference plane unless re-captured. A destructive state change shouldn't be discoverable only from the log or the run-start warning. (First-time capture failures with nothing to lose can stay status-label-only.)

2. CSV reference identity — please do it in this PR rather than a follow-up. Without it, the import gate added here still accepts offsets measured against a different reference plane (export → re-set reference → import is a natural workflow). Embedding an identity (e.g. a reference id/timestamp written at set_reference time) in the CSV header and warning/refusing on mismatch closes finding 6 fully.

3. Planning docs — please move them out in this PR (software/docs/superpowers/plans/... and .../specs/...-design.md): repo convention routes design/plan documents to the AI-docs repo; in-tree they rot immediately (both already cite line numbers that this very commit shifted). The user-facing docs/per-region-laser-af-offset.md stays, it's good.

With those three, this looks ready to merge from my side.

🤖 Generated with Claude Code