Skip to content

feat: per-channel z-offset for laser autofocus#551

Draft
hongquanli wants to merge 43 commits into
masterfrom
feat/laser-af-channel-offset
Draft

feat: per-channel z-offset for laser autofocus#551
hongquanli wants to merge 43 commits into
masterfrom
feat/laser-af-channel-offset

Conversation

@hongquanli
Copy link
Copy Markdown
Contributor

Summary

Adds a per-channel z-offset (µm) for use with laser reflection autofocus. When laser AF is the active AF method, each acquisition channel can carry a saved offset measured from the laser AF reference plane and have it applied automatically during multi-point acquisition. Optionally, the same offset can be applied on channel switches in live view (absolute positioning, robust to manual z jogs).

Motivation: different fluorescence channels need different best-focus positions due to chromatic aberration and sample-induced effects (refractive index, coverslip / mounting variations). The offset is treated as sample-dependent, not a pure optical constant — LaserAutofocusSettingWidget gains a "Reset all channel offsets" button for the new-sample workflow.

What's new

  • Acquisition behavior (MultiPointWorker):
    • Delta-tracking helpers (_apply_channel_z_offset, _reset_channel_z_offset, _move_z_for_offset) that emit the minimum stage/piezo moves needed to reach each channel's offset relative to the AF reference.
    • Tail-correction in try/finally around the inner channel loop so the offset is always undone before move_z_for_stack / _last_time_point_z_pos / abort.
    • Belt-and-braces reset in handle_acquisition_abort.
    • Works for both stage path and piezo path (with range clamping + warning log).
    • Startup log summarising any non-zero offsets that won't be applied this run (laser AF off or checkbox off).
  • UI:
    • LiveControlWidget gains a hidden-by-default "Show Z-offset controls" row: spinbox (±50 µm), "Capture current" (reads measure_displacement()), "Reset", and "Apply on channel switch" (absolute positioning).
    • Three acquisition widgets (FlexibleMultiPointWidget, WellplateMultiPointWidget, MultiPointWithFluidicsWidget) gain an "Apply per-channel z-offset" checkbox via a shared _ApplyChannelOffsetMixin.
    • LaserAutofocusSettingWidget gains a "Reset all channel offsets" button.
  • Plumbing:
    • apply_channel_offset: bool = True on AcquisitionParameters; default keeps TCP/MCP callers backward-compatible.
    • New "ZOffset" setting key on ConfigRepository.update_channel_setting (with a location == "channel" dispatch branch).
    • New signal_reference_changed = Signal(bool) on LaserAutofocusController.
  • Pre-existing bug fix (M1 in design review): repository.py:761-782's "create objective config from general" path was silently dropping z_offset_um when constructing a new AcquisitionChannel. The fix is one line; the regression test in test_repository.py would catch any reintroduction.

Skipped on purpose

NapariLiveWidget was not updated — it is gated off by USE_NAPARI_FOR_LIVE_VIEW = False and never instantiated. A follow-up cleanup PR should remove the dead widget (notes left in worktrees/docs/2026-05-23-napari-live-widget-dead-code-cleanup.md).

Test plan

  • pytest software/tests/control/test_MultiPointWorker_offsets.py — 14 unit tests (delta tracking, stage + piezo paths, abort, z-stack invariant, _log_ignored_offsets)
  • pytest software/tests/control/test_LiveControlWidget_offset.py — 6 unit tests (absolute positioning, gating, exception path)
  • pytest software/tests/control/core/config/test_repository.py — added 3 tests (M1 regression + ZOffset round-trip + zero-value persistence)
  • pytest software/tests/control/test_MultiPointController.py — added 2 tests (default apply_channel_offset + override)
  • Full suite: 1310 passed, 6 failed (zero new failures — all 6 are pre-existing on master)
  • black --config software/pyproject.toml --check software/ — clean
  • Manual simulation smoke (display required): launch python3 main_hcs.py --simulation, walk through Show Z-offset toggle → Capture → Reset → run acquisition with two channels at distinct offsets → confirm stage moves match the design
  • Hardware verification — capture offsets on a real microscope and confirm chromatic-shift correction works as expected

Design docs

  • Design spec: software/docs/laser-af-channel-offset-design.md (committed)
  • Implementation plan: software/docs/laser-af-channel-offset-plan.md (committed)

🤖 Generated with Claude Code

hongquanli and others added 28 commits May 23, 2026 17:12
Specifies the data model (reuse AcquisitionChannel.z_offset_um), delta-tracking
acquisition algorithm with backlash inheritance, UI in LiveControlWidget /
LaserAutofocusSettingWidget / acquisition widgets, sample-dependent reference
handling, and test strategy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses critical and major findings from independent review:
- C1: piezo path now dispatched via _move_z_for_offset (stage vs piezo) with
  range clamping; matches move_z_for_stack's piezo branch.
- C2: z-level body wrapped in try/finally so _reset_channel_z_offset runs on
  abort/exception; handle_acquisition_abort also resets defensively.
- C3: explicit invariant that _current_z_offset_um == 0 at every point where
  acquire_pos / _last_time_point_z_pos / stack helpers run.
- M1: repository.py:761-782 create-from-general path will be fixed to copy
  z_offset_um (latent bug exposed by the feature).
- M2: backlash caveat for < 5 µm regime near soft limits documented.
- M3: test plan extended with piezo, time-lapse, abort, multi-region, and
  the create-from-general persistence path.
- M4: in-memory currentConfiguration.z_offset_um mutation pattern made explicit.
- Minor: MultiPointWithFluidicsWidget added to UI scope; live-view uses
  absolute positioning to be robust against manual z jogs; checkbox-read
  timing for MultiPointController clarified.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
17 tasks across 6 phases:
  1-2: Repository fix + ZOffset setting key (TDD)
  3-4: Model description, signal_reference_changed
  5: AcquisitionParameters plumbing
  6-9: Worker logic — helpers, loop integration, abort handling, logging
  10-12: Acquisition widget checkboxes (FlexibleMultiPoint, Wellplate, WithFluidics)
  13-14: LiveControlWidget UI (toggle + spinbox + capture + reset + apply-on-switch)
  15: NapariLiveWidget mirror
  16: LaserAutofocusSettingWidget Reset All
  17: End-to-end simulation smoke

Each task has TDD-style steps (failing test → implementation → verify → commit)
where unit-testable; UI tasks rely on the final simulation smoke.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…from general

When update_channel_setting creates an objective config from the general
config (because no objective-specific file exists yet), the AcquisitionChannel
constructor omitted z_offset_um, silently resetting it to 0.0. Added
z_offset_um=ch.z_offset_um to the comprehension so per-channel laser-AF
offsets survive the first objective-level write.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… and worker

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…andle_z_offset

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ultiPointWidget

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…WithFluidicsWidget

Mirrors the pattern from Tasks 10-11: declares checkbox_applyChannelOffset,
adds it inside the SUPPORT_LASER_AUTOFOCUS layout block, wires the enable-state
helper to checkbox_withReflectionAutofocus.toggled, syncs initial state, and
adds _update_apply_channel_offset_enable_state / _on_apply_channel_offset_changed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Widget

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…trolWidget

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The "wired to behavior in Task 14" comment was left over from development
notes and adds no information to a reader unfamiliar with the task plan.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion widgets

Extract the two duplicated laser-AF per-channel z-offset methods into a
_ApplyChannelOffsetMixin and have FlexibleMultiPointWidget,
WellplateMultiPointWidget, and MultiPointWithFluidicsWidget inherit it,
eliminating three copies of the same code.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e positioning

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ainst stage errors

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… handler

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…l offsets

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support for per-acquisition-channel Z offsets (µm) relative to the laser reflection autofocus (laser AF) reference plane, applying them during multi-point acquisition (including z-stacks) and optionally during live-view channel switching, with config persistence and tests.

Changes:

  • Adds delta-tracked per-channel Z-offset application/reset logic in MultiPointWorker, gated by laser AF + a new apply_channel_offset flag.
  • Adds UI controls to view/edit/capture/reset per-channel offsets and to apply offsets on live channel switch; adds a “Reset all channel offsets” workflow.
  • Extends config repository plumbing to persist z_offset_um via a new "ZOffset" setting key and fixes an existing objective-config creation path that dropped z_offset_um.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
software/control/core/multi_point_worker.py Adds delta-tracking helpers, integrates per-channel offsets into acquisition loop, abort cleanup, and “ignored offsets” logging.
software/control/core/multi_point_utils.py Adds apply_channel_offset to AcquisitionParameters (default True).
software/control/core/multi_point_controller.py Plumbs apply_channel_offset from controller state into worker parameters.
software/control/core/config/repository.py Adds "ZOffset" setting support and preserves z_offset_um when creating objective config from general config.
software/control/core/laser_auto_focus_controller.py Adds signal_reference_changed and emits it on reference set/clear (partial coverage).
software/control/widgets.py Adds live-view Z-offset controls, “apply on channel switch”, “reset all offsets”, and acquisition checkbox via mixin.
software/control/gui_hcs.py Wires “reset all offsets” signal to refresh the live Z-offset spinbox.
software/control/models/acquisition_config.py Updates z_offset_um field description/semantics.
software/tests/control/test_MultiPointWorker_offsets.py New unit tests for worker offset helpers (stage + piezo + abort + logging).
software/tests/control/test_LiveControlWidget_offset.py New unit tests for live absolute-positioning helper.
software/tests/control/test_MultiPointController.py Tests apply_channel_offset default and override behavior.
software/tests/control/core/config/test_repository.py Regression + persistence tests for "ZOffset" and create-from-general behavior.
software/docs/laser-af-channel-offset-design.md Design spec for the feature.
software/docs/laser-af-channel-offset-plan.md Implementation plan/checklist for the feature.
Comments suppressed due to low confidence (1)

software/control/core/laser_auto_focus_controller.py:32

  • signal_reference_changed is only emitted in initialize_auto() (False) and set_reference() (True). However on_settings_changed() reloads cached laser AF configs on profile/objective change and can flip laser_af_properties.has_reference without emitting the signal, so widgets can end up with stale enabled/disabled state. Suggest emitting signal_reference_changed.emit(self.laser_af_properties.has_reference) after load_cached_configuration() (or in initialize_manual() when applying a config) so listeners stay in sync.
class LaserAutofocusController(QObject):
    image_to_display = Signal(np.ndarray)
    signal_displacement_um = Signal(float)
    signal_cross_correlation = Signal(float)
    signal_piezo_position_update = Signal()  # Signal to emit piezo position updates
    signal_reference_changed = Signal(bool)  # emitted with new has_reference state

    def __init__(
        self,
        microcontroller: Microcontroller,
        camera: AbstractCamera,

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

Comment thread software/control/widgets.py Outdated
Comment on lines +4057 to +4063
# Wire 'Apply on channel switch' checkbox enable state to laser AF reference availability.
laser_af = getattr(self.liveController.microscope, "laser_autofocus_controller", None)
if laser_af is not None:
laser_af.signal_reference_changed.connect(self._on_laser_af_reference_changed)
initial_has_ref = bool(getattr(laser_af.laser_af_properties, "has_reference", False))
else:
initial_has_ref = False
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code] Fixed in e2d07bc. Confirmed Microscope had no public laser_autofocus_controller attribute — the widget's getattr fell back to None silently. Added a public property exposing _laser_af_controller, and pre-populated it in gui_hcs.py with the GUI's LaserAutofocusController so both code paths share the same instance.

Comment on lines +1225 to +1229
self._current_z_offset_um = 0.0
try:
self._move_z_for_offset(-saved)
except Exception:
self._log.exception(f"Failed to reset channel z-offset of {saved:+.2f} µm; stage may be at non-baseline z")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code] Fixed in 7c4e4eb (and tracked by an updated test in the same commit). _reset_channel_z_offset now zeros _current_z_offset_um only after _move_z_for_offset returns successfully; on failure the tracker is retained so a follow-on reset attempt knows the outstanding amount. The test test_reset_handles_stage_failure_without_stranding_state was updated to assert the retain-on-failure semantics plus a retry-succeeds case.

Comment on lines +1 to +10
# Per-Channel Z-Offset for Laser Autofocus — Implementation Plan

> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.

**Goal:** Apply a per-channel z-offset from the laser-AF reference plane during acquisition and (opt-in) when switching channels in live view, with capture / reset UI affordances and a robust delta-tracking algorithm.

**Architecture:** Reuse the existing `AcquisitionChannel.z_offset_um` field. In acquisition, the worker tracks a running offset and emits only the minimum stage/piezo moves needed (`_apply_channel_z_offset` / `_reset_channel_z_offset`). In live view, the widget computes the absolute target z each time it switches channels, robust against manual jogs. Behavior gated on laser AF being the active AF method AND a new acquisition-widget checkbox.

**Tech Stack:** Python 3, PyQt5 (qtpy compat), Pydantic v2, pytest. CI uses `black==25.12.0`; run `black --config software/pyproject.toml software/` before commit.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code] Acknowledged — the plan doc has tool-specific references and machine-local paths. Leaving cleanup of that doc for the PR author since it's documentation of how this work was done, not part of the shipping behavior.

Alpaca233 and others added 12 commits May 25, 2026 17:52
…lures

Five correctness fixes to _apply_channel_z_offset / _reset_channel_z_offset /
_move_z_for_offset, all surfaced by the PR #551 review:

* Piezo state mutation order: _move_z_for_offset now commands piezo.move_to
  before updating self.z_piezo_um. Previously the cache was updated first,
  so any exception in move_to left the software cache pointing at a position
  the hardware never reached, biasing every subsequent z_stack step.

* Reset tracker order: _reset_channel_z_offset zeros _current_z_offset_um
  only after the reset move succeeds. On failure the tracker is retained so
  a follow-on reset attempt knows the outstanding amount and follow-on
  applies compute deltas from the right baseline.

* Float-equality guard: 'if delta_um == 0' was missing near-zero deltas
  generated by accumulated FP error across repeated channel switches,
  causing spurious sub-nm moves and stabilization sleeps. Replaced with an
  epsilon (_Z_OFFSET_EPS_UM = 1e-4 µm) tolerance, well below stage/piezo
  step resolution.

* NaN guard: a non-finite z_offset_um in the channel config (e.g. NaN
  persisted by an older buggy capture path) was forwarded to the stage as
  a NaN move. Now treated as 0 with a warning.

* AF-success gate: per-channel offsets were applied even when reflection
  AF failed for the FOV, shifting the FOV relative to an absent reference.
  perform_autofocus now records _last_af_succeeded; _apply_channel_z_offset
  skips with a warning when the most recent reflection-AF attempt failed.

Test stub gains _last_af_succeeded and _Z_OFFSET_EPS_UM. Existing test
test_reset_handles_stage_failure_without_stranding_state updated to match
the new retain-on-failure semantics (with a retry assertion). Four new
regression tests cover the AF-success gate, NaN handling, near-zero delta,
and piezo move failure leaving cache untouched.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four fixes to LiveControlWidget surfaced by the PR #551 review:

* capture_current_z_offset: laser_af.measure_displacement() returns
  float('nan') on a soft failure (laser-on timeout, no spot, invalid
  centroid) rather than raising, so the existing try/except did not catch
  it. Capture would persist NaN to the channel YAML, breaking every
  subsequent acquisition. Now validates math.isfinite and warns the user.
  Also rejects values outside the spinbox range (the spinbox would
  silently clamp setValue, leaving model and UI in disagreement).

* _maybe_apply_live_channel_offset: same NaN path could feed
  stage.move_z_to(NaN). Added isfinite checks on both the AF reading and
  the stored channel offset. Also added a magnitude bound
  (_LIVE_OFFSET_MAX_JUMP_UM = 500 µm): a wildly wrong AF reading (drift,
  secondary peak, stale reference) could otherwise drive an unbounded
  millimetre-scale absolute move on every channel switch.

* update_ui_for_mode / refresh_z_offset_from_config: the 'value or 0.0'
  idiom treats NaN as truthy and propagates it into setValue. Replaced
  with a _safe_z_offset_value helper so a pre-existing NaN in config
  doesn't poison the spinbox.

* _reset_all_channel_offsets: signal_channel_offsets_reset was emitted
  only when every channel update succeeded. On partial failure the
  channels that DID reset left LiveControlWidget's spinbox showing a
  stale non-zero offset, which a subsequent edit would re-persist. Now
  emitted whenever any channel reset succeeded; the warning dialog still
  fires for the failed ones.

Four new regression tests cover the NaN-from-AF, NaN-from-config,
magnitude-cap, and small-move-still-allowed cases.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses Copilot review feedback on PR #551:

* Microscope: add public `laser_autofocus_controller` property exposing the
  lazily-initialized `_laser_af_controller`. LiveControlWidget reads via
  `liveController.microscope.laser_autofocus_controller`; before this fix
  the attribute didn't exist, getattr returned None, and the new Capture /
  Apply-on-channel-switch / Reset-all controls silently no-op'd in the
  real GUI.

* gui_hcs: pre-populate Microscope's controller slot with the GUI's
  LaserAutofocusController instance so both call paths share the same
  object (lazy init in Microscope.perform_laser_af stays available for
  headless scripts).

* laser_auto_focus_controller.initialize_manual: emit
  signal_reference_changed at the end so widgets stay in sync when
  on_settings_changed / load_cached_configuration reload a config whose
  has_reference differs from the previous one. The previous emissions
  in initialize_auto / set_reference still fire; the new emit covers the
  reload path that Copilot's suppressed comment called out.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 'Capture current' label was vague — capture what's current? The button
reads the live laser-AF displacement and saves it as this channel's z-offset,
which is more naturally framed as 'use the current focus as the offset'.
Renames the button and the related dialog titles ('Capture failed' →
'Reading failed', 'Capture out of range' → 'Reading out of range') for
consistency.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… AF off

* LiveControlWidget per-channel Z-offset row:
  - 'Apply on channel switch' → 'Apply in Live' (clearer scope: applies
    while in live view, on channel switch).
* Acquisition-widget checkbox (Flexible/Wellplate/Fluidics):
  - 'Apply per-channel z-offset' → 'Per-channel Z-offset'.
  - Hidden + unchecked when reflection AF is off (previously just disabled);
    the feature is meaningless without an AF anchor, so hiding makes the
    dependency obvious and clears the controller flag so it can't be a
    silent opt-in once AF is later turned on.
* Z-offset string casing unified to 'Z-offset' (capital Z, hyphen, lowercase
  'o') across user-visible labels, buttons, tooltips, and dialogs:
  - Button 'Reset all channel offsets' → 'Reset all channel Z-offsets'.
  - Dialog title 'Reset channel offsets' → 'Reset channel Z-offsets'.
  - QLabel 'Z offset:' → 'Z-offset:'.
  - Spinbox/button tooltips and dialog bodies use 'Z-offset'.
  Log messages, internal comments, and Python identifiers keep their
  lowercase 'z-offset' / 'z_offset_um' forms.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
These docs are about the implementation process and live with other
in-progress AI work-product, not in the Squid source tree. Moved to
AI-docs/Squid/in-progress/.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ctive

User-reported: after clicking 'Use Current' the captured offset never
applied at acquisition time. Root cause: update_channel_setting('ZOffset')
writes to the OBJECTIVE config (z_offset_um is per-objective), but
merge_channel_configs pulled the value from the GENERAL config, so every
acquisition read the stale general value and ignored the captured offset.

Other per-objective fields (exposure, gain, pixel_format) already merge
from the objective channel; this brings z_offset_um in line with them.

Tests in test_acquisition_config_models.py and test_utils.py were
asserting the bugged behavior (objective channel constructed without
z_offset_um, defaulting to 0.0, with the merge expected to fall back to
general). Updated to mirror the real auto-create-from-general flow that
copies z_offset_um into the objective channel, and added an explicit
regression test that asserts a captured objective offset wins over a
stale general one.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User-reported: clicking Use Current while live view was running returned
NaN; manually stopping live first made it work. Root cause: laser AF's
measure_displacement() issues turn_on_AF_laser via the microcontroller
and waits for the operation-completed ack, while live continuously queues
trigger commands on the same serial link. The two contend and the wait
times out, so measure_displacement returns float('nan').

capture_current_z_offset now records was_live, stops live for the
measurement window, and restarts it in a finally block so live resumes
even when measure_displacement raises or returns NaN. The live restart
fires before any subsequent warning dialog so the user doesn't stare at
a frozen frame while dismissing the message.

Not applied to _maybe_apply_live_channel_offset: that path is the
'Apply in Live' feature itself, and stopping live on every channel
switch would defeat its purpose. If users hit NaN there too we can
revisit, but the existing NaN guard prevents bad stage moves regardless.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…olWidget

Removes the 'Reset all channel Z-offsets' button from the laser AF settings
panel and adds a 'Reset All' button next to the existing 'Reset' button in
LiveControlWidget's Z-offset row. Co-locating the bulk-reset action with
the per-channel reset matches how users mentally group these operations and
removes the cross-widget signal indirection.

* New layout: Z-offset: [spinbox] [Use Current] [Reset] [Reset All] [Apply in Live]
* _reset_all_channel_z_offsets() moved into LiveControlWidget and refreshes
  the spinbox directly instead of going through signal_channel_offsets_reset.
* signal_channel_offsets_reset removed (no remaining consumers) and the
  gui_hcs.py wiring that connected it dropped.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User report: 'Apply in Live' is clipped at the right edge of the Z-offset
row because the default QPushButton sizing for 'Reset' (~70px) plus
'Reset All' (~85px) leaves no room. Fixed widths of 55px / 75px give
just enough padding around the text and free ~25px for the checkbox.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ive-restart

Two code-review follow-ups from PR #551 that survived the dialog Z-offset
column revert:

* Replace setFixedWidth(55)/(75) on the Reset and Reset All buttons with a
  setMaximumWidth derived from fontMetrics().horizontalAdvance(text) + 16
  px padding. The fixed pixel values would clip 'Reset All' on HiDPI
  displays and accessibility-large-text setups.

* capture_current_z_offset:
  - Wrap stop_live() in try/except so a failure shutting down live doesn't
    propagate before the finally block enters; the finally still attempts
    start_live() so we don't leave live in a stuck-stopped state.
  - Emit signal_start_live after the programmatic start_live() so
    subscribers (tab switch / alignment widget enable) re-fire — matches
    what toggle_live() does when the user clicks the button manually.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…int for buttons

Two follow-ups to 42b8758 surfaced by code review:

* Remove signal_start_live.emit() from capture_current_z_offset's finally.
  Its subscriber onStartLive (gui_hcs.py) unconditionally calls
  imageDisplayTabs.setCurrentIndex(0), so a user clicking 'Use Current'
  from a non-Live tab (Multichannel Acquisition, Mosaic, NDViewer,
  Laser-Based Focus) was being yanked back to the Live View tab. Even
  worse, on the laser-focus tab the cascading onDisplayTabChanged(0)
  shuts down the focus-camera preview the user is monitoring. start_live()
  alone is enough to resume the camera stream; toggle_live()'s other side
  effects are user-press-driven and not appropriate here.

* Switch _shrink_btn_to_text from fontMetrics+16px-padding to
  setFixedWidth(btn.sizeHint().width()). sizeHint already includes the
  active QStyle's button margin, frame thickness, focus rect, font, and
  icon — so the label cannot clip on themes where the chrome exceeds the
  flat 16px. Matches the existing repo pattern at widgets.py:12969/12990
  and removes the setMaximumWidth-without-setMinimumWidth regression
  (layout pressure could otherwise compress the button below text width).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment on lines +1242 to +1246
self._log.warning(
f"Channel '{config.name}' has non-finite z_offset_um={raw_target!r}; "
f"treating as 0 and skipping the move"
)
target_um = 0.0
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code] Fixed in 7ec3d8b — kept the 'treating as 0' fall-through (it correctly resets the stage to baseline when a prior channel applied an offset), but reworded the log to 'treating as 0 (will reset to the un-offset baseline if a prior channel already applied an offset)' so the message matches the actual behavior.

Comment on lines +4024 to +4028
initial_has_ref = False

self.checkbox_applyOnChannelSwitch.setEnabled(initial_has_ref)
self.checkbox_applyOnChannelSwitch.setChecked(initial_has_ref)
self.btn_captureZOffset.setEnabled(initial_has_ref)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code] Fixed in 7ec3d8b — removed setChecked(initial_has_ref) so 'Apply in Live' is now opt-in regardless of laser AF reference state. setEnabled is still gated on has_reference, so the control is visible/clickable only when a reference exists, but never auto-fires a stage move on the next channel switch.

Alpaca233 and others added 3 commits May 26, 2026 20:40
…t + NaN log

* widgets.py: stop auto-checking checkbox_applyOnChannelSwitch when laser
  AF reports an existing reference. Auto-check made 'Apply in Live'
  effectively opt-out — a user with a previously-set reference would get
  an absolute Z move on the next channel switch with no prior explicit
  consent. Keep setEnabled() gated on has_reference; leave checked state
  to the user.

* multi_point_worker.py: fix the misleading warning for a non-finite
  channel z_offset_um. The previous text said 'treating as 0 and skipping
  the move', but the code still issues a move when _current_z_offset_um
  is non-zero (delta = 0 - prior_offset). Reworded to say it resets to
  the un-offset baseline.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…an at start

User report: 'the width of Reset button did not change and offsets are still
not being applied'.

Width: my prior 'use sizeHint()' change set the button to its natural
Qt-layout width, which is *wider* than the previous setFixedWidth(55) — so
the user saw no shrinkage. The whole point of the constraint was to push
the buttons *below* sizeHint so 'Apply in Live' fits in the row. Reverting
to the empirically-verified 55/75 inline; dropping the unused
_shrink_btn_to_text helper that no longer made sense.

Diagnostics: extend _log_ignored_offsets to also log when the gate is OPEN
and non-zero offsets exist ('Per-channel z-offsets will be applied: [...]').
The previous version logged only when offsets were ignored. The new positive
log lets users confirm at acquisition start that the worker saw the captured
offsets — useful when the symptom is 'offsets not applied' but the cause is
elsewhere (laser AF reference not set, sim AF failing, etc).

Updated test_log_ignored_offsets_silent_on_happy_path → now
test_log_ignored_offsets_logs_will_apply_on_happy_path to match the new
behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User feedback: 'af should not work on channel switch. only the offset need
to be applied based on the current position of offset = 0.'

Rewrites _maybe_apply_live_channel_offset to drop the laser AF call and
apply the stored per-channel offset as a RELATIVE delta against a tracker
of the currently-applied offset:

* Tracker (_live_current_z_offset_um) starts at 0 and is reset to 0 each
  time the user enables 'Apply in Live' — treating the current stage z
  as the offset=0 baseline.
* On each subsequent channel switch with the box checked, the helper
  reads new_config.z_offset_um, computes delta = target - tracker, and
  issues stage.move_z(delta/1000) (relative). On success the tracker
  is updated to the new target so the next switch only moves by the
  difference between consecutive channels' offsets.
* Gates retained: 'Apply in Live' checkbox checked; laser AF has a
  reference (offsets come from AF capture); stored offset is finite;
  |delta| <= safety cap (500 µm).
* No measure_displacement() call, no absolute move_z_to — the switch is
  pure delta-tracking and doesn't disturb the user's chosen focal plane
  beyond the relative offset between channels.

This matches the worker's delta-tracking semantics inside an FOV (sans the
per-FOV AF anchor) and avoids the failure mode where measure_displacement
returning NaN (or AF taking seconds) blocked switches in live view.

Tests rewritten to cover delta tracking, sequence accumulation, tracker
reset on enable, safety cap on delta, and that measure_displacement is
NOT called on switch.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants