feat(record-zstack): Hamamatsu fps not settable — report achievable rate, read-only UI - #587
Open
hongquanli wants to merge 64 commits into
Open
feat(record-zstack): Hamamatsu fps not settable — report achievable rate, read-only UI#587hongquanli wants to merge 64 commits into
hongquanli wants to merge 64 commits into
Conversation
Implements set_frame_rate override on SimulatedCamera to cap the frame rate in continuous acquisition mode. When set_frame_rate is called, the streaming thread's frame cadence gate now honors both exposure time and the target frame period, using whichever is larger. The method returns the effective achievable FPS clamped by the total frame time (exposure + strobe). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Lift shared acquisition mechanics out of MultiPointWorker into a new MultiPointWorkerBase so a future RecordZStackWorker sibling can reuse them: camera/stage/channel-apply, single-frame capture (acquire_camera_image), the camera frame callback (_image_callback) with CaptureInfo handoff + job creation/dispatch + backpressure gating + ready-for-next-trigger event, job-result summarize/finish, move_to_z_level, _select_config, _sleep, _frame_wait_timeout_s, wait_till_operation_is_completed, update_use_piezo. MultiPointWorker now subclasses MultiPointWorkerBase, calls super().__init__() for the shared handles/state, then builds the real job runners / backpressure controller and sets all MultiPoint-specific state (NZ/deltaZ/z_range/ z_stacking_config/use_piezo/selected_configurations/scan coords/time-point/ AF + per-channel-offset). Orchestration (run, run_single_time_point, run_coordinate_acquisition, acquire_at_position) and the z-stack/offset/AF helpers stay in MultiPointWorker unchanged. _emit_plate_layout gets a base no-op stub overridden by MultiPointWorker's real implementation so the lifted frame callback stays self-contained. Method bodies moved verbatim; net MultiPointWorker state is identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
….py (no behavior change) Move the experiment-ID timestamping + directory creation logic from MultiPointController.start_new_experiment into a free function create_experiment_dir(base_path, experiment_id) -> (resolved_id, dir_path) in control/core/acquisition_setup.py so a future RecordZStackController can reuse it without duplicating code. Pre-warmed JobRunner methods left in MultiPointController — they mutate instance state and do not factor out cleanly as free functions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rt/write race) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Appends ContinuousFrameSource (wraps AbstractCamera into start/stop source) and StreamingCapture (orchestrates source, router, stop-condition, writer) to streaming_capture.py. run() accepts an optional timeout parameter so Task D can bound wall-clock wait without hanging on a stalled camera. Adds fake-source test confirming 5-frame emit + downsampling + finalize. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…le); document stop/finalize assumption Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add RecordZStackAcquisitionParameters dataclass and pure helper functions frame_count, zstack_plane_count, and zstack_offsets_um for z-stack and recording acquisition planning. These helpers handle frame counting, z-plane calculation with epsilon tolerance, and offset list generation. Includes full test coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address code review: add ValueError tests for invalid z-stack inputs (z_max<z_min, step<=0) and add a comment explaining the floor epsilon. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add RecordZStackWorker(MultiPointWorkerBase) orchestrating per-FOV record + z-stack acquisition: - record() streams a continuous high-fps capture to a per-FOV recording .ome.zarr via the C3 StreamingCapture primitive - zstack() reuses the inherited triggered-capture + SaveZarrJob dispatch path (builds its own JobRunner + BackpressureController like MultiPointWorker), managing camera streaming/callback lifecycle locally - establish_reference() uses laser AF when available, falls back to current stage Z - run() loops time points -> regions -> FOVs, abort- and dt-aware Smoke test (simulated microscope, 2 wells x 2 FOV x 2 t, both phases) verifies recording dataset count/shape and z-stack per-FOV zarr shape. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d fix - Add RecordZStackController to record_zstack_controller.py: builds params via setters, pre-warms a JobRunner subprocess (mirrors MultiPointController), resolves a timestamped experiment dir via create_experiment_dir, constructs RecordZStackWorker, and spawns it on a daemon thread. Exposes request_abort(), join(), close(), and per-param setters for the widget. - Fix ZarrWriter._get_loop(): after calling get_event_loop() check that the returned loop is not already closed; if it is, fall through to new_event_loop() so recordings on daemon threads across multiple time-points don't raise 'Event loop is closed'. - Add test_record_zstack_controller_smoke: controller-driven test covering Nt=2 with dt_s=0.1, both recording and z-stack phases, shape assertions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements Task E1: widget skeleton (Option-A single-column layout with Output, Wells & FOV, Time-lapse/Focus, Recording phase, Z-Stack phase, Start groups), pure _validate_record_zstack_params() helper, validate(), build_parameters(), and _add_zstack_channel_row(). 18 tests pass (13 pure-helper, 5 widget via qtbot). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ow table Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… labels - Recording group: add exposure/gain/illumination spinboxes and a '⟳ Live' button that copies currentConfiguration into the recording channel dropdown + inline editors - Z-stack table expanded to 5 columns (channel, exposure, gain, illumination, actions); each row has per-channel spinboxes, a '⟳' copy-from-live button, and a '✕' remove button - '+ Add channel' combo+button below the z-stack table - Added _remove_zstack_channel_row(), _set_zstack_row_values(), _get_zstack_row_values(), _copy_recording_from_live(), _copy_zstack_row_from_live(), _on_zstack_add_channel_clicked() - build_parameters() now reads inline editor values for both the recording channel and each z-stack row; uses model_copy(deep=True) to avoid mutating source channel objects - 7 new qtbot tests added (26 total, all passing) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cstring Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…RDING) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Completes E4: gui_hcs.py:1148 connects acquisition_finished -> recordZStackWidget.acquisition_is_finished; these slots were authored with E4 but omitted from its commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirrors the recordZStackController sentinel; avoids a latent AttributeError if recordZStackWidget is referenced when ENABLE_RECORDING is False. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… lifecycle CRITICAL 1: recording captured dark frames — turn illumination on around cap.run() (off in finally), since set_microscope_mode only energizes illumination when live and the CONTINUOUS stream does not gate it. CRITICAL 2: z-stack used the wrong acquire_camera_image branch — set liveController.set_trigger_mode(SOFTWARE) (capturing/restoring the previous mode) so camera mode and liveController.trigger_mode agree. IMPORTANT 6: zstack() now calls camera.stop_streaming() in the finally, mirroring ContinuousFrameSource.stop(), so the next FOV's recording starts on a stopped camera. IMPORTANT 9a: hoist stop-live to run() start (once, capturing was_live) and restart live once in run()'s finally; removed per-FOV stop-live from record(). MEDIUM: precompute zstack offsets and frame shape once in __init__. SIMPLIFICATION: resolve recording_channel to a local in record(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…g, non-blocking enqueue, abort sealing, partial warning) - ContinuousFrameSource.start: set CONTINUOUS mode before set_frame_rate so the toupcam PRECISE_FRAMERATE hint survives the mode switch (IMPORTANT 5). - RecordingWriter: track _started, set only after thread.start(); finalize/abort skip the join when not started so a failing initialize() propagates its real error instead of a 'cannot join thread' crash (IMPORTANT 7). - StreamingCapture._on_frame: re-check stop condition before routing so no frame is enqueued at/past t-index T into a (T,...)-shaped dataset (IMPORTANT 8). - RecordingWriter.enqueue: put_nowait + drop-on-full (no 0.5s block on the camera hot thread); default queue maxsize 64 -> 256 (MEDIUM). - StreamingCapture: track _aborted; run() finally calls writer.abort() on abort (seals zarr incomplete) else finalize() (MEDIUM). - StreamingCapture.run: WARNING when emitted < expected (CountStop.expected()); trailing zarr planes are blank fill (MEDIUM). - Document that the post-stop _emitted read is safe (MINOR). - Tests: start-error path, OOB gating, abort->writer.abort, complete->finalize, partial warning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n fixes - IMPORTANT 3+4: fix _get_selected_well_count to resolve lazily via scanCoordinates.get_selected_wells() instead of a stale cached well_selection_widget; handles None (glass-slide → count=1) and empty dict (no wells selected → count=0 → validation rejects) - IMPORTANT 9b: reject recording configs where frame_count(fps, duration_s) < 1 in _validate_record_zstack_params with clear message - MEDIUM: replace _abort_requested bool with threading.Event (_abort_event); set/clear/is_set are thread-safe; abort_requested_fn passed to worker uses lambda: self._abort_event.is_set() - REUSE: add run_acquisition(params) direct-params path; toggle_acquisition now calls run_acquisition(self.build_parameters()) — no 15-setter fan-out; setters kept as shims for test_record_zstack_worker.py compatibility - SIMPLIFICATION: remove unreachable None guard in _on_zstack_add_channel_clicked; log warnings in _get_zstack_row_values instead of silently substituting defaults - Tests: 39 pass (8 new); cover frame_count=0, glass-slide/None well count, lazy well-selector resolution, and abort Event lifecycle Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… base, extract compute_pixel_size_um, drop redundant _job_runners re-assignments, fix misleading __class__ comment - REUSE#1: move _wait_for_outstanding_callback_images verbatim to MultiPointWorkerBase; remove subclass overrides from MultiPointWorker and RecordZStackWorker (both inherit the shared implementation) - REUSE#2: add compute_pixel_size_um(objective_store, camera) to acquisition_setup.py; call from both worker __init__s - REUSE#5: deferred — ZarrWriterInfo constructions differ materially (is_hcs, use_6d_fov) between MultiPointWorker and RecordZStackWorker - SIMP#1: drop redundant self._job_runners = [] in MultiPointWorker and self._job_runners/_backpressure/_abort_on_failed_job/ _first_job_dispatched in RecordZStackWorker (base __init__ sets them) - B1: correct misleading comment — __class__ in a method body always refers to the defining class, not the runtime type; type(self) is what produces per-subclass logger names Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ix-batch5) - Add signal_acquisition_started(bool) to RecordZStackMultiPointWidget, emitting True on valid start and False in acquisition_is_finished(), mirroring the pattern used by WellplateMultiPointWidget and FlexibleMultiPointWidget. - Connect the signal to gui_hcs.toggleAcquisitionStart inside the ENABLE_RECORDING guard so other tabs, click-to-move, and live-scan-grid are locked during acquisition. - Update toggle_acquisition docstring to reflect build_parameters() call path. - Add comment in record_zstack_controller explaining the abort_event clear→start window. - Add dropped_count property to RecordingWriter and a summary WARNING log in StreamingCapture.run so slow-disk dropped frames are diagnosable without grepping. - Add 5 new tests covering the signal and dropped-count behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code review found a race: toggle_acquisition emitted True AFTER run_acquisition() returned, but run_acquisition spawns a worker thread that on a fast/one-frame acquisition could finish (acquisition_is_finished -> emit False) before the GUI thread reached the emit(True) line. The GUI would then process False then True, permanently locking all tabs. Fix mirrors WellplateMultiPointWidget (which emits True via _set_ui_acquisition_running before run_acquisition): - Emit True before calling run_acquisition. - Wrap run_acquisition in try/except; on failure un-check the button and emit False to unlock the UI (the worker never started, so acquisition_finished will not fire). - Document the emit(False) paths in acquisition_is_finished docstring. - Add 2 tests: emit-ordering (True before run) and run-raises (True then False). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ntroller The widget already uses the clean run_acquisition(params) path, so the 15 one-line setter methods, the corresponding duplicate per-field instance attributes, and the legacy params=None fallback are all dead code. Remove them and update the controller test (test_record_zstack_controller_smoke) to build RecordZStackAcquisitionParameters directly and pass it to run_acquisition(params), matching how the widget works. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t, state restore, fps Fixes the critical/high findings from the streaming/channel-control code review (worktrees/docs/code-review-record-zstack-streaming-2026-07-03.md): - Size the recording dataset from one real processed frame instead of camera.get_resolution(): real cameras crop/rotate frames, so every write failed shape-check and the recording was silently blank. Probe runs in run() after live view stops. - Drain thread counts write_frame failures and seals the store as incomplete (abort) instead of stamping acquisition_complete=True over blank fill; the worker fails fast (aborts the acquisition) on write errors instead of recording blank data at every remaining FOV. - finalize() no longer deadlocks pushing the sentinel into a full queue while the drain thread is wedged in a stalled write (bounded put with fallback to the abort path). - StreamingCapture.run() polls abort_fn while waiting, so Stop works when the camera delivers no frames, and the aborted capture is sealed aborted instead of complete. - run() captures and restores the pre-acquisition trigger mode and channel configuration (record-only runs left the camera stuck in SOFTWARE_TRIGGER; z-stack runs left the last z-stack channel applied with the GUI showing the old one). - zstack()'s trigger-mode fallback keeps liveController.trigger_mode in sync with the camera so acquire_camera_image still gates illumination (a stale mode captured the whole stack dark). - RecordingRouter anchors emission slots to the first frame with a quarter-period tolerance: at-rate delivery jitter no longer rejects alternate frames (halving the capture rate); faster-than-target cameras still downsample correctly. - record() sizes T, pacing, and time_increment_s from the achievable fps returned by set_frame_rate() instead of stalling to timeout and leaving blank trailing planes when the camera clamps the rate. TDD: each fix has a test that failed before it (10 new tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ing, refresh, cleanup, bookkeeping Remaining findings from the streaming/channel-control review: - RecordingWriter queue is bounded by bytes (default 2 GiB) as well as count: 256 full-resolution 16-bit frames is ~13 GB, so a count-only bound could OOM the process on a slow disk. - _move_xy honors the optional z of (x, y, z) scan coordinates like MultiPointWorker.move_to_coordinate — stored per-FOV focus planes were silently ignored (out of focus on tilted samples without laser AF). - _wait_for_dt paces timepoint STARTS at acquisition_start + k*dt (matching multipoint and the recorded time_increment_s), instead of appending dt after each timepoint's work; logs when work overruns dt. - RecordZStackMultiPointWidget.refresh_channel_list() repopulates the channel combos on objective/profile/config changes (wired in gui_hcs alongside the other multipoint widgets), and the _make_channel_base fallback warns loudly instead of silently producing a channel with no illumination mapping (dark images). - gui_hcs._cleanup_common closes recordZStackController on exit so the JobRunner subprocess finalizes Zarr writers instead of being killed mid-write. - Each experiment directory now gets the acquisition_channels.yaml settings snapshot (controller) and the .done completion marker (worker), matching every multipoint acquisition. TDD: 8 new tests, each watched failing first (except the two gui_hcs wiring changes, covered by the CI gui test file and runtime driver). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Inline-verified findings from the second review pass (the workflow's verify phase hit API limits; candidates were verified by hand): - Dropped frames (byte cap / queue full) no longer seal the store acquisition_complete=True: StreamingCapture counts routed frames, so the stop condition was satisfied even when enqueue dropped them and the completeness attribute lied over fill-value planes. Any drop or write error now seals incomplete. - finalize() exposes finalize_wedged; the worker's fail-fast treats a wedged drain as failure (write_error_count reads 0 when finalize returns before the errors happen, so the check alone was bypassable — a stalled disk would have burned the timeout at all remaining FOVs). - finalize()'s sentinel put and join share one timeout budget (a late sentinel followed by a full-length join blocked callers ~2x). - Timepoint pacing uses time.monotonic() (NTP steps skewed wall-clock pacing) and skips slots that already passed, mirroring MultiPointWorker's grid-preserving skip; dt=0 remains continuous. - refresh_channel_list fetches before clearing: a transient get_channels failure (or empty set) no longer wipes the user's configured z-stack rows and combos. - Profile changes now also refresh the tab's channel combos (was objective-only; profile switches replace the channel set too). - _probe_frame_shape rejects color (Y,X,3) frames with a clear error instead of building a dataset every write fails against. - zstack() no longer restores the trigger mode per FOV — run() restores once at the end, removing 2 camera mode switches + 2 MCU commands per FOV. - Hot-path drop warnings are rate-limited (first + every 100th) — per-frame logging at fps rate on the camera delivery thread. Deferred (documented in the review doc): NTP-step handling in RecordingRouter's absolute-slot anchor, a GUI-visible error dialog for fail-fast aborts, folding the achievable-fps probe into ContinuousFrameSource, and app-exit-vs-running-worker hardware races. TDD: 6 new tests watched failing first; controller smoke updated to dt=0 (grid-skip semantics make dt<work legitimately skip timepoints). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ification R1-R5 + R9 from the full adversarial verification pass (see worktrees/docs/code-review-record-zstack-streaming-2026-07-03.md): - Under-delivered captures (camera stall, no user abort) no longer seal acquisition_complete=True: StreamingCapture calls mark_incomplete() and the drain seals with captured/expected counts. - record() fails fast on dropped frames too — sustained backpressure drops are the systematic slow-disk case the guard exists for. - RecordingRouter maps frames to the slot nearest their arrival time: a burst after a delivery stall leaves the stall as fill holes instead of back-filling consecutive slots and compressing the time axis; slots past T stop the capture instead of writing out of bounds. - A wedged finalize() now sets flush-and-exit instead of abort: the captured backlog is written whenever the stall clears rather than discarded; join-timeout on a slow-but-progressing drain no longer sets finalize_wedged (false-positive fail-fast aborted whole runs over stores that finished fine). - Non-abort incomplete seals no longer stamp aborted=true: ZarrWriter.abort(mark_aborted=False, extra_attrs=...) writes incomplete=True with error/drop/captured counts so tooling can tell "user pressed Stop" from "finished with missing planes". Wedge-style tests join their drain threads before returning (the class-level ZarrWriter monkeypatches made leftover daemon drains call the next test's stubs — order-dependent flakes). TDD: 6 new tests watched failing first; router/index test updated to the honest-timeline semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ification R6-R8, R10-R13 (R1-R5+R9 landed in the previous commit): - toggleAcquisitionStart shares the tab-needs-well-selector decision with onTabChanged (_tab_uses_well_selector): finishing a Record + Z-Stack acquisition no longer hides the well selector permanently, and transient acquisition hides no longer overwrite the user's remembered visibility preference. - Entering the Record + Z-Stack tab rebuilds the FOV grid (regions were cleared but never rebuilt), and well clicks on this tab rebuild it too (wellplate's handler early-returns for other tabs), so the navigation viewer shows scan coverage before Start. - refresh_channel_list warns loudly when the previously selected recording channel vanishes after an objective/profile change (the combo silently swapped to the first channel while the exposure/gain/ illumination spins kept the old channel's values). - acquisition_channels.yaml dedupes channels by name (recording entry wins) — a channel used by both phases appeared twice. - ContinuousFrameSource(already_configured=True): record() already probes CONTINUOUS mode + frame rate for the achievable fps, so the source no longer repeats both per FOV (mode switch + strobe/exposure re-send on toupcam); the clamped rate is re-applied when it differs. - _populate_channel_combo(names=...) shared by construction and refresh (two divergent population paths). - _cleanup_common closes recordZStackController with a 30s budget — 5s let app teardown race a worker still unwinding on a slow disk (partial fix; closeEvent acquisition guard remains a follow-up). TDD: 4 new tests watched failing first (well-selector restore test rides in the CI-only gui test file). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Conflict resolution notes (software/control/core/multi_point_worker.py): this branch MOVED the shared capture mechanics (_image_callback, acquire_camera_image, _wait_for_outstanding_callback_images, _sleep) into MultiPointWorkerBase, while master MODIFIED them in place (acquisition watchdog #565, per-channel z-offset logging #573, error-vs-user-abort classification). Resolution keeps the refactored layout and ports master's edits into the base-class copies: - _abort_due_to_error and a no-op _run_state_beat live on the base (MultiPointWorker overrides the beat with the real watchdog one), so every request_abort_fn() error site in the moved methods now tags _abort_cause = "error" exactly as on master. - _abort_cause initialized in the base __init__; MultiPointWorker keeps run_state_writer/_run_state and _compute_end_reason. - Per-image _run_state_beat() call added in the base _image_callback, matching master's placement after the image counters. - Master's z-offset move logging auto-merged (methods not moved). Verified: record-zstack suites + master's watchdog/worker-reason tests (127 passed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eeding, narrower layout Mirrors WellplateMultiPointWidget's XY/Time tab row (checkbox + combo in a colored frame that shows/hides its controls), skipping Z since this widget's own Z-Stack phase group already covers z-stacking: - checkbox_xy + combobox_xy_mode now offer two real modes: "Select Wells" (today's tile-over-selected-wells behavior, kept as default) and "Current Position" (new: single FOV at the live stage position, bypassing well selection — validate() no longer requires wells in this mode). Unchecking XY forces Current Position and disables the combo; re-checking restores the previous mode, mirroring WellplateMultiPointWidget exactly. - checkbox_time wraps Nt/dt: unchecked forces a single timepoint and hides the controls, restoring the previous Nt/dt on re-check. - checkbox_laser_af relocated into this row as a plain checkbox, matching how WellplateMultiPointWidget places its own Laser AF toggle. Channel-add now seeds from the channel's own configured settings (via a new _channel_settings/_find_channel lookup) instead of hardcoded 50ms/0 gain/50%, for both the z-stack "+ Add" button and the recording-channel combo (including its initial row and on selection change). Layout: removed the channel tables' fixed 530px width (fighting against whatever width the main window happens to lock in for this tab) in favor of a responsive fill plus tightened field widths throughout, so the panel no longer needs a horizontal scrollbar. Z-Stack channel table now spans the group's full width to match the Recording table. Z-Stack channel names get a tooltip showing the full name since the Channel column truncates. ~20 new tests covering the XY/Time toggling, mode switching, channel seeding, and table behavior.
…isition params Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…cquisition_channels.yaml Adds _save_record_zstack_yaml(), wired into run_acquisition() right after the existing acquisition_channels.yaml snapshot call. Writes a superset settings snapshot (recording phase, z-stack phase, wellplate scan region list when xy_mode == "Select Wells") to acquisition.yaml in the same experiment directory, without touching the existing snapshot call.
…et_type Replace the hardcoded multipointController.camera lookup with an overridable _get_camera_for_binning_check() hook, and change _get_other_widget_name() to map an actual widget_type string to a display name via a lookup table instead of guessing a binary wellplate/flexible opposite. Behavior-preserving for WellplateMultiPointWidget and FlexibleMultiPointWidget; needed so RecordZStackMultiPointWidget can reuse the mixin. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…label Task 5's review caught that "Record/Z-Stack" doesn't match the real tab label "Record + Z-Stack" (gui_hcs.py) — would confuse users once Task 7 wires the widget into the dialog.
Task review findings (PR #564): - Recording channel exp/gain/illum spinboxes were set unconditionally even when the YAML channel name wasn't found in the current combo, silently pairing a stale display name with mismatched numeric settings. Now only applied when found; a warning is logged otherwise and the row is left untouched. - checkbox_time was never synced from yaml_data.nt, leaving the Time checkbox unchecked (and time_controls_frame hidden) after loading a multi-timepoint YAML even though entry_Nt/entry_dt were updated. Now checkbox_time is blocked/set like the sibling WellplateMultiPointWidget, and the frame's visibility is refreshed directly in the finally block (not via _on_time_toggled, which would clobber the just-loaded Nt/dt via its stored/restore side effect). Adds two regression tests in tests/test_record_zstack_widget.py. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_apply_yaml_settings blocks checkbox_time's toggled signal while loading, so the normal _on_time_toggled -> _update_tab_styles path never fires. Fix Round 1 already restored checkbox state and frame visibility but left the frame's border/background stylesheet stale. _update_tab_styles() has no interaction with Nt/dt or _stored_time_params, so it's safe to call directly in the finally block.
Implement Task 8 of the Record/Z-Stack feature. Add two buttons to the output group that allow users to save and load full acquisition settings via file dialogs: - "Save Settings…" button calls _on_save_settings_clicked to save current widget state (calls _update_scan_regions, builds parameters, and calls _save_record_zstack_yaml) - "Load Settings…" button calls _on_load_settings_clicked to load from a file (calls the mixin's _load_acquisition_yaml) Both buttons use QFileDialog for file selection. Error handling shows warning dialog on save failure. Tests verify button clicks are wired correctly and call underlying save/load machinery. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tests Save test now hardens its own objectiveStore/camera stubs so the real _build_objective_info() -> _save_record_zstack_yaml() runs unmocked and writes a real, parseable YAML file, instead of mocking the save function itself (a workaround for the shared fixture's un-serializable MagicMocks). Load test now writes a real, valid record_zstack YAML file and only mocks QFileDialog.getOpenFileName, so the full parse_acquisition_yaml -> validate_hardware -> _apply_yaml_settings chain is actually exercised instead of a hand-rolled substitute. Also drops a redundant local `from unittest.mock import patch` now that the load test no longer needs to patch the instance method. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…c fields The test_full_save_load_round_trip_preserves_settings test was only checking channel names and basic parameters, missing critical assertions on: - recording_channel object equality (the actual channel object round-trips) - z-stack channels' numeric fields (exposure_time, analog_gain, illumination_intensity) Added comprehensive assertions to verify that these values survive YAML serialization/deserialization round-trips. Updated docstring to accurately describe what's now tested. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_apply_yaml_settings()'s finally block resynced time_controls_frame after signal-blocked mutation (checkbox_time.toggled was blocked during load), but missed the equivalent for XY: checkbox_xy.toggled and combobox_xy_mode.currentTextChanged were also blocked, so _on_xy_mode_changed's visibility refresh never fired. A loaded YAML with xy_mode="Current Position" left the FOV overlap/shape/size controls visible even though they don't apply in that mode. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… drop mixin _load_acquisition_yaml() (AcquisitionYAMLDropMixin, shared by wellplate/ flexible/record_zstack widgets) called self._apply_yaml_settings(yaml_data) with no guard. RecordZStackMultiPointWidget is the first (and only) consumer whose _apply_yaml_settings() calls AcquisitionChannel.model_validate() on embedded channel dicts, so a syntactically-valid YAML with a malformed channel (e.g. missing a required field) raised a pydantic ValidationError straight through the drop/button Qt slot instead of showing a warning. Wrap the call the same way the existing parse-error handling above it does: log, show a "Load Error" QMessageBox, and return False. Purely additive for the success path of all three widgets. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_save_record_zstack_yaml() wrapped its open()/yaml.dump() call in a try/except (OSError, yaml.YAMLError): log.error(...) that swallowed the error and returned normally. That's correct for nothing on its own -- it made the interactive "Save Settings..." button handler (_on_save_settings_clicked) log "Settings saved" and show no warning even when the file was never written, because its own try/except QMessageBox.warning guard never got a chance to fire. Remove the inner swallow and let OSError/yaml.YAMLError propagate. Both real call sites already handle this correctly one level up: - run_acquisition()'s snapshot call site already wraps this in try/except Exception: log.exception(...), so a failed settings snapshot still can't abort a real acquisition. - _on_save_settings_clicked() already wraps this in try/except Exception: QMessageBox.warning(...), which is now reachable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…acquisition # Conflicts: # software/control/gui_hcs.py
) master (PR #584) removed the duck-typed emit_selected_channels() / display_progress_bar() calls from gui_hcs.onTabChanged and toggleAcquisitionStart — channels are now emitted by each multipoint widget at acquisition start. RecordZStackMultiPointWidget's no-op stubs (added in 9b5be65 to satisfy the old contract) and their regression test are now unreachable dead code; remove them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ee-run max, not the triggered frame time The recording phase reads the camera's achievable rate via set_frame_rate(). On the ToupTek the PRECISE_FRAMERATE option can't be read (E_UNEXPECTED), so set_frame_rate() fell back to 1000/get_total_frame_time(). get_total_frame_time() is the *sequential* software/hardware-trigger period (readout + trigger_delay + exposure) — ~2x the true continuous free-run period — so recordings were capped at roughly half the camera's real rate: a 20 or 30 fps request clamped to ~14 fps (and dropping exposure didn't help, since trigger_delay grows to keep the total pinned). Add ToupcamCamera._continuous_max_framerate() = 1000/max(readout, exposure), where readout = strobe_time_us (the exposure-independent sensor readout period), and return it from set_frame_rate()'s fallbacks. record()'s existing "honor the requested fps unless the camera can't reach it" logic then works: requests up to the readout ceiling are honored; higher requests clamp to the true max. Triggered-mode timing is untouched — get_strobe_time / get_total_frame_time / _calculate_strobe_info are unchanged; only set_frame_rate (continuous-only) and the new helper change. Verified on a real ITR3CMOS26000KMA: 25 fps x 5 s -> 125 frames (honored); 30 fps x 4 s -> 112 frames at 28.04 fps (the true readout max), where the old code produced ~56 at the bogus 14 fps. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cleanup from the /simplify review (no behavior change): - Remove the unreachable `if frame_ms > 0` guard and the `1000/get_total_frame_time()` fallback. frame_ms is always positive (this model's sensor resolutions all map to a positive readout period), and that fallback returned the exact triggered-mode value the helper exists to reject — a misleading dead branch, not a real safety net. - De-duplicate the ~2x rationale: set_frame_rate's Returns now points to _continuous_max_framerate instead of repeating it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ate, don't try to set Hamamatsu ORCA can't command a frame rate in CONTINUOUS mode (INTERNAL_FRAMERATE and INTERNAL_FRAMEINTERVAL are read-only, bench-verified: DCAMERR.NOTWRITABLE). It free-runs at the exposure/readout-limited max, overlapping exposure with readout. Make the UI and worker match, generalized via a camera capability so ToupTek (which CAN set via PRECISE_FRAMERATE) is unaffected. - AbstractCamera.can_set_frame_rate() (default True) + estimate_frame_rate(exposure_ms) (default sequential 1000/(exposure+strobe)). - HamamatsuCamera: can_set_frame_rate() -> False. set_frame_rate() reports the camera's AUTHORITATIVE free-run rate by reading the read-only INTERNAL_FRAMERATE (which reflects the current readout mode fast/standard/ultraquiet, bit depth, ROI and exposure), falling back to the overlap estimate only if the read fails. estimate_frame_rate() = overlap 1000/max(readout, exposure) (readout = line_interval * rows) for the widget's preview at a hypothetical exposure. Neither path writes anything (removes the NOTWRITABLE ERROR + the READOUTSPEED side-effect of the earlier approach). - Record widget: when the camera can't set fps, the fps field is read-only and shows "~ X fps", recomputed from the (still-editable) recording exposure (incl. after a YAML load). - Worker: for not-settable cameras, size/pace the recording to the achievable max instead of honoring params.fps. Tests: base/sim capability + estimate; worker sizes to achievable; widget read-only vs editable. Hamamatsu-specific test (authoritative-read vs formula-fallback + overlap math) guarded on the DCAM lib (absent off-instrument); the overlap math is also covered by the ToupTek continuous-max tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Alpaca233
force-pushed
the
feat/record-zstack-acquisition
branch
from
July 27, 2026 21:30
0afae24 to
d07b875
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On the Hamamatsu ORCA the continuous-mode frame rate is not settable —
INTERNAL_FRAMERATEandINTERNAL_FRAMEINTERVALare read-only in every camera state (bench-verified:DCAMERR.NOTWRITABLE). It free-runs at the exposure/readout-limited max, overlapping exposure with readout. This makes the Record + Z-Stack UI and worker reflect that, generalized via a camera capability so it composes cleanly with the ToupTek_continuous_max_frameratework already on this branch (ToupTek can set the rate viaPRECISE_FRAMERATEand is untouched).Supersedes the earlier
INTERNAL_FRAMERATE-writing attempt (which produced aNOTWRITABLEERROR at the bench and silently forcedREADOUTSPEED=FASTEST).Changes
AbstractCamera:can_set_frame_rate()(defaultTrue) +estimate_frame_rate(exposure_ms)(default sequential1000/(exposure+strobe)).HamamatsuCamera:can_set_frame_rate() → False.set_frame_rate()reports the camera's authoritative free-run rate by reading the read-onlyINTERNAL_FRAMERATE— which reflects readout mode (fast/standard/ultraquiet), bit depth, ROI and exposure — falling back to the overlap estimate only if the read fails. No writes.estimate_frame_rate()= overlap1000/max(readout, exposure)(readout =INTERNAL_LINEINTERVAL × rows), for the widget's preview at a hypothetical exposure the camera isn't set to.params.fps.Testing
max()branches) vs editable for settable cameras.blackclean.Review
Independent review: no Critical, no Important; Minor items addressed (docstring accuracy on the capability default; refresh the read-only display after YAML load) or noted (display-only spinbox clamp at multi-second exposures). Reading
INTERNAL_FRAMERATEfor the actual rate resolves the reviewer's "ROI not reflected in the estimate" note for the worker path.Off-instrument constraint
HamamatsuCameracan't be imported/run without the DCAM library, so its methods arepy_compile+ static-review verified here and will first execute on the ORCA. Bench check: confirmset_frame_ratereturns a plausible full-frame value (~40–95 fps at short exposures) and the widget's "≈ X fps" tracks exposure.🤖 Generated with Claude Code