[feat] Plugin to acquire a z-stack by moving the stage#3374
[feat] Plugin to acquire a z-stack by moving the stage#3374pieleric wants to merge 1 commit intodelmic:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a new plugin StageZStackPlugin that enables Z-stack acquisitions using the microscope stage z-axis. The plugin registers a menu entry, validates presence of a stage with z movement and a Localization tab with streams, pauses live streams, stores the original stage position, generates z levels from UI parameters, temporarily maps stream focus control to the stage, and initiates an asynchronous acquireZStack. On completion or error it restores focuser and stage position, processes and saves results with a timestamped filename via a selected exporter, and provides progress, warnings, error dialogs, and logging. Sequence Diagram(s)sequenceDiagram
actor User
participant GUI as GUI/Plugin
participant Stage as Stage (z-axis)
participant Streams as Optical Streams (Fluo/Brightfield)
participant AcqMng as Acquisition Manager
participant DataIO as Data IO Exporter
User->>GUI: Click "Acquisition/Stage Z-Stack..."
rect rgba(100,150,200,0.5)
Note over GUI: Validation
GUI->>GUI: Verify Localization tab active
GUI->>Streams: Check streams exist
GUI->>Stage: Verify z-axis available
end
rect rgba(100,200,150,0.5)
Note over GUI: Preparation
GUI->>Streams: Pause live acquisition
GUI->>Stage: Store original position
GUI->>GUI: Generate z-levels (zMin, zMax, zStep)
end
rect rgba(200,150,100,0.5)
Note over GUI: Acquisition
GUI->>Streams: Temporarily swap focuser to Stage
GUI->>AcqMng: Initiate acquireZStack(levels, streams)
AcqMng->>Stage: Move to next z-level
AcqMng->>Streams: Capture images per stream at level
AcqMng-->>GUI: Progress updates
end
rect rgba(150,100,200,0.5)
Note over GUI: Completion & Saving
AcqMng-->>GUI: Acquisition complete / future resolved
GUI->>Streams: Restore original focuser
GUI->>Stage: Return to original position
GUI->>DataIO: Select converter, export data (timestamped filename)
GUI-->>User: Show completion / warnings / errors
end
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
plugins/zstack_stage.py (3)
41-46: Missing class docstring and type hints on several methods.Per the coding guidelines, all classes need docstrings (reST style) and all functions need type hints for parameters and return types. Currently missing:
- Class-level docstring for
StageZStackPlugin- Type hints on
__init__,start,_acquire,_on_acquisition_done(parameters and return types)As per coding guidelines, "Always use type hints for function parameters and return types in Python code" and "Include docstrings for all functions and classes, following the reStructuredText style guide".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/zstack_stage.py` around lines 41 - 46, The class StageZStackPlugin lacks a reStructuredText docstring and several methods are missing type hints; add a class-level reST docstring describing purpose/usage to StageZStackPlugin and add explicit type hints for parameters and return types on __init__, start, _acquire, and _on_acquisition_done (use appropriate types such as self, optional/Union types or specific classes used in the module, and -> None or the correct return type), and update any internal variable annotations if needed so signatures are fully annotated per the coding guidelines.
26-26: Minor: use built-in generics for Python 3.10+.Since the guidelines specify Python 3.10+, you can use
dictandlistdirectly as generic types instead of importingDictandListfromtyping.As per coding guidelines, "Ensure code is valid for Python 3.10 and above".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/zstack_stage.py` at line 26, Remove the typing import of Dict and List and switch all uses of those annotations in this module to the built-in generic types (dict, list) supported on Python 3.10+; locate the import statement ("from typing import Dict, List") and replace references to Dict[...] and List[...] in functions/classes (e.g., any parameter or return annotations in this file) with dict[...] and list[...] and then delete the unused typing import.
118-118: Redundant exception object inlogging.exceptioncalls (also lines 166, 249, 276).
logging.exceptionalready captures and prints the current exception's traceback. Passingevia%sduplicates the message. Use a plain string instead:- logging.exception("Failed to start stage z-stack acquisition: %s", e) + logging.exception("Failed to start stage z-stack acquisition")Same pattern applies at lines 166, 249, and 276.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/zstack_stage.py` at line 118, The review flags redundant exception objects passed to logging.exception; remove the "%s" format and the exception variable so logging.exception only receives the plain message (e.g., replace logging.exception("Failed to start stage z-stack acquisition: %s", e) with logging.exception("Failed to start stage z-stack acquisition")). Apply the same fix for the other logging.exception calls flagged (the calls at the other occurrences in plugins/zstack_stage.py) so none pass the caught exception as a formatting argument; alternatively, if you prefer explicit control, use logging.error("message", exc_info=True) but do not pass the exception into the format string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/zstack_stage.py`:
- Around line 190-196: The code sets self.main_data.is_acquiring.value = True
before calling acqmng.acquireZStack and registering the done callback, so if
acquireZStack or subsequent lines raise the GUI flag remains True; fix by either
(a) moving the is_acquiring assignment until after acquireZStack returns and
after add_done_callback is registered on self._acq_future, or (b) wrap the
acquireZStack + add_done_callback block in a try/except that on any exception
resets self.main_data.is_acquiring.value = False and re-raises/logs the error;
target the symbols self.main_data.is_acquiring, self._acq_future, acquireZStack,
add_done_callback, and the start() caller to implement the change.
- Around line 259-276: The _save_data method builds the output filename with a
fragile f-string and ignores the existing create_filename utility and GUI
patterns; replace the manual basename/filename assembly with a call to
create_filename (passing config, basename or equivalent pattern) so you get
collision-avoidance and the user-configured filename pattern, or at minimum
construct the path with os.path.join(config.last_path,
f"{basename}{config.last_extension}") instead of f-string; update calls around
get_acqui_conf and dataio.find_fittest_converter to use that returned filename
and remove the unused create_filename import if you opt not to call it.
- Around line 130-143: The code overwrites self._focuser for each stream in
_acquire, losing per-stream original focusers; initialize a per-stream mapping
(e.g., self._original_focusers = {}) in __init__, then in _acquire for each
FluoStream/BrightfieldStream save the original focuser into that mapping keyed
by the stream object (self._original_focusers[s] = s._focuser) and set
s._focuser = self.stage without touching self._focuser, and in
_on_acquisition_done iterate the mapping to restore each stream's focuser
(s._focuser = self._original_focusers[s]) and then clear the mapping.
- Around line 165-185: The _acquire method currently swaps stream.focusers to
self.stage before validating z-levels, so if generate_zlevels or the zlevels
emptiness check fails the method returns without restoring focusers; to fix,
either move the focuser swap loop (the code that assigns s.focuser = self.stage
for streams) to after the call to generate_zlevels and after the zlevels empty
check, or keep the swap where it is but add guaranteed restoration (restore
original focuser objects on all early-return paths including the exception
handler around generate_zlevels and the "no optical streams" branch) so streams
do not remain bound to self.stage. Ensure changes reference the _acquire
function, the generate_zlevels call, the zlevels dict creation, and the
focuser-swap loop to locate the code to move or add restoration.
- Around line 275-276: The except block that currently logs failures (the except
Exception as e around exporter.export) silently swallows errors and must notify
the user; update the exception handler that catches exceptions from
exporter.export (and related save steps) to display an error dialog with the
error message/traceback (e.g., via the application's UI error dialog helper or
QMessageBox) and ensure the error is propagated or a non-success state is
returned so the caller knows the save failed. Keep the logging.exception call,
but add a user-visible dialog call (include the exception text) and then either
re-raise the exception or return/raise a specific SaveFailed error so the loss
of acquisition data is not silent.
---
Nitpick comments:
In `@plugins/zstack_stage.py`:
- Around line 41-46: The class StageZStackPlugin lacks a reStructuredText
docstring and several methods are missing type hints; add a class-level reST
docstring describing purpose/usage to StageZStackPlugin and add explicit type
hints for parameters and return types on __init__, start, _acquire, and
_on_acquisition_done (use appropriate types such as self, optional/Union types
or specific classes used in the module, and -> None or the correct return type),
and update any internal variable annotations if needed so signatures are fully
annotated per the coding guidelines.
- Line 26: Remove the typing import of Dict and List and switch all uses of
those annotations in this module to the built-in generic types (dict, list)
supported on Python 3.10+; locate the import statement ("from typing import
Dict, List") and replace references to Dict[...] and List[...] in
functions/classes (e.g., any parameter or return annotations in this file) with
dict[...] and list[...] and then delete the unused typing import.
- Line 118: The review flags redundant exception objects passed to
logging.exception; remove the "%s" format and the exception variable so
logging.exception only receives the plain message (e.g., replace
logging.exception("Failed to start stage z-stack acquisition: %s", e) with
logging.exception("Failed to start stage z-stack acquisition")). Apply the same
fix for the other logging.exception calls flagged (the calls at the other
occurrences in plugins/zstack_stage.py) so none pass the caught exception as a
formatting argument; alternatively, if you prefer explicit control, use
logging.error("message", exc_info=True) but do not pass the exception into the
format string.
| def _acquire(self): | ||
| """Start the z-stack acquisition""" | ||
| # Get acquisition streams | ||
| acq_streams = self.tab_data.acquisitionStreams.value | ||
| for s in acq_streams: | ||
| # Hack: we temporarily change .focuser of the streams. | ||
| # This is a little dangerous, as if something goes wrong, the streams | ||
| # stay as-is. The better way would be to duplicate the streams, but | ||
| # that's not so simple with the different types of streams, and copying | ||
| # the values of the VAs. | ||
| if isinstance(s, (FluoStream, BrightfieldStream)): | ||
| self._focuser = s._focuser | ||
| s._focuser = self.stage | ||
|
|
There was a problem hiding this comment.
Bug: only the last stream's original focuser is saved, corrupting restore for multi-stream setups.
self._focuser is overwritten on every iteration (line 141), so if there are multiple optical streams with different focusers, only the last one's focuser is remembered. On restore (line 222), every stream gets that same focuser — silently misconfiguring any stream whose original focuser differed.
Store a per-stream mapping instead.
Proposed fix
In __init__, change the field initialisation:
- self._focuser = None
+ self._original_focusers: Dict[Stream, object] = {}In _acquire, save per-stream:
for s in acq_streams:
if isinstance(s, (FluoStream, BrightfieldStream)):
- self._focuser = s._focuser
+ self._original_focusers[s] = s._focuser
s._focuser = self.stageIn _on_acquisition_done, restore per-stream:
for s in acq_streams:
- if isinstance(s, (FluoStream, BrightfieldStream)):
- s._focuser = self._focuser
+ if s in self._original_focusers:
+ s._focuser = self._original_focusers.pop(s)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/zstack_stage.py` around lines 130 - 143, The code overwrites
self._focuser for each stream in _acquire, losing per-stream original focusers;
initialize a per-stream mapping (e.g., self._original_focusers = {}) in
__init__, then in _acquire for each FluoStream/BrightfieldStream save the
original focuser into that mapping keyed by the stream object
(self._original_focusers[s] = s._focuser) and set s._focuser = self.stage
without touching self._focuser, and in _on_acquisition_done iterate the mapping
to restore each stream's focuser (s._focuser = self._original_focusers[s]) and
then clear the mapping.
| except (ValueError, IndexError, ZeroDivisionError, KeyError) as e: | ||
| logging.exception("Failed to generate z-levels: %s", e) | ||
| return | ||
|
|
||
| # Only apply z-stack to optical streams (not SEM streams) | ||
| zlevels: Dict[Stream, List[float]] = { | ||
| s: levels for s in acq_streams | ||
| if isinstance(s, (FluoStream, BrightfieldStream)) | ||
| } | ||
|
|
||
| if not zlevels: | ||
| logging.warning("No optical streams found for z-stack acquisition") | ||
| dlg = wx.MessageDialog( | ||
| self.main_app.main_frame, | ||
| "No optical streams (Fluorescence or Brightfield) available for z-stack acquisition.", | ||
| "Stage Z-Stack acquisition not possible", | ||
| wx.OK | wx.ICON_STOP | ||
| ) | ||
| dlg.ShowModal() | ||
| dlg.Destroy() | ||
| return |
There was a problem hiding this comment.
Bug: early returns in _acquire leave streams with a corrupted focuser.
If generate_zlevels raises (line 165) or zlevels is empty (line 175), the method returns without restoring the focusers that were already swapped at lines 140-142. The streams will permanently use self.stage as their focuser.
Either move the focuser swap to after all validation succeeds, or add focuser restoration in these early-return paths.
Simplest fix: move focuser swap after validation
Reorder _acquire so that z-level generation and the zlevels emptiness check happen before the focuser swap loop:
def _acquire(self):
"""Start the z-stack acquisition."""
acq_streams = self.tab_data.acquisitionStreams.value
+ # Generate z-levels first (before modifying any stream state)
+ try:
+ zmin = self.tab_data.zMin.value
+ zmax = self.tab_data.zMax.value
+ zstep = self.tab_data.zStep.value
+ levels = generate_zlevels(self.stage, (zmin, zmax), zstep)
+ except (ValueError, IndexError, ZeroDivisionError, KeyError) as e:
+ logging.exception("Failed to generate z-levels")
+ return
+
+ zlevels: Dict[Stream, List[float]] = {
+ s: levels for s in acq_streams
+ if isinstance(s, (FluoStream, BrightfieldStream))
+ }
+ if not zlevels:
+ # ... show dialog ...
+ return
+
+ # Now safe to swap focusers
for s in acq_streams:
if isinstance(s, (FluoStream, BrightfieldStream)):
- self._focuser = s._focuser
+ self._original_focusers[s] = s._focuser
s._focuser = self.stage
-
- # ... (move z-level generation and validation above) ...🧰 Tools
🪛 Ruff (0.15.1)
[warning] 166-166: Redundant exception object included in logging.exception call
(TRY401)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/zstack_stage.py` around lines 165 - 185, The _acquire method
currently swaps stream.focusers to self.stage before validating z-levels, so if
generate_zlevels or the zlevels emptiness check fails the method returns without
restoring focusers; to fix, either move the focuser swap loop (the code that
assigns s.focuser = self.stage for streams) to after the call to
generate_zlevels and after the zlevels empty check, or keep the swap where it is
but add guaranteed restoration (restore original focuser objects on all
early-return paths including the exception handler around generate_zlevels and
the "no optical streams" branch) so streams do not remain bound to self.stage.
Ensure changes reference the _acquire function, the generate_zlevels call, the
zlevels dict creation, and the focuser-swap loop to locate the code to move or
add restoration.
| self.main_data.is_acquiring.value = True | ||
|
|
||
| self._acq_future = acqmng.acquireZStack( | ||
| acq_streams, | ||
| zlevels, | ||
| self.main_data.settings_obs | ||
| ) |
There was a problem hiding this comment.
is_acquiring not reset if acquireZStack or subsequent lines raise.
If anything between line 190 (where is_acquiring is set to True) and the add_done_callback (line 210) throws, the exception propagates to start()'s except block (line 117), which does not reset is_acquiring. The GUI will remain in the "acquiring" state indefinitely.
Consider wrapping lines 190–210 in a try/except that resets is_acquiring on failure, or setting is_acquiring only after acquireZStack returns successfully and its callback is registered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/zstack_stage.py` around lines 190 - 196, The code sets
self.main_data.is_acquiring.value = True before calling acqmng.acquireZStack and
registering the done callback, so if acquireZStack or subsequent lines raise the
GUI flag remains True; fix by either (a) moving the is_acquiring assignment
until after acquireZStack returns and after add_done_callback is registered on
self._acq_future, or (b) wrap the acquireZStack + add_done_callback block in a
try/except that on any exception resets self.main_data.is_acquiring.value =
False and re-raises/logs the error; target the symbols
self.main_data.is_acquiring, self._acq_future, acquireZStack, add_done_callback,
and the start() caller to implement the change.
| def _save_data(self, data: List) -> None: | ||
| """Save the acquired data to a file""" | ||
| try: | ||
| # Generate filename with timestamp | ||
| config = get_acqui_conf() | ||
| basename = time.strftime("%Y%m%d-%H%M%S-stage-zstack") | ||
| filename = f"{config.last_path}/{basename}{config.last_extension}" | ||
|
|
||
| logging.info("Saving stage z-stack data to: %s", filename) | ||
|
|
||
| # Find appropriate exporter and save | ||
| exporter = dataio.find_fittest_converter(filename) | ||
| exporter.export(filename, data) | ||
|
|
||
| logging.info("Data saved successfully to: %s", filename) | ||
|
|
||
| except Exception as e: | ||
| logging.exception("Failed to save data: %s", e) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unused import create_filename; hand-rolled path is fragile and may overwrite files.
create_filename (imported at line 38) is never called. Instead, the path is assembled with an f-string (line 265), which:
- Skips the collision-avoidance logic that
create_filenameprovides. - Uses string interpolation instead of
os.path.join, risking malformed paths on some platforms. - Ignores the user-configured filename pattern from the GUI.
Consider using create_filename (or at minimum os.path.join) to build the output path, and drop the unused import if you don't plan to use it.
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 276-276: Redundant exception object included in logging.exception call
(TRY401)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/zstack_stage.py` around lines 259 - 276, The _save_data method builds
the output filename with a fragile f-string and ignores the existing
create_filename utility and GUI patterns; replace the manual basename/filename
assembly with a call to create_filename (passing config, basename or equivalent
pattern) so you get collision-avoidance and the user-configured filename
pattern, or at minimum construct the path with os.path.join(config.last_path,
f"{basename}{config.last_extension}") instead of f-string; update calls around
get_acqui_conf and dataio.find_fittest_converter to use that returned filename
and remove the unused create_filename import if you opt not to call it.
| except Exception as e: | ||
| logging.exception("Failed to save data: %s", e) |
There was a problem hiding this comment.
Save failure is silently swallowed — user loses data without notification.
If exporter.export (or any other step) raises, the exception is logged but no dialog is shown to the user. The acquisition data is lost without any visible feedback. This is especially problematic because the acquisition may have taken a long time.
Proposed fix: show an error dialog on save failure
except Exception as e:
logging.exception("Failed to save data: %s", e)
+ dlg = wx.MessageDialog(
+ self.main_app.main_frame,
+ f"Failed to save acquisition data:\n{e}",
+ "Stage Z-Stack save failed",
+ wx.OK | wx.ICON_ERROR,
+ )
+ dlg.ShowModal()
+ dlg.Destroy()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| logging.exception("Failed to save data: %s", e) | |
| except Exception as e: | |
| logging.exception("Failed to save data: %s", e) | |
| dlg = wx.MessageDialog( | |
| self.main_app.main_frame, | |
| f"Failed to save acquisition data:\n{e}", | |
| "Stage Z-Stack save failed", | |
| wx.OK | wx.ICON_ERROR, | |
| ) | |
| dlg.ShowModal() | |
| dlg.Destroy() |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 276-276: Redundant exception object included in logging.exception call
(TRY401)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/zstack_stage.py` around lines 275 - 276, The except block that
currently logs failures (the except Exception as e around exporter.export)
silently swallows errors and must notify the user; update the exception handler
that catches exceptions from exporter.export (and related save steps) to display
an error dialog with the error message/traceback (e.g., via the application's UI
error dialog helper or QMessageBox) and ensure the error is propagated or a
non-success state is returned so the caller knows the save failed. Keep the
logging.exception call, but add a user-visible dialog call (include the
exception text) and then either re-raise the exception or return/raise a
specific SaveFailed error so the loss of acquisition data is not silent.
There was a problem hiding this comment.
Pull request overview
This PR adds a new plugin for the METEOR and MIMAS microscopes that enables z-stack acquisition by moving the sample stage's z-axis instead of the traditional focus actuator. The plugin provides a menu option "Acquisition/Stage Z-Stack..." that reuses the z-stack parameters from the Localization tab but redirects the z-axis movement to the stage.
Changes:
- Added a new plugin that temporarily replaces the focuser with the stage in optical streams (FluoStream, BrightfieldStream) to perform z-stack acquisitions
- Implemented automatic saving of acquired z-stack data with timestamped filenames
- Integrated progress tracking via the existing acquisition gauge in the Localization tab
| # Restore stage position | ||
| self.stage.moveAbs(self._original_stage_pos) |
There was a problem hiding this comment.
In the exception handler at line 128, stage.moveAbs() is called without waiting for the result or handling potential errors. If the stage movement fails, the exception will be silently ignored or propagate unexpectedly. Consider using moveAbs().result() with appropriate error handling, or at minimum log if the restoration fails.
| # Restore stage position | |
| self.stage.moveAbs(self._original_stage_pos) | |
| # Restore stage position, waiting for completion and logging any failure | |
| try: | |
| move_future = self.stage.moveAbs(self._original_stage_pos) | |
| # moveAbs may return a future; if so, wait for completion to detect errors | |
| if hasattr(move_future, "result"): | |
| move_future.result() | |
| except Exception as restore_error: | |
| logging.exception( | |
| "Failed to restore stage position after failed z-stack acquisition: %s", | |
| restore_error, | |
| ) |
plugins/zstack_stage.py
Outdated
| logging.warning("Acquisition completed with exceptions: %s", exp) | ||
| dlg = wx.MessageDialog( | ||
| self.main_app.main_frame, | ||
| f"Acquisition completed with warnings:\n{exp}", |
There was a problem hiding this comment.
The error message at line 236 directly exposes the exception object in the user-facing dialog. This could display technical details that are confusing to users. Consider providing a more user-friendly error message while still logging the full exception details. For example: "Acquisition completed but some data could not be processed. Check the logs for details."
| data, exp = future.result() | ||
|
|
||
| if exp: |
There was a problem hiding this comment.
The variable 'exp' is referenced in the success path at line 232, but it's only defined inside the try block after future.result(). If future.result() doesn't return a tuple (which would be unexpected but possible in error conditions), 'exp' would be undefined. Consider initializing 'exp = None' before the try block to ensure it's always defined.
| def _save_data(self, data: List) -> None: | ||
| """Save the acquired data to a file""" |
There was a problem hiding this comment.
The docstring for _save_data is too brief and doesn't follow the reStructuredText style. According to the custom coding guidelines, docstrings should include parameter descriptions and any exceptions that might be raised. The docstring should describe the 'data' parameter and note that exceptions are caught and logged but not re-raised.
| except (ValueError, IndexError, ZeroDivisionError, KeyError) as e: | ||
| logging.exception("Failed to generate z-levels: %s", e) | ||
| return |
There was a problem hiding this comment.
If an exception occurs in _acquire() before the acquisition future is created (e.g., during z-level generation), the stage position will not be restored because the restoration only happens in _on_acquisition_done callback (line 226). The exception handler in start() at line 128 attempts to restore the stage position, but this doesn't cover cases where _acquire() returns early without raising an exception (e.g., lines 167, 185). Consider adding a finally block or ensuring stage restoration happens in all exit paths from _acquire().
|
|
||
| # Restore original stage position | ||
| logging.debug("Restoring stage to original position: %s", self._original_stage_pos) | ||
| self.stage.moveAbs(self._original_stage_pos) |
There was a problem hiding this comment.
The stage position restoration at line 226 should be wrapped in a try-except block. If the stage movement fails (e.g., hardware error), the exception would propagate and prevent the completion dialog from showing. This could leave the user confused about whether the acquisition succeeded or not.
| self.stage.moveAbs(self._original_stage_pos) | |
| try: | |
| self.stage.moveAbs(self._original_stage_pos) | |
| except Exception as e: | |
| logging.exception("Failed to restore stage to original position: %s", e) | |
| dlg = wx.MessageDialog( | |
| self.main_app.main_frame, | |
| f"Acquisition completed, but failed to restore stage position:\n{e}", | |
| "Stage Z-Stack acquisition warning", | |
| wx.OK | wx.ICON_WARNING | |
| ) | |
| dlg.ShowModal() | |
| dlg.Destroy() |
| self._gauge_future_conn = ProgressiveFutureConnector( | ||
| future=self._acq_future, | ||
| bar=self.tab_panel.gauge_cryosecom_acq, | ||
| label=self.tab_panel.txt_cryosecom_left_time, | ||
| full=False, | ||
| ) |
There was a problem hiding this comment.
The ProgressiveFutureConnector created at line 199 is never explicitly disconnected or cleaned up. While the connector might clean itself up when the future completes, it's better to explicitly clean up in _on_acquisition_done. Consider storing the connector and calling an appropriate cleanup method if available, or at minimum setting self._gauge_future_conn = None after acquisition completes.
| self.addMenu("Acquisition/Stage Z-Stack...", self.start) | ||
| logging.info("Stage Z-Stack plugin loaded") | ||
|
|
||
| def start(self): |
There was a problem hiding this comment.
Missing type hints and docstring for the start method. According to the custom coding guidelines, all functions should have type hints for parameters and return types, as well as docstrings following the reStructuredText style guide.
plugins/zstack_stage.py
Outdated
| # Hack: we temporarily change .focuser of the streams. | ||
| # This is a little dangerous, as if something goes wrong, the streams | ||
| # stay as-is. The better way would be to duplicate the streams, but | ||
| # that's not so simple with the different types of streams, and copying | ||
| # the values of the VAs. | ||
| if isinstance(s, (FluoStream, BrightfieldStream)): | ||
| self._focuser = s._focuser | ||
| s._focuser = self.stage |
There was a problem hiding this comment.
The hack of temporarily changing s._focuser is brittle and could cause issues if an exception occurs between setting and restoring. While the comment acknowledges this is a "hack" and mentions that duplication would be better, this approach leaves streams in an inconsistent state if errors occur. Consider using a try-finally block in _acquire() to ensure _focuser is always restored, or implement proper stream duplication as suggested in the comment.
| tab.streambar_controller.pauseStreams() | ||
|
|
||
| # Store original stage position | ||
| self._original_stage_pos = self.stage.position.value |
There was a problem hiding this comment.
The stage position is stored at line 112 after pausing streams. However, if the user had already moved the stage between opening the plugin menu and the actual acquisition start, this might not be the desired restoration position. Consider whether the original position should be the position when the plugin was initialized or when start() is called. The current approach seems reasonable but should be clearly documented.
z-stack is normally acquired by moving the focus (lens). In some experiments, it's handy to move the stage (in Z) instead. This, very basic, plugin adds a menu option "Acquisition/Stage Z-Stack..." on the METEOR to run the current acquisition with z-stack options, using the stage.
pieleric
force-pushed
the
feat-plugin-to-acquire-a-z-stack-by-moving-the-stage
branch
from
beaa623 to
9230f36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugins/zstack_stage.py (3)
192-197:_gauge_future_connnot cleared after acquisition completes.
self._acq_futureis set toNone(line 196) butself._gauge_future_connis not. The connector retains references to the completed future and UI widgets until the next acquisition.♻️ Proposed fix
self.main_data.is_acquiring.value = False self._acq_future = None + self._gauge_future_conn = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/zstack_stage.py` around lines 192 - 197, _on_acquisition_done currently clears self._acq_future and flips main_data.is_acquiring but leaves self._gauge_future_conn attached, retaining references; update _on_acquisition_done to safely disconnect and clear self._gauge_future_conn (e.g., if self._gauge_future_conn: attempt to disconnect/unsubscribe the connection and then set self._gauge_future_conn = None), ensuring the disconnect is guarded so it won't raise if already disconnected.
117-117: Redundant exception argument in all fourlogging.exceptioncalls (Ruff TRY401).
logging.exceptionautomatically appends the current exception's type, message, and traceback. The trailing%s, eis unnecessary and adds visual noise.♻️ Proposed fix (applies to all four sites)
- logging.exception("Failed to start stage z-stack acquisition: %s", e) + logging.exception("Failed to start stage z-stack acquisition")- logging.exception("Failed to generate z-levels: %s", e) + logging.exception("Failed to generate z-levels")- logging.exception("Stage Z-Stack acquisition failed: %s", e) + logging.exception("Stage Z-Stack acquisition failed")- logging.exception("Failed to save data: %s", e) + logging.exception("Failed to save data")Also applies to: 145-145, 230-230, 257-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/zstack_stage.py` at line 117, Remove the redundant exception argument from the logging.exception calls in plugins/zstack_stage.py: wherever you see logging.exception(..., "%s", e) or logging.exception("...: %s", e) (the four sites that currently pass the caught exception as a formatting argument), drop the "%s" placeholder and the trailing ", e" so the call is simply logging.exception("Descriptive message"); logging.exception will automatically include the exception type, message and traceback. Ensure each message remains descriptive (e.g., "Failed to start stage z-stack acquisition") and update all four occurrences.
41-47: Missing type hints and class docstring (coding guideline violation).Several methods violate the project's type-hint and docstring requirements:
Location Issue class StageZStackPluginMissing class-level docstring __init__(self, microscope, main_app)Both parameters and return type unannotated; no __init__docstringdef start(self)Missing -> Nonedef _acquire(self)Missing -> Nonedef _on_acquisition_done(self, future)futureunannotated; missing-> Nonedef _save_data(self, data: List)Listhas no type argument — useList[model.DataArray](orlist[model.DataArray]for Python 3.10+)✏️ Proposed additions
+class StageZStackPlugin(Plugin): + """Plugin for acquiring a z-stack by moving the microscope stage in Z. + + Provides a menu entry that runs the current Localization-tab acquisition + while stepping the stage Z-axis, instead of the focus actuator. + """ name = "Stage Z Stack"- def __init__(self, microscope, main_app): + def __init__(self, microscope: model.MicroscopeModel, main_app) -> None: + """ + :param microscope: the microscope component, or None if not available. + :param main_app: the main application GUI object. + """- def start(self): + def start(self) -> None:- def _acquire(self): + def _acquire(self) -> None:- def _on_acquisition_done(self, future): + def _on_acquisition_done(self, future: "ProgressiveFuture") -> None:- def _save_data(self, data: List) -> None: + def _save_data(self, data: List[model.DataArray]) -> None:As per coding guidelines: "Always use type hints for function parameters and return types in Python code" and "Include docstrings for all functions and classes, following the reStructuredText style guide".
Also applies to: 79-79, 122-122, 193-193, 240-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/zstack_stage.py` around lines 41 - 47, Add a class-level docstring to StageZStackPlugin and add reStructuredText docstrings to __init__, start, _acquire, _on_acquisition_done and _save_data; annotate __init__(self, microscope, main_app) with appropriate types (e.g., microscope: MicroscopeType, main_app: MainAppType) and -> None; annotate start(self) -> None and _acquire(self) -> None; annotate _on_acquisition_done(self, future: concurrent.futures.Future) -> None (or asyncio.Future if used); change _save_data(self, data: List) to data: List[model.DataArray] or list[model.DataArray]; import typing names as needed and apply the same docstring/type-hint pattern to the other listed methods/locations (79, 122, 193, 240) to satisfy the project guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/zstack_stage.py`:
- Around line 79-93: start() lacks a reentrancy guard and can overwrite
_original_stage_pos and _acq_future when called while an acquisition is running;
before doing any setup, check self.main_data.is_acquiring.value and if True show
the same MessageDialog (or a brief warning) and return to prevent re-entry.
Ensure the guard is placed at the top of start() (before saving
_original_stage_pos or creating _acq_future), and use the existing dialog
pattern so concurrent calls do not replace the in-flight acquisition's state or
callback.
---
Duplicate comments:
In `@plugins/zstack_stage.py`:
- Around line 163-170: The code overwrites stream._focuser in _acquire() without
saving originals and later _on_acquisition_done() restores focuser
indiscriminately from self.main_data.focuser (and reads the current
acquisitionStreams.value which may differ), so add a per-stream backup dict
(e.g., self._original_focusers = {}) keyed by stream object in __init__, in
_acquire() when you set s._focuser = self.stage save the previous focuser into
that dict for only the streams you modify (apply the same change for the similar
loop at the other swap site around lines 199–202), and in _on_acquisition_done()
iterate the saved dict to restore each stream's focuser to its original value
(do not re-read acquisitionStreams.value), then clear the dict after restoring.
- Around line 114-120: The except handler in start() only restores the stage
position, leaving is_acquiring True and the FluoStream/BrightfieldStream
focusers swapped (set to self.stage); update the error path to also reset
self.is_acquiring = False and restore the original focusers for each stream
(capture the previous focuser references before swapping in _acquire() or in
start(), e.g., save old_fluo_focuser/old_bf_focuser) so they are put back on
exception, and ensure any other acquisition state cleaned up (e.g., release or
rollback via a new helper like _release_acquisition) so acqmng.acquireZStack
failures (and other exceptions in _acquire()) do not leave the GUI locked in
acquiring state.
- Line 38: The code imports create_filename but builds output paths with a
fragile f-string using config.last_path, basename and config.last_extension;
replace that hand-rolled f-string with a call to create_filename so path
joining, filename-pattern respect, and collision avoidance are handled
correctly. Locate the usage that constructs
f"{config.last_path}/{basename}{config.last_extension}" and instead call
create_filename providing the directory (config.last_path), base name
(basename), extension (config.last_extension) and the user-configured filename
pattern/option from the GUI (e.g., config.filename_pattern or equivalent),
ensuring create_filename's collision-avoidance behavior is used; remove the
unused manual string assembly.
- Around line 256-257: The except Exception block that currently only calls
logging.exception("Failed to save data: %s", e) must also surface the failure to
the user: after logging the exception in the save routine (the except Exception
as e handler) call the application’s user-facing error dialog API (e.g.,
QMessageBox.critical or your existing show_error_dialog/notify_error helper)
with a clear message like "Failed to save data" plus the exception message, and
ensure the handler either returns a failure status or re-raises/propagates the
error so the UI can stop as appropriate; update the except block around the save
operation to both log and display the error to the user.
---
Nitpick comments:
In `@plugins/zstack_stage.py`:
- Around line 192-197: _on_acquisition_done currently clears self._acq_future
and flips main_data.is_acquiring but leaves self._gauge_future_conn attached,
retaining references; update _on_acquisition_done to safely disconnect and clear
self._gauge_future_conn (e.g., if self._gauge_future_conn: attempt to
disconnect/unsubscribe the connection and then set self._gauge_future_conn =
None), ensuring the disconnect is guarded so it won't raise if already
disconnected.
- Line 117: Remove the redundant exception argument from the logging.exception
calls in plugins/zstack_stage.py: wherever you see logging.exception(..., "%s",
e) or logging.exception("...: %s", e) (the four sites that currently pass the
caught exception as a formatting argument), drop the "%s" placeholder and the
trailing ", e" so the call is simply logging.exception("Descriptive message");
logging.exception will automatically include the exception type, message and
traceback. Ensure each message remains descriptive (e.g., "Failed to start stage
z-stack acquisition") and update all four occurrences.
- Around line 41-47: Add a class-level docstring to StageZStackPlugin and add
reStructuredText docstrings to __init__, start, _acquire, _on_acquisition_done
and _save_data; annotate __init__(self, microscope, main_app) with appropriate
types (e.g., microscope: MicroscopeType, main_app: MainAppType) and -> None;
annotate start(self) -> None and _acquire(self) -> None; annotate
_on_acquisition_done(self, future: concurrent.futures.Future) -> None (or
asyncio.Future if used); change _save_data(self, data: List) to data:
List[model.DataArray] or list[model.DataArray]; import typing names as needed
and apply the same docstring/type-hint pattern to the other listed
methods/locations (79, 122, 193, 240) to satisfy the project guidelines.
| def start(self): | ||
| """Called when the menu entry is selected""" | ||
| tab = self.main_data.tab.value | ||
|
|
||
| # Check that we're on the correct tab | ||
| if tab.name != "cryosecom-localization": | ||
| box = wx.MessageDialog( | ||
| self.main_app.main_frame, | ||
| "Stage Z-Stack acquisition must be done from the LOCALIZATION tab.", | ||
| "Stage Z-Stack acquisition not possible", | ||
| wx.OK | wx.ICON_STOP | ||
| ) | ||
| box.ShowModal() | ||
| box.Destroy() | ||
| return |
There was a problem hiding this comment.
No reentrancy guard — concurrent invocations corrupt _original_stage_pos and _acq_future.
start() does not check self.main_data.is_acquiring.value. If the menu entry remains reachable while an acquisition is running, a second call overwrites _original_stage_pos and replaces _acq_future, causing the in-flight acquisition's callback to restore the wrong position.
🛡️ Proposed fix
def start(self) -> None:
"""Called when the menu entry is selected"""
+ if self.main_data.is_acquiring.value:
+ logging.warning("Stage Z-Stack: acquisition already in progress, ignoring request")
+ return
+
tab = self.main_data.tab.value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/zstack_stage.py` around lines 79 - 93, start() lacks a reentrancy
guard and can overwrite _original_stage_pos and _acq_future when called while an
acquisition is running; before doing any setup, check
self.main_data.is_acquiring.value and if True show the same MessageDialog (or a
brief warning) and return to prevent re-entry. Ensure the guard is placed at the
top of start() (before saving _original_stage_pos or creating _acq_future), and
use the existing dialog pattern so concurrent calls do not replace the in-flight
acquisition's state or callback.
|
@pieleric I don't see a jira task related to this PR, why did you implement this and why did you implement it as a plugin and not an actual feature? |
2 participants
z-stack is normally acquired by moving the focus (lens). In some
experiments, it's handy to move the stage (in Z) instead.
This, very basic, plugin adds a menu option "Acquisition/Stage Z-Stack..."
on the METEOR to run the current acquisition with z-stack options, using
the stage.