-
Notifications
You must be signed in to change notification settings - Fork 41
[MSD-376][feat] Plugin to acquire a z-stack by moving the stage #3374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,257 @@ | ||||||||||||||||||||||||||||
| # -*- coding: utf-8 -*- | ||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||
| Created on 12 Feb 2026 | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @author: Éric Piel | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Plugin for the METEOR (and MIMAS). | ||||||||||||||||||||||||||||
| Acquire a z-stack using the sample stage z axis instead of the focus actuator. | ||||||||||||||||||||||||||||
| Uses the same z-stack parameters as the Localization tab. | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| This file is part of Odemis. | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Odemis is free software: you can redistribute it and/or modify it under the terms of the GNU | ||||||||||||||||||||||||||||
| General Public License version 2 as published by the Free Software Foundation. | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Odemis is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even | ||||||||||||||||||||||||||||
| the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General | ||||||||||||||||||||||||||||
| Public License for more details. | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| You should have received a copy of the GNU General Public License along with Odemis. If not, | ||||||||||||||||||||||||||||
| see http://www.gnu.org/licenses/. | ||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||||
| import time | ||||||||||||||||||||||||||||
| from typing import Dict, List | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| import wx | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| from odemis import dataio, model | ||||||||||||||||||||||||||||
| from odemis.acq import acqmng | ||||||||||||||||||||||||||||
| from odemis.acq.stream import BrightfieldStream, FluoStream, Stream | ||||||||||||||||||||||||||||
| from odemis.gui.conf import get_acqui_conf | ||||||||||||||||||||||||||||
| from odemis.gui.plugin import Plugin | ||||||||||||||||||||||||||||
| from odemis.gui.util import call_in_wx_main | ||||||||||||||||||||||||||||
| from odemis.gui.util.widgets import ProgressiveFutureConnector | ||||||||||||||||||||||||||||
| from odemis.util.comp import generate_zlevels | ||||||||||||||||||||||||||||
| from odemis.util.filename import create_filename | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| class StageZStackPlugin(Plugin): | ||||||||||||||||||||||||||||
| name = "Stage Z Stack" | ||||||||||||||||||||||||||||
| __version__ = "1.0" | ||||||||||||||||||||||||||||
| __author__ = "Éric Piel" | ||||||||||||||||||||||||||||
| __license__ = "GPLv2" | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def __init__(self, microscope, main_app): | ||||||||||||||||||||||||||||
| super().__init__(microscope, main_app) | ||||||||||||||||||||||||||||
|
Comment on lines
+47
to
+48
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # Can only be used with a microscope with a stage | ||||||||||||||||||||||||||||
| self.main_data = main_app.main_data | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if not microscope or self.main_data.stage is None: | ||||||||||||||||||||||||||||
| logging.info("Stage Z-Stack plugin not available: no stage found") | ||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # Check that the stage has a z-axis | ||||||||||||||||||||||||||||
| if 'z' not in self.main_data.stage.axes: | ||||||||||||||||||||||||||||
| logging.info("Stage Z-Stack plugin not available: stage has no z-axis") | ||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||
| tab = self.main_data.getTabByName("cryosecom-localization") | ||||||||||||||||||||||||||||
| self.tab_data = tab.tab_data_model | ||||||||||||||||||||||||||||
| self.tab_panel = tab.panel | ||||||||||||||||||||||||||||
| except LookupError: | ||||||||||||||||||||||||||||
| logging.info("Stage Z-Stack plugin not available: Localization tab not found") | ||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| self.stage = self.main_data.stage | ||||||||||||||||||||||||||||
| self._acq_future = None | ||||||||||||||||||||||||||||
| self._gauge_future_conn = None | ||||||||||||||||||||||||||||
| self._original_stage_pos = None | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # Add menu entry | ||||||||||||||||||||||||||||
| self.addMenu("Acquisition/Stage Z-Stack...", self.start) | ||||||||||||||||||||||||||||
| logging.info("Stage Z-Stack plugin loaded") | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| 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.", | ||||||||||||||||||||||||||||
|
pieleric marked this conversation as resolved.
|
||||||||||||||||||||||||||||
| "Stage Z-Stack acquisition not possible", | ||||||||||||||||||||||||||||
| wx.OK | wx.ICON_STOP | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| box.ShowModal() | ||||||||||||||||||||||||||||
| box.Destroy() | ||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||
|
Comment on lines
+79
to
+93
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No reentrancy guard — concurrent invocations corrupt
🛡️ 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 |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # Check if there are streams to acquire | ||||||||||||||||||||||||||||
| if not self.tab_data.acquisitionStreams.value: | ||||||||||||||||||||||||||||
| box = wx.MessageDialog( | ||||||||||||||||||||||||||||
| self.main_app.main_frame, | ||||||||||||||||||||||||||||
| "No streams available for acquisition. Please add streams first.", | ||||||||||||||||||||||||||||
| "Stage Z-Stack acquisition not possible", | ||||||||||||||||||||||||||||
| wx.OK | wx.ICON_STOP | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| box.ShowModal() | ||||||||||||||||||||||||||||
| box.Destroy() | ||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # Pause live streams | ||||||||||||||||||||||||||||
| tab.streambar_controller.pauseStreams() | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # Store original stage position | ||||||||||||||||||||||||||||
| self._original_stage_pos = self.stage.position.value | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # Start acquisition, which will continue in the background and call _on_acquisition_done when finished | ||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||
| self._acquire() | ||||||||||||||||||||||||||||
| except Exception: | ||||||||||||||||||||||||||||
| logging.exception("Failed to start stage z-stack acquisition") | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # Restore stage position | ||||||||||||||||||||||||||||
| self.stage.moveAbs(self._original_stage_pos) | ||||||||||||||||||||||||||||
|
Comment on lines
+119
to
+120
|
||||||||||||||||||||||||||||
| # 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, | |
| ) |
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing type hints and improved docstring for the _acquire method. According to the custom coding guidelines, all functions should have type hints for parameters and return types. The docstring should be expanded to describe what the method does in more detail, its return value, and any exceptions it might raise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If _acquire() returns early at line 185 due to no optical streams being found, the stage position is not restored to its original position. The restoration only happens in _on_acquisition_done (line 226), but that callback is only attached when the acquisition future is created (line 210). This means the stage could be left at a different position than when the plugin started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing type hints and incomplete docstring for the _on_acquisition_done method. According to the custom coding guidelines, all functions should have type hints for parameters and return types. The docstring should be expanded to describe the parameter 'future' and what the method does in detail.
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() |
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message at line 252 directly exposes the exception object. Consider providing a more user-friendly message while keeping technical details in the logs. The exception is already logged at line 249 with full details.
| f"Acquisition failed:\n{e}", | |
| "Acquisition failed due to an unexpected error.\nPlease check the log for more details.", |
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _save_data method is missing type hints for its parameter and return type. According to the custom coding guidelines, all functions should have type hints. While the List type is used in the function signature, the 'data' parameter should be properly typed, and the return type annotation '-> None' is present but should be documented in the docstring as well.
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _save_data method generates a filename without using the proper create_filename utility that was imported. The current approach (line 265) doesn't follow the codebase convention of using create_filename to ensure uniqueness and proper counter handling. The filename should be generated using create_filename with the path, pattern, and extension separately, similar to how it's done in other plugins like zstack.py and timelapse.py.
| # 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}" | |
| # Generate filename with timestamp using the common filename utility | |
| config = get_acqui_conf() | |
| basename = time.strftime("%Y%m%d-%H%M%S-stage-zstack") | |
| filename = create_filename(config.last_path, basename, config.last_extension) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import of create_filename from odemis.util.filename (line 38) is unused. The _save_data method generates filenames directly using time.strftime instead of using the imported create_filename function. Either remove the unused import or use create_filename to generate unique filenames following the codebase convention.