feat: Record + Z-Stack acquisition mode - #564
Conversation
Hardware test planSimulation testing (GUI driven end-to-end, Zarr outputs verified — happy path, validation, Copy-from-Live, mid-recording abort, Setup
Tests
Inspecting outputsPer-FOV shapes: recording import json, glob
for p in glob.glob("<experiment_dir>/**/zarr.json", recursive=True):
m = json.load(open(p))
if m.get("node_type") == "array":
print(p, m["shape"], json.load(open(p)).get("attributes", {}).get("_squid", {}))Log signals worth watching: Camera-only bench (no assembled scope)If you only have the ToupTek camera on a dev machine, Squid's per-component simulation covers most of the list above. In [SIMULATION]
simulate_camera = false ; real ToupTek camera
simulate_microcontroller = true ; simulated MCU, stage, illumination
simulate_spinning_disk = true
simulate_filter_wheel = true
simulate_objective_changer = true
simulate_laser_af_camera = trueThen launch without What this bench covers, in value order:
Still instrument-only after this bench: illumination energize (dark-frame check), hardware-trigger interplay, laser AF, real stage-settle timing (tests 1, 3, 4 above). 🤖 Generated with Claude Code |
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>
…work Merges origin/feat/record-zstack-acquisition (18 commits: widget layout changes + Record/Z-Stack settings reuse via YAML + drag-and-drop) into feat/multi-plane-recording (#579) so the stacked PR is unblocked. Conflicts resolved in: - control/core/record_zstack_controller.py: kept both the multi-plane recording_plane_offsets_um() helper and the settings-reuse _build_objective_info()/_save_record_zstack_yaml() helpers. - control/widgets.py: kept HEAD's Nz/dz/bottom-Z recording-plane redesign (which superseded the single entry_recording_z_offset field) and dropped the now-redundant old Z-offset widget from the other side's layout tweaks; updated _apply_yaml_settings() and its widgets_to_block list (a non-conflicting hunk that still referenced the removed widget/field) to use the new Nz/bottom-Z/dz widgets. - tests/core/test_record_zstack_worker.py and tests/test_record_zstack_widget.py: unioned both branches' new test functions (git's diff3 had misaligned several of them because they shared boilerplate setup code); updated the handful of assertions that referenced the pre-multi-plane recording_z_offset_um field. Also renamed RecordZStackYAMLData.recording_z_offset_um -> recording_bottom_z_offset_um + new recording_nz/recording_dz_um fields (and the corresponding acquisition.yaml recording: keys) in control/acquisition_yaml_loader.py, since the settings-reuse YAML schema needs to round-trip the multi-plane fields that superseded the single z-offset -- this wasn't flagged as a textual conflict but broke without the update. Updated tests/control/test_acquisition_yaml_loader.py to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a new Record + Z-Stack acquisition mode to the HCS GUI, enabling per-FOV sequential acquisition of (1) a continuous recording phase saved as per-FOV Zarr and (2) a multi-channel Z-stack saved as OME-Zarr, with shared acquisition mechanics refactored into a reusable worker base.
Changes:
- Introduces a recording pipeline (
StreamingCapture) with bounded backpressure and integrity sealing for incomplete captures. - Adds
RecordZStackController/RecordZStackWorkerplus GUI wiring for a new “Record + Z-Stack” tab. - Refactors shared capture/job-dispatch mechanics into
MultiPointWorkerBaseand adds a best-effort camera FPS hint API (set_frame_rate) including Toupcam support.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| software/control/acquisition_yaml_loader.py | Extends YAML parsing to support widget_type: record_zstack. |
| software/control/camera_toupcam.py | Adds Toupcam set_frame_rate() support and continuous-mode max FPS calculation. |
| software/control/core/acquisition_setup.py | Introduces shared helpers for pixel size calculation and experiment directory creation. |
| software/control/core/multi_point_controller.py | Uses shared experiment-dir creation helper. |
| software/control/core/multi_point_worker.py | Refactors shared acquisition mechanics into MultiPointWorkerBase. |
| software/control/core/record_zstack_controller.py | Adds controller for Record + Z-Stack acquisition setup/threading and YAML snapshotting. |
| software/control/core/record_zstack_worker.py | Adds worker implementing per-FOV recording + z-stack sequencing. |
| software/control/core/streaming_capture.py | Adds recording primitives (frame source/router/stop condition/writer) with bounded queues. |
| software/control/core/zarr_writer.py | Improves event-loop handling and expands abort sealing to distinguish incomplete vs aborted runs. |
| software/control/gui_hcs.py | Wires new Record + Z-Stack tab/widget/controller into the HCS GUI and well-selector behavior. |
| software/squid/abc.py | Adds AbstractCamera.set_frame_rate() API (default no-op returning max achievable). |
| software/squid/camera/utils.py | Implements set_frame_rate() and continuous pacing behavior in the simulated camera. |
| software/tests/control/test_HighContentScreeningGui.py | Adds GUI tests ensuring well-selector behavior on the new tab. |
| software/tests/control/test_acquisition_yaml_loader.py | Adds YAML loader tests for record_zstack acquisitions. |
| software/tests/core/init.py | Test package init (structure/support for new core tests). |
| software/tests/core/test_record_zstack_worker.py | Adds worker/controller smoke + behavior tests for Record + Z-Stack acquisition. |
| software/tests/core/test_streaming_capture.py | Adds extensive unit tests for the recording pipeline and its failure modes. |
| software/tests/test_acquisition_yaml_drop_mixin.py | Adds tests for YAML drop mixin generalizations supporting the 3rd widget type. |
| software/tests/test_record_zstack_camera_fps.py | Adds tests for camera FPS hinting/clamping (sim + Toupcam math). |
Comments suppressed due to low confidence (1)
software/control/acquisition_yaml_loader.py:133
- parse_acquisition_yaml() is annotated/documented to return AcquisitionYAMLData, but it now returns RecordZStackYAMLData when widget_type == "record_zstack". This makes the public API misleading and can break type-aware tooling (and any downstream code that relies on AcquisitionYAMLData-specific fields without checking widget_type first).
def parse_acquisition_yaml(file_path: str) -> AcquisitionYAMLData:
"""Parse acquisition YAML file and return structured data.
Args:
file_path: Path to the acquisition.yaml file
Returns:
AcquisitionYAMLData with parsed values
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| period_s = self._exposure_time_ms / 1000.0 | ||
| if self._target_frame_period_s is not None: | ||
| period_s = max(period_s, self._target_frame_period_s) | ||
| time_since = time.time() - last_frame_time | ||
| # use self._exposure_time and _acquisition_mode so as not to spam the logs, | ||
| # but this could case issues if subclassed for testing. | ||
| if ( | ||
| self._exposure_time_ms / 1000.0 | ||
| ) - time_since <= 0 and self._acquisition_mode == CameraAcquisitionMode.CONTINUOUS: | ||
| if time_since >= period_s and self._acquisition_mode == CameraAcquisitionMode.CONTINUOUS: |
| deadline = time.monotonic() + timeout_s | ||
| while True: | ||
| try: | ||
| self._q.put(_SENTINEL, timeout=min(1.0, max(0.1, timeout_s))) | ||
| break |
feat: multi-plane recording — N z-planes per FOV (stacks on #564)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
software/squid/camera/utils.py:312
- SimulatedCamera.set_frame_rate() clamps achievable fps using get_total_frame_time() (exposure + strobe), but the CONTINUOUS streaming loop paces frames using only exposure time. This can make the simulated camera deliver frames faster than the achievable fps it reports, which can mis-size recordings and invalidate tests that rely on achievable-fps behavior.
period_s = self._exposure_time_ms / 1000.0
if self._target_frame_period_s is not None:
period_s = max(period_s, self._target_frame_period_s)
time_since = time.time() - last_frame_time
if time_since >= period_s and self._acquisition_mode == CameraAcquisitionMode.CONTINUOUS:
| timeout_s = (self._exposure_time_ms / 1000.0) * 1.02 + 4 | ||
| deadline = time.time() + timeout_s | ||
| while self._current_frame is None and time.time() < deadline: | ||
| time.sleep(0.001) | ||
| return self._current_frame |
There was a problem hiding this comment.
fixed in lastest master
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
software/squid/camera/utils.py:312
- The simulated camera’s streaming loop paces frames using exposure time only, but set_frame_rate() clamps/returns an achievable rate using get_total_frame_time() (exposure + strobe). This makes the simulated camera’s actual free-run period inconsistent with the frame-time contract used elsewhere (and with the achievable fps that callers compute).
last_frame_time = time.time()
while self._continue_streaming:
period_s = self._exposure_time_ms / 1000.0
if self._target_frame_period_s is not None:
period_s = max(period_s, self._target_frame_period_s)
time_since = time.time() - last_frame_time
if time_since >= period_s and self._acquisition_mode == CameraAcquisitionMode.CONTINUOUS:
| # wrong. | ||
| non_hw_frame_timeout = 5 * self.camera.get_total_frame_time() / 1e3 + 2 | ||
| if not self._ready_for_next_trigger.wait(non_hw_frame_timeout): | ||
| self._log.error("Timed out waiting {non_hw_frame_timeout} [s] for a frame, aborting acquisition.") |
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>
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>
…d-trip test test_full_save_load_round_trip_preserves_settings previously ran with Laser AF off, which forces recording_bottom_z_offset_um to 0.0 in build_parameters() and left recording_Nz/recording_dz_um at their defaults -- so the multi-plane recording fields introduced by this branch were never actually exercised by the round trip. Enable Laser AF and set Nz/dz/bottom-Z to distinctive values so the save->parse->apply cycle is verified for these fields too.
…quisition Restores the plane-count/Z-range/per-FOV-duration summary that a prior UI change removed (it was an inline label, dropped for being too wide) as a QMessageBox.question shown right after validate() succeeds and before the acquisition actually starts, giving the user one last look. Answering No aborts the start and re-unchecks the button; the dialog is skipped entirely when the Recording phase is disabled. The summary's Z offset uses the same effective-value logic as build_parameters() (0.0 when Laser AF is off) so it can never show a stale value from the hidden entry_recording_bottom_z field that doesn't match what actually runs. Also fixes entry_recording_bottom_z's tooltip, which stayed frozen at "Bottom plane offset relative to the Z reference" even after Nz dropped to 1 and the caption switched to "Z offset:" -- it now tracks the same multi/ single wording as the label.
…offset moves to its own row Live-testing feedback: FPS/Duration should come first, and the Bottom-Z offset field (visible only when Laser AF is on) shouldn't share a row with Nz/dz/FPS/Dur since it shifts that row's width when it appears/disappears.
…yout Live-testing feedback: the offset row should line up with Nz, not start flush-left. Switched the recording-controls block from two independent QHBoxLayouts to one QGridLayout so the offset shares Nz's columns and aligns exactly regardless of font/platform metrics.
) 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>
Live-testing feedback across several rounds landed on: FPS/Duration always lead row 1; Nz/dz/offset append to row 1 too as long as at most 2 of them are visible, but move to their own row 2 (as a group) only when all 3 are shown at once (Nz>1 AND Laser AF on) -- that's too many fields for one row. When on row 2, Nz/dz get temporarily widened to match FPS/Dur's widths so the columns align exactly (both rows share the same left margin, so equal leading widths are sufficient -- no grid needed). Otherwise they use their own compact widths sized to the actual label/spinbox content, fixing an earlier pass that forced the wider shared width unconditionally and left visible dead space in the common single-row case. Also: dz now shows 1 decimal instead of 2, and both rows share one uniform inter-item spacing instead of a larger explicit gap between field groups.
…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>
…hecked phases Recording phase now starts checked (was unchecked) since it's the more commonly used mode. Also, unchecked phase groups (Recording, Z-Stack) now collapse their fields entirely instead of Qt's default checkable-QGroupBox behavior of graying them out while still showing and reserving space for them.
…dividers QGroupBox (even flat) renders a sunken box border under the Fusion style, inconsistent with WellplateMultiPointWidget's borderless-section convention. Switch Output/Wells+FOV/Recording/Z-Stack to plain QFrame with a bold QCheckBox header for the checkable sections, and add a thin sunken HLine divider between each of the widget's 4 sections so the boundaries stay visually clear.
…ettings - _build_start_group returned a QGroupBox, the one section still showing the sunken-box look the other 4 sections were switched away from; make it a plain QFrame like the rest. - Move the Save/Load Settings buttons out of the output group and into the start group (just above Start Acquisition), and hide them by default — drag-and-drop of an acquisition YAML already covers the same functionality without permanently using UI space.
… YAML load; fix double-indented content Code review of the QGroupBox->QFrame refactor found two regressions: - _apply_yaml_settings blocks checkbox_recording/checkbox_zstack's toggled signal while calling setChecked(), so the collapse-when-unchecked wiring in _build_recording_group/_build_zstack_group never fired on load, leaving each section's visible/hidden state stale relative to its checkbox. Store the content vbox/grid layout as attributes and resync visibility directly in the finally block, matching the existing checkbox_time/checkbox_xy pattern. Added a regression test. - Both sections' content layout set its own (4, 2, 4, 2) margins on top of the new outer_vbox's identical margins, double-indenting the channel table/fields relative to the checkbox header and the other 3 sections. Zero the inner layout's margins since the outer frame margin already applies.
recording_channel_table's fixed height was computed from rowHeight(0) before the row's cell widgets (combo/spinboxes/button, all taller than plain text) were placed, then padded by 8px to guess enough room for them — leaving visible empty space below the row once they were actually added. Move the height calculation after the cell widgets are set and call resizeRowsToContents() first, so the fixed height matches the row's real final height.
…d wrong channel - Recording/Z-Stack channel table headers were rendering bold (default header font weight); explicitly set them to normal weight. - _copy_recording_from_live and _copy_zstack_row_from_live read liveController.currentConfiguration — whatever channel happens to be active in the Live tab — instead of the settings for the channel actually selected in the recording row / z-stack row being refreshed. This silently switched the recording row's channel selection to match Live's active channel, and overwrote z-stack rows with the wrong channel's values whenever Live wasn't showing that row's channel. Both now look up settings via _channel_settings() for their own channel instead.
…RY header/test fixes Code review of e57dbb1d found: - _copy_recording_from_live/_copy_zstack_row_from_live switched to _channel_settings(name), which silently falls back to a hardcoded (50, 0, 50) triple when the channel isn't found (e.g. the objective changed elsewhere and the row's stale channel selection no longer exists) — a quiet data-reset the old currentConfiguration-based code never had. Use _find_channel() directly and leave the row untouched (with a warning) on a miss instead. Added a regression test. - test_copy_from_live_uses_selected_channels_own_settings didn't actually exercise the button's click handler, since selecting the channel via setCurrentText() already auto-seeds the same target values through _on_recording_channel_changed. Overwrite the spinboxes before clicking so the assertions can only pass if the click handler itself re-applies them. - Factored the duplicated 3-line header-unbold snippet (recording + z-stack tables) into a shared _set_header_not_bold() helper. - recording_channel_table's tightened fixed-height padding used a bare "+2" magic number; use 2 * frameWidth() (the table's actual top+bottom border) instead, so it stays correct if the frame style ever changes.
…trigger live can't break recording
Starting a recording while Live ran in SOFTWARE-trigger mode aborted the run on
real hardware. _probe_frame_shape() sized the recording dataset by sending a
software trigger, but Live's last trigger was still outstanding
(get_ready_for_trigger() == False), so send_trigger() was rejected ("trigger too
early"). The probe then fell back to get_resolution() — the raw binned sensor
size (3104x2084) — while the camera delivers cropped 2084x2084 frames, so every
write_frame failed with a dimension-alignment error and the store sealed
incomplete, fail-fasting the whole acquisition.
Probe in CONTINUOUS mode instead — the same mode recording uses — and read one
free-run frame (no send_trigger, so trigger readiness can't block it); the probed
shape is then exactly what recording writes. On a genuine probe failure, abort
with a clear error instead of sizing from get_resolution() and grinding out a
blank, incomplete recording.
Also fix SimulatedCamera.read_camera_frame() to wait for a frame (matching the
real camera / AbstractCamera contract) rather than returning None before the
first CONTINUOUS frame — otherwise the continuous probe gets no frame in sim.
Verified on a real ITR3CMOS26000KMA: the continuous probe returns (2084, 2084)
(matching delivery) vs get_resolution()'s (3104, 2084), and get_ready_for_trigger()
is False right after a software trigger (reproducing the reject). New tests cover
the probe using CONTINUOUS/never triggering and aborting (not get_resolution) on
no frame.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… enabled Previously the button was always clickable, and clicking with both phases disabled just surfaced validate()'s "At least one phase must be enabled" error via a dialog after the fact. Disable the button proactively instead, kept in sync via checkbox_recording/checkbox_zstack toggled signals and resynced after YAML load (same signal-blocking issue as the earlier Recording/Z-Stack section-visibility fix).
RecordZStackMultiPointWidget was the only multipoint tab setting QFrame.Panel | QFrame.Raised on itself — WellplateMultiPointWidget and FlexibleMultiPointWidget use the default QFrame.NoFrame. The raised panel gave this tab a visibly different embossed background/border compared to the others.
…ts own gray fill The raised-panel removal (9907f81) wasn't the actual cause: WellplateMultiPointWidget also sets QFrame.Panel | QFrame.Raised, but it doesn't use a QScrollArea. This widget's QScrollArea viewport and content QWidget both auto-fill an opaque palette-Window background, visibly darker than the tab pane under Fusion. Disable auto-fill on both so the pane's own background shows through, matching every other multipoint tab.
Rebasing this branch onto master replays its 86 commits linearly, which drops the 8 merge commits — including the two `Merge origin/master` merges and the feat/multi-plane-recording merges. The conflict resolutions those merges carried are not reproducible commit-by-commit, so replaying the older side of each pair silently reverted work that was already settled. This restores the tree to exactly what `git merge upstream/master` produces from the pre-rebase tip, i.e. the reviewed content of the PR: - multi_point_worker.py — keep `_abort_cause` / `_abort_due_to_error()` / `_run_state_beat()` hoisted into MultiPointWorkerBase, and the frame callback's `_abort_due_to_error()` calls, rather than the pre-merge `request_abort_fn()` form that the extract-base commit had moved verbatim. - widgets.py, acquisition_yaml_loader.py, record_zstack_controller.py — keep the multi-plane recording fields (Nz / dz / bottom-Z offset) that superseded the single `recording_z_offset_um`, plus the `_update_recording_planes_ui()` resync after a YAML load. - gui_hcs.py — drop the `emit_selected_channels()` call that a later commit had already removed in favour of `_tab_uses_well_selector()`. - tests — matching updates and test ordering. Verified: this branch's tree is now byte-identical to the clean merge of upstream/master into the pre-rebase tip (0afae24).
0afae24 to
d07b875
Compare
Summary
Adds a new Record + Z-Stack acquisition mode (its own tab, next to Wellplate Multipoint, gated by
ENABLE_RECORDING). It walks selected wells × a per-well FOV grid ×Nttime points and, at each FOV, runs two phases in sequence:T = round(fps × duration)frames at one Z plane, saved as a per-FOV Zarr(T, 1, 1, Y, X).(Nt, C, NZ, Y, X)per FOV.Both phases are positioned in Z by offsets relative to a reference plane (laser reflection AF when enabled+referenced, otherwise the current Z). Each phase is independently enable-able.
Architecture
MultiPointWorkerBase— behavior-preserving refactor lifting the shared capture mechanics (channel apply, single-frame capture, frame callback +SaveZarrJobdispatch, backpressure, abort/progress) out ofMultiPointWorker. After merging master, the acquisition-watchdog hooks (_abort_due_to_error, per-image run-state beats) live on the base so both workers classify error-aborts identically.StreamingCapture(control/core/streaming_capture.py) — the recording primitive: a continuous frame source → bounded queue (count- and byte-capped) → writer thread → directZarrWriter. Genericframe_source/frame_router/stop_conditionseam, shaped so a future hardware-sequenced source drops in.RecordZStackWorker/RecordZStackController— thin t×well×FOV loop reusing the inherited mechanics; recording viaStreamingCapture, z-stack via the existingSaveZarrJobpath. Timepoints are paced on the absolutet0 + k·dtgrid with grid-preserving skip (mirrorsMultiPointWorker); each experiment dir gets theacquisition_channels.yamlsnapshot and.donemarker like every multipoint acquisition.RecordZStackMultiPointWidget+gui_hcstab wiring. Per-well FOV grid configurable inline and rebuilt on tab entry/well clicks; channels editable inline with Copy-from-Live; channel combos refresh on objective/profile/config changes. XY/Time controls mirrorWellplateMultiPointWidget's tabbed row (see below).AbstractCamera.set_frame_rate(fps)(no-op default; ToupTekPRECISE_FRAMERATE; simulated camera honors it). The recording sizes its dataset, pacing, andtime_increment_sfrom the achievable rate the camera reports, with software downsampling as the portable safety net.Widget UI polish
WellplateMultiPointWidget(checkbox + combo in a frame that highlights orange/green when active), skipping Z since the widget's own Z-Stack phase group already covers z-stacking:combobox_xy_modenow offers two real modes: Select Wells (today's tile-a-grid-over-selected-wells behavior, default) and Current Position (single FOV at the live stage position, bypassing well selection entirely —validate()no longer requires a well selection in this mode). Unchecking XY forces Current Position and disables the combo; re-checking restores the previous mode.checkbox_timewraps Nt/dt: unchecked forces a single timepoint and hides the controls, restoring the previous Nt/dt on re-check.checkbox_laser_afmoved into this row as a plain checkbox, matching howWellplateMultiPointWidgetplaces its own Laser AF toggle.liveController.get_channels(), instead of a hardcoded 50 ms / 0 gain / 50%.centralWidget.setFixedWidth(minimumSizeHint())ingui_hcs.pysizes the whole app once at startup from whichever tab is active then) — in favor of a responsive fill plus tightened field widths, so the panel no longer needs a horizontal scrollbar. The Z-Stack channel table now spans the group's full width to match the Recording table, and z-stack channel names get a tooltip with the full name since the Channel column truncates.refresh_channel_list()was silently resetting the user's manually-edited recording exposure/gain/illumination back to the channel's base config (repopulating the combo transiently firescurrentIndexChanged); a checkbox toggle could rebuild the scan region twice; dark-theme text contrast on the new Time tab; duplicated channel-lookup logic consolidated into a shared_find_channel()helper.Data-integrity guarantees (recording pipeline)
These came out of the review rounds below and are worth stating as contract:
acquisition_complete: trueis only stamped when every expected frame was written. Write errors, backpressure drops, and under-delivery (camera stall) all seal the store incomplete with counts (write_errors,dropped_frames,captured_frames/expected_frames); user aborts additionally stampaborted: true.get_resolution()— sensor-vs-delivered mismatches previously produced silently blank recordings on real cameras.Review & hardening
Three review rounds (multi-agent adversarial review with independent verification, plus hand-verification when API limits interrupted round 2) — 40 confirmed findings, all fixed, each with a test that failed first:
get_resolution()sizing, error-swallowing drain, finalize deadlock, unresponsive abort, trigger/channel state corruption after runs, fps-gate jitter halving capture rate, count-only queue bound (~13 GB RAM risk), per-FOV z coords dropped, dt pacing/metadata mismatch, missing cleanup/bookkeeping.Testing
StreamingCapture/RecordingWriter/RecordingRouter(error/OOB/abort/wedge/drop/jitter/burst paths), worker simulation (shapes, state restore, pacing, fail-fast, bookkeeping), widget (validation, Copy-from-Live, refresh, Start/Stop handoff, FOV-grid wiring), gui-level tab/selector behavior, camera fps.Nt=2t-indexing, phase-solo runs, z-offset stage return, back-to-back acquisitions.install-and-testsuite) after resolving the master conflict — note the PR had beenCONFLICTING, which silently skipspull_requestworkflows, so earlier pushes only ran lint.blackclean (CI 25.12.0).Hardware testing (pending — the remaining merge gate)
See the test-plan comment: instrument checklist + a camera-only bench mode (real ToupTek camera, everything else simulated via the
[SIMULATION]INI section). Highest-value bench test: thePRECISE_FRAMERATEfps hint, which has never executed against real hardware.Deferred follow-ups (documented, out of scope here)
JobRunnermixin, and replacing the duck-typed record-tab stubs (display_progress_bar,emit_selected_channels) with a protocol.🤖 Generated with Claude Code