fix: restore legacy RecordingWidget Record button#535
Open
Alpaca233 wants to merge 12 commits into
Open
Conversation
The Record button always short-circuited with a "Please choose base saving directory first" modal even though DEFAULT_SAVING_PATH was already populated, because base_path_is_set was never flipped after the default path was applied. Also, cancelling the Browse dialog clobbered the existing path with an empty string. Drop the dead flag and gating modal (DEFAULT_SAVING_PATH is normalized in _def.py and always valid), and make set_saving_dir a no-op on cancel. Add regression tests covering construction, Record-without- Browse, Browse-cancel, and end-to-end frame persistence. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Switch the RecordingWidget fixture to monkeypatch for DEFAULT_SAVING_PATH (automatic restore on test failure), drop the dead `is_live` cell, hoist imports, and rely on observable widget/saver state instead of reaching through `stream_handler._handler.save_image_flag`. Replace the manual `time.sleep` polling with `qtbot.waitUntil` so it pumps the Qt event loop, and use `qtbot.mouseClick` to also exercise the Record button's clicked signal wiring. Tighten file-search to `Path.rglob`. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The in-code default for individual-image saves becomes TIFF for both multipoint and the recording widget. Users with an [ACQUISITION] image_format= override in their INI keep getting whatever they set — this only changes the source default. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Cross-thread callable returning the active AcquisitionChannel; saver thread calls it per frame. Plumbing only — process_queue still uses the inline imwrite path until the next commit. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
process_queue stops doing inline iio.imwrite / cv2.imwrite and instead calls utils_acquisition.save_image(), inheriting dtype-based extension (uint16 -> tiff, else Acquisition.IMAGE_FORMAT), channel-aware filenames, BF-LED RGB->GRAY conversion, and pseudo-color from the multipoint pipeline. A small _DefaultRecordingChannel sentinel handles the case where no channel provider is registered. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
One row per saved frame: frame_id, timestamp_iso, channel, exposure_ms, gain, illumination_intensity, file (relative path). Lets users identify recordings by metadata long after capture and provides bidirectional lookup between filename and frame_id. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
- start_new_experiment: ERROR + raise on dir-create failure (was silent) - enqueue: throttled WARNING with dropped-count tracking (was print) - process_queue: ERROR + stop on OSError; WARNING per per-frame failure - close(): INFO summary line with frames_saved, dropped, duration - start: INFO line plus a one-time WARNING if no channel_provider is set WARNING/ERROR records surface via the existing WarningErrorWidget panel. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
RecordingWidget gains a channel_provider constructor kwarg; gui_hcs passes a lambda returning liveController.currentConfiguration. On Record-press the widget registers the provider on the saver; on stop (button or auto via stop_recording signal) it clears it. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restores the legacy “Simple Recording” widget behavior so the Record button works out-of-the-box with DEFAULT_SAVING_PATH, and modernizes single-camera recording output to match the multipoint acquisition saving path (TIFF-by-default, channel-aware filenames, and a per-recording frames.csv sidecar).
Changes:
- Fixes
RecordingWidgetdirectory selection logic so recording can start without an explicit browse, and canceling the browse dialog no longer clobbers the current saving directory. - Updates
ImageSaverto write frames viautils_acquisition.save_image()and emit per-frame metadata toframes.csv, with improved logging for write/queue issues. - Flips the in-code default
Acquisition.IMAGE_FORMATfrom"bmp"to"tiff"and adds regression tests for recording + default format.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
software/control/widgets.py |
Restores legacy RecordingWidget behavior (no blocking modal), adds optional channel provider wiring, and fixes browse-cancel behavior. |
software/control/gui_hcs.py |
Passes a channel provider from the live controller into RecordingWidget for per-frame metadata. |
software/control/core/core.py |
Refactors ImageSaver to use save_image(), writes frames.csv, and adds logging/backpressure tracking. |
software/control/_def.py |
Switches default acquisition image format to TIFF. |
software/tests/control/test_widgets.py |
Adds focused regression tests for RecordingWidget + ImageSaver behavior, filenames, CSV, and logging paths. |
software/tests/control/test_def.py |
Adds a test asserting the source default for IMAGE_FORMAT is TIFF. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+195
to
+200
| self.queue.task_done() | ||
| self.image_lock.release() | ||
| except: | ||
| pass | ||
| except OSError as e: | ||
| log.error(f"Writer fatal error: {e}; stopping recording") | ||
| self.stop_recording.emit() | ||
| except Exception as e: | ||
| log.warning(f"Failed to write frame {frame_id}: {e}") |
Comment on lines
+239
to
+242
| csv_path = os.path.join(experiment_dir, "frames.csv") | ||
| self._csv_file = open(csv_path, "w", newline="") | ||
| self._csv_writer = csv.writer(self._csv_file) | ||
| self._csv_writer.writerow( |
Comment on lines
203
to
+211
| try: | ||
| self.queue.put_nowait([image, frame_ID, timestamp]) | ||
| if (self.recording_time_limit > 0) and ( | ||
| time.time() - self.recording_start_time >= self.recording_time_limit | ||
| ): | ||
| self.stop_recording.emit() | ||
| # when using self.queue.put(str_), program can be slowed down despite multithreading because of the block and the GIL | ||
| except: | ||
| print("imageSaver queue is full, image discarded") | ||
| self.queue.put_nowait([image, frame_id, timestamp]) | ||
| except Exception: | ||
| self._dropped_count += 1 | ||
| now = time.time() | ||
| if now - self._last_queue_full_warning_ts >= 1.0: | ||
| log.warning(f"Image queue full; frame {frame_id} dropped") | ||
| self._last_queue_full_warning_ts = now | ||
| return |
Comment on lines
4499
to
4503
| self.streamHandler.start_recording() | ||
| else: | ||
| self.streamHandler.stop_recording() | ||
| self.imageSaver.set_channel_provider(None) | ||
| self.lineEdit_experimentID.setEnabled(True) |
Four related fixes from PR #535 review: 1. queue.task_done() now runs in finally; a write exception no longer leaves the item un-acked, which would have hung close()'s queue.join() indefinitely. 2. Extract stop_experiment(): closes frames.csv and logs the per-recording summary. Makes the Stop button (and time-limit auto-stop) finalize the CSV instead of leaving it open until app shutdown. 3. enqueue() now catches queue.Full specifically (was blanket Exception), so unrelated errors aren't silently misreported as queue saturation. 4. start_new_experiment() defensively calls stop_experiment() to close any prior CSV — protects against multi-record sessions leaking handles if a caller forgets to stop. The widget's toggle_recording() and stop_recording() now both call imageSaver.stop_experiment() so a single Record/Stop cycle produces a finalized CSV plus a summary log line, instead of one CSV per session. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
- Drop test_channel_provider_can_be_set_and_cleared (trivial slot getter/setter; the same code path is exercised through the widget by test_widget_constructed_with_channel_provider). - Merge test_multiple_recordings_close_previous_csv and test_stop_experiment_finalizes_csv_and_logs_summary into a single test_recording_lifecycle_finalizes_csv_and_starts_fresh that walks start -> stop (assert closed + summary log) -> start (assert new CSV). No coverage loss; less ceremony. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Two bugs reported on PR #535: 1. 'Writer fatal error: directory doesn't exist' — Stop clears experiment_ID="" but the saver thread can still have buffered frames to process. Subsequent writes resolve to <base>/<file_id>/, a path that was never created. stop_experiment() now drains the queue via queue.join() before tearing down state, so buffered frames land in the right experiment dir. 2. Recording FPS dropped to 2-3. utils_acquisition.save_image() routes every frame through imageio.imwrite, which is ~10x slower than cv2.imwrite for uint8 frames. Plus csv_file.flush() per row added an fsync per frame. Now we use get_image_filepath() for channel-aware naming consistency with the multipoint pipeline but write directly with cv2.imwrite/iio.imwrite (legacy fast path); CSV is flushed only on stop/close. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
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
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
Restores the legacy Simple Recording widget and gives it scientifically usable output.
Bugfix (original PR):
base_path_is_setflag that was never flipped afterDEFAULT_SAVING_PATHwas applied, so clicking Record always short-circuited with a "Please choose base saving directory first" modal.set_saving_diris now a no-op when the user cancels the file dialog (it previously clobbered the existing path).New: metadata, logging, and TIFF default
Acquisition.IMAGE_FORMATdefault flipped from"bmp"to"tiff"(_def.py:119). BMP path stays available via INI override.ImageSavernow delegates per-frame writes toutils_acquisition.save_image()— same code path the multipoint pipeline uses, with channel-aware filenames, BF-LED conversion, and pseudo-color for free.frames.csvsidecar emitted per recording:frame_id, timestamp_iso, channel, exposure_ms, gain, illumination_intensity, file.RecordingWidgetaccepts achannel_providercallable;gui_hcs.pypasseslambda: liveController.currentConfigurationso per-frame channel metadata is captured automatically.except: pass/printpaths replaced withsquid.logging:OSErrorduring writeWarningErrorWidgetpanel.Notes
ENABLE_RECORDINGin_def.py(defaultfalse). To see the tab, addenable_recording = trueto yourconfiguration*.ini.base_path_is_setpattern still exists in a few other widgets in this file; left alone here to keep the fix scoped.Test plan
QT_QPA_PLATFORM=offscreen pytest tests/control/test_widgets.py tests/control/test_def.py— 167 passedblack --config pyproject.toml --check— clean across_def.py,core/core.py,widgets.py,gui_hcs.py,tests/control/test_widgets.py,tests/control/test_def.pyenable_recording = true, click Record, confirmframes.csv+ per-frame TIFF files under~/Downloads/<exp>_<timestamp>/0/. Toggle the live channel mid-recording and confirm both channel names appear in filenames.🤖 Generated with Claude Code