Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
257 changes: 257 additions & 0 deletions plugins/zstack_stage.py
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
Copy link

Copilot AI Feb 20, 2026

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.

Suggested change
from odemis.util.filename import create_filename

Copilot uses AI. Check for mistakes.


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
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing type hints for the init method parameters. According to the custom coding guidelines, Python code should always use type hints for function parameters and return types. The parameters 'microscope' and 'main_app' should have type annotations.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +47 to +48
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing docstring for the init method. According to the custom coding guidelines, all functions and classes should include docstrings following the reStructuredText style guide. The init method should document its parameters and purpose.

Copilot generated this review using guidance from repository custom instructions.

# 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):
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot generated this review using guidance from repository custom instructions.
"""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.",
Comment thread
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
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


# 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
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

# 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
Copy link

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
# 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 uses AI. Check for mistakes.

def _acquire(self):
"""Start the z-stack acquisition"""
Comment on lines +122 to +123
Copy link

Copilot AI Feb 20, 2026

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.

Copilot generated this review using guidance from repository custom instructions.
# Get acquisition streams
acq_streams = self.tab_data.acquisitionStreams.value

Comment on lines +122 to +126
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.stage

In _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.

logging.debug("Acquisition streams: %s", acq_streams)

# Generate z-levels based on current stage position and z-stack parameters
try:
zmin = self.tab_data.zMin.value
zmax = self.tab_data.zMax.value
zstep = self.tab_data.zStep.value

logging.info(
"Generating z-levels with zmin=%g, zmax=%g, zstep=%g",
zmin, zmax, zstep
)

levels = generate_zlevels(self.stage, (zmin, zmax), zstep)

logging.info("Generated %d z-levels: %s", len(levels), levels)

except (ValueError, IndexError, ZeroDivisionError, KeyError):
logging.exception("Failed to generate z-levels")
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")
return
Comment on lines +154 to +156
Copy link

Copilot AI Feb 20, 2026

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.

Copilot uses AI. Check for mistakes.

logging.info("Applying z-stack to %d streams", len(zlevels))

# Start the acquisition
self.main_data.is_acquiring.value = True

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
Comment thread
pieleric marked this conversation as resolved.
# that's not so simple with the different types of streams, and copying
# the values of the VAs.
if isinstance(s, (FluoStream, BrightfieldStream)):
s._focuser = self.stage

self._acq_future = acqmng.acquireZStack(
acq_streams,
zlevels,
self.main_data.settings_obs
)
Comment on lines +161 to +176
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


# link the acquisition gauge to the acquisition future
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,
)
Comment on lines +179 to +184
Copy link

Copilot AI Feb 20, 2026

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 uses AI. Check for mistakes.

logging.info("Stage Z-Stack acquisition started")

# Show progress in the status bar or create a simple progress dialog
# For now, we'll just add a callback to handle completion
self._acq_future.add_done_callback(self._on_acquisition_done)

@call_in_wx_main
def _on_acquisition_done(self, future):
"""Called when the acquisition is complete"""
Comment on lines +192 to +194
Copy link

Copilot AI Feb 20, 2026

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 generated this review using guidance from repository custom instructions.
self.main_data.is_acquiring.value = False
self._acq_future = None

# Hack: restore the focuser
acq_streams = self.tab_data.acquisitionStreams.value
for s in acq_streams:
if isinstance(s, (FluoStream, BrightfieldStream)):
s._focuser = self.main_data.focuser
Comment thread
pieleric marked this conversation as resolved.

# Restore original stage position
logging.debug("Restoring stage to original position: %s", self._original_stage_pos)
self.stage.moveAbs(self._original_stage_pos)
Copy link

Copilot AI Feb 20, 2026

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.

Suggested change
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 uses AI. Check for mistakes.
# No wait, as it's fast (~1s), and in the worst case the user will notice it and wait

# Get the acquisition results
try:
data, exp = future.result()

if exp:
Comment on lines +211 to +213
Copy link

Copilot AI Feb 20, 2026

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 uses AI. Check for mistakes.
logging.warning("Acquisition completed with exceptions: %s", exp)
dlg = wx.MessageDialog(
self.main_app.main_frame,
f"Acquisition only partially completed:\n{exp}",
"Stage Z-Stack acquisition completed",
wx.OK | wx.ICON_WARNING
)
dlg.ShowModal()
dlg.Destroy()

# Save the data automatically
self._save_data(data)

logging.info("Stage Z-Stack acquisition completed successfully")

except Exception as e:
logging.exception("Stage Z-Stack acquisition failed")
dlg = wx.MessageDialog(
self.main_app.main_frame,
f"Acquisition failed:\n{e}",
Copy link

Copilot AI Feb 20, 2026

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.

Suggested change
f"Acquisition failed:\n{e}",
"Acquisition failed due to an unexpected error.\nPlease check the log for more details.",

Copilot uses AI. Check for mistakes.
"Stage Z-Stack acquisition failed",
wx.OK | wx.ICON_ERROR
)
dlg.ShowModal()
dlg.Destroy()

def _save_data(self, data: List) -> None:
Copy link

Copilot AI Feb 20, 2026

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 generated this review using guidance from repository custom instructions.
"""Save the acquired data to a file"""
Comment on lines +240 to +241
Copy link

Copilot AI Feb 20, 2026

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 generated this review using guidance from repository custom instructions.
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}"
Comment on lines +243 to +246
Copy link

Copilot AI Feb 20, 2026

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.

Suggested change
# 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)

Copilot uses AI. Check for mistakes.

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:
logging.exception("Failed to save data")
Loading