Skip to content

[feat] save drift corrector images #3487

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.

Sign up for GitHub

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

Open
nandishjpatel wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[feat] save drift corrector images #3487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7401f14
[feat] added save_images
yxkdejong May 4, 2026
0250ddb
[update] address pr comments
nandishjpatel Jul 9, 2026
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
65 changes: 65 additions & 0 deletions plugins/sparc_save_drift_corrector_images.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# -*- coding: utf-8 -*-
"""
Created on 11 June 2026

@author: Nandish Patel

Copyright © 2026 Nandish Patel, Delmic

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 wx

from odemis.gui.model import TabName
from odemis.gui.plugin import Plugin


class SaveDriftCorrectorImgPlugin(Plugin):
name = "Save drift corrector images"
Comment on lines +30 to +31

@coderabbitai coderabbitai Bot Jun 11, 2026

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show relevant portions of the file around the referenced lines
FILE="plugins/sparc_save_drift_corrector_images.py"
ls -la "$FILE" || true
nl -ba "$FILE" | sed -n '1,120p'
nl -ba "$FILE" | sed -n '120,240p'
nl -ba "$FILE" | sed -n '240,360p'

# Quick structural search for type hints and docstrings in the plugin file
rg -n "class SaveDriftCorrectorImgPlugin|def " "$FILE" || true
rg -n "->|: " "$FILE" || true
rg -n '"""|\\\'\\\'\\\'' "$FILE" || true

Repository: delmic/odemis

Length of output: 212

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="plugins/sparc_save_drift_corrector_images.py"
echo "== File =="; ls -la "$FILE"

echo "== Lines 1-120 (for context) =="
awk 'NR<=120{printf "%6d:%s\n", NR, $0}' "$FILE"

echo "== Focused ranges =="
for start in 25 30 34 36 48 54; do
  end=$((start+15))
  echo "--- Lines ${start}-${end} ---"
  awk -v s="$start" -v e="$end" 'NR>=s && NR<=e{printf "%6d:%s\n", NR, $0}' "$FILE"
done

echo "== Search: class/defs ==" 
rg -n "class |def " "$FILE" || true

echo "== Search: type hints (-> or annotated params) =="
rg -n "->|def .*:.*|def .*: .*=" "$FILE" || true

echo "== Search: docstrings ==" 
rg -n '"""|'\'''\''' "$FILE" || true

Repository: delmic/odemis

Length of output: 8063

Add type hints and rST docstrings to the new SaveDriftCorrectorImgPlugin

File: plugins/sparc_save_drift_corrector_images.py

  • SaveDriftCorrectorImgPlugin (lines 30-31) is missing a class docstring.
  • __init__(self, microscope, main_app) (lines 36-37) is missing type hints for parameters/return type and has no docstring.
  • _save_drift_corrector_images(self, menu_item) (lines 54-62) is missing type hints for menu_item and the return type (it already has a docstring).
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plugins/sparc_save_drift_corrector_images.py` around lines 30 - 31, The
SaveDriftCorrectorImgPlugin class lacks a class rST docstring and its methods
lack type hints/docstrings; add an rST-style class docstring to
SaveDriftCorrectorImgPlugin describing its purpose, then update __init(self,
microscope, main_app) to include proper type hints for microscope and main_app
(and -> None) and add a short rST docstring describing parameters, types, and
return; also add type hints to _save_drift_corrector_images(self, menu_item)
(typing for menu_item and -> None) and ensure its existing docstring is
converted/expanded to rST param/return sections. Reference the class name
SaveDriftCorrectorImgPlugin and the methods __init__ and
_save_drift_corrector_images when making these changes.

Source: Coding guidelines

__version__ = "1.0"
__author__ = "Nandish Patel"
__license__ = "GPLv2"

def __init__(self, microscope, main_app):
super().__init__(microscope, main_app)

# It only makes sense if the SPARC acquisition tab is present
try:
sparc_acq_tab = main_app.main_data.getTabByName(TabName.SPARC_ACQUI)
except LookupError:
logging.debug(
"Not loading save drift corrector images tool since SPARC acquisition tab is not present."
)
return

self._sparc_acq_tab = sparc_acq_tab
self.addMenu("Help/Development/Save drift corrector images",
self._save_drift_corrector_images,
item_kind=wx.ITEM_CHECK,
pass_menu_item=True)

def _on_sparc_acq_ctrl_filename(self, filename):
self._sparc_acq_tab.tab_data_model.driftCorrector.log_path = filename

def _save_drift_corrector_images(self, menu_item):

@pieleric pieleric Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add type annotation to all methods.

"""Menu callback for: Help/Development/Save drift corrector images"""
checked = menu_item.IsChecked()
if checked:
self._sparc_acq_tab._acquisition_controller.filename.subscribe(self._on_sparc_acq_ctrl_filename, init=True)
logging.debug("Save drift corrector images checked, will acquire drift corrector images")
else:
self._sparc_acq_tab._acquisition_controller.filename.unsubscribe(self._on_sparc_acq_ctrl_filename)
Comment on lines +63 to +64

@coderabbitai coderabbitai Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Clear log_path when unchecking the menu item.

Unsubscribing the callback stops future filename updates but leaves driftCorrector.log_path set to the last value. Any acquisition started after unchecking will still save drift images, contradicting the user's intent. Set log_path to None in the unsubscribe branch.

🔧 Proposed fix 
         else:
             self._sparc_acq_tab._acquisition_controller.filename.unsubscribe(self._on_sparc_acq_ctrl_filename)
+            self._sparc_acq_tab.tab_data_model.driftCorrector.log_path = None
             logging.debug("Save drift corrector images unchecked, will not acquire drift corrector images")
📝 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.

Suggested change
else:
self._sparc_acq_tab._acquisition_controller.filename.unsubscribe(self._on_sparc_acq_ctrl_filename)
else:
self._sparc_acq_tab._acquisition_controller.filename.unsubscribe(self._on_sparc_acq_ctrl_filename)
self._sparc_acq_tab.tab_data_model.driftCorrector.log_path = None
logging.debug("Save drift corrector images unchecked, will not acquire drift corrector images")
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plugins/sparc_save_drift_corrector_images.py` around lines 63 - 64, The
uncheck branch in the menu toggle logic only unsubscribes
`_on_sparc_acq_ctrl_filename` from the acquisition controller, but
`driftCorrector.log_path` remains set to the previous filename. Update the same
`else` path in the menu handler to also clear `log_path` on the drift corrector
so future acquisitions do not keep saving drift images after the menu item is
turned off.

logging.debug("Save drift corrector images unchecked, will not acquire drift corrector images")
21 changes: 18 additions & 3 deletions src/odemis/acq/drift/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,13 @@
import itertools
import logging
import math
import os
import threading

import numpy
import cv2
import numpy

from odemis import model
from odemis import dataio, model
from odemis.acq.align.shift import MeasureShift

MIN_RESOLUTION = (20, 20) # sometimes 8x8 works, but it's not reliable enough
Expand All @@ -46,7 +47,7 @@ class AnchoredEstimator(object):
to measure the drift.
"""

def __init__(self, scanner, detector, region, dwell_time, max_pixels=MAX_PIXELS, follow_drift=True):
def __init__(self, scanner, detector, region, dwell_time, max_pixels=MAX_PIXELS, follow_drift=True, log_path=None):
"""
scanner (Emitter)
detector (Detector)
Expand All @@ -58,11 +59,14 @@ def __init__(self, scanner, detector, region, dwell_time, max_pixels=MAX_PIXELS,
follow_drift (bool): If True, the anchor region position is adjusted based on the drift measured. It is useful
when drift compensation is done by adjusting the scanner settings. If False, the anchor region is fixed.
It is useful when drift compensation is based on beam shift or stage movement.
log_path (Optional[str]): directory and filename pattern to save drift corrector images for debugging
"""
self._emitter = scanner
self._semd = detector
self._dwell_time = dwell_time
self._follow_drift = follow_drift
self._log_path = log_path
self._image_counter = 0

# Latest drift vector from the previous acquisition
self.drift = (0, 0) # in sem px
Expand Down Expand Up @@ -143,6 +147,17 @@ def acquire(self):
if data.shape[::-1] != self._res:
logging.warning("Shape of data is %s instead of %s", data.shape[::-1], self._res)

# Save all the drift region scans for offline autocorrelation purposes
if self._log_path is not None:
filename = os.path.basename(self._log_path)
if not filename:
raise ValueError("Filename is not found on log path.")
exporter = dataio.find_fittest_converter(filename)
path, base = os.path.split(filename)
Comment on lines +152 to +156

@tepals tepals Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

filename is the basename of log_path (see line 152), when you split that (line 156) the path variable is just going to be an empty string.

Suggested change
filename = os.path.basename(self._log_path)
if not filename:
raise ValueError("Filename is not found on log path.")
exporter = dataio.find_fittest_converter(filename)
path, base = os.path.split(filename)
path, filename = os.path.split(self._log_path)
if not filename:
raise ValueError("Filename is not found on log path.")
exporter = dataio.find_fittest_converter(filename)

@tepals tepals Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you do this, you also need to change base to filename on line 157

fn = f"drift_{self._image_counter:05d}_" + base

@pieleric pieleric Jul 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you add drift at the beginning of the filename, all the "drift_xxx" file images will be next to each other in the directory. I think it'd be nicer to use "{filename}_drift_{self._image_counter:05d}_". You can use dataio.splitext() for finding the place just before the extension.

exporter.export(data, os.path.join(path, fn))
Comment on lines +152 to +158

@coderabbitai coderabbitai Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Directory path is lost — drift images save to the current working directory instead of the intended directory.

os.path.basename(self._log_path) strips the directory, producing just the filename. Then os.path.split(filename) on that basename returns path="", so os.path.join("", fn) resolves to just fn. The exported image lands in the CWD, not the directory from log_path.

Example: log_path = "/data/acq/scan.tif" → filename = "scan.tif" → path = "" → file saved as "drift_00000_scan.tif" in CWD.

Per coding guidelines, prefer pathlib.Path for file path construction. This also fixes the bug:

🐛 Proposed fix using pathlib.Path 
-            if self._log_path is not None:
-                filename = os.path.basename(self._log_path)
-                if not filename:
-                    raise ValueError("Filename is not found on log path.")
-                exporter = dataio.find_fittest_converter(filename)
-                path, base = os.path.split(filename)
-                fn  = f"drift_{self._image_counter:05d}_" + base
-                exporter.export(data, os.path.join(path, fn))
-                self._image_counter += 1
+            if self._log_path is not None:
+                log_path = Path(self._log_path)
+                if not log_path.name:
+                    raise ValueError("Filename is not found on log path.")
+                exporter = dataio.find_fittest_converter(log_path.name)
+                fn = f"drift_{self._image_counter:05d}_{log_path.name}"
+                exporter.export(data, str(log_path.parent / fn))
+                self._image_counter += 1
📝 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.

Suggested change
filename = os.path.basename(self._log_path)
if not filename:
raise ValueError("Filename is not found on log path.")
exporter = dataio.find_fittest_converter(filename)
path, base = os.path.split(filename)
fn = f"drift_{self._image_counter:05d}_" + base
exporter.export(data, os.path.join(path, fn))
if self._log_path is not None:
log_path = Path(self._log_path)
if not log_path.name:
raise ValueError("Filename is not found on log path.")
exporter = dataio.find_fittest_converter(log_path.name)
fn = f"drift_{self._image_counter:05d}_{log_path.name}"
exporter.export(data, str(log_path.parent / fn))
self._image_counter += 1
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/odemis/acq/drift/__init__.py` around lines 152 - 158, The drift export
path logic in the save block loses the directory by using basename and then
splitting that basename, so `exporter.export` writes to the current working
directory instead of the `log_path` directory. Update the path handling around
`self._log_path`, `filename`, and `exporter.export` to preserve the original
directory when constructing the output filename, preferably by using
`pathlib.Path` to derive the target directory and final file path. Keep the
existing filename prefix logic (`drift_{self._image_counter:05d}_...`) but
ensure the export destination remains anchored to the directory from
`self._log_path`.

Source: Coding guidelines

self._image_counter += 1
Comment on lines +150 to +159

@coderabbitai coderabbitai Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Wrap image export in error handling to prevent debugging saves from crashing the acquisition.

Both the ValueError for an empty filename and any exporter.export failure (disk full, permission denied, invalid path) will propagate through the try block and disrupt the ongoing acquisition. Since this image saving is for debugging purposes, it should degrade gracefully.

🛡️ Proposed fix 
             # Save all the drift region scans for offline autocorrelation purposes
             if self._log_path is not None:
-                filename = os.path.basename(self._log_path)
-                if not filename:
-                    raise ValueError("Filename is not found on log path.")
-                exporter = dataio.find_fittest_converter(filename)
-                path, base = os.path.split(filename)
-                fn  = f"drift_{self._image_counter:05d}_" + base
-                exporter.export(data, os.path.join(path, fn))
-                self._image_counter += 1
+                try:
+                    log_path = Path(self._log_path)
+                    if not log_path.name:
+                        logging.warning("Cannot save drift corrector image: filename is empty in log path '%s'", self._log_path)
+                    else:
+                        exporter = dataio.find_fittest_converter(log_path.name)
+                        fn = f"drift_{self._image_counter:05d}_{log_path.name}"
+                        exporter.export(data, str(log_path.parent / fn))
+                        self._image_counter += 1
+                except Exception:
+                    logging.exception("Failed to save drift corrector image to '%s'", self._log_path)
📝 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.

Suggested change
# Save all the drift region scans for offline autocorrelation purposes
if self._log_path is not None:
filename = os.path.basename(self._log_path)
if not filename:
raise ValueError("Filename is not found on log path.")
exporter = dataio.find_fittest_converter(filename)
path, base = os.path.split(filename)
fn = f"drift_{self._image_counter:05d}_" + base
exporter.export(data, os.path.join(path, fn))
self._image_counter += 1
# Save all the drift region scans for offline autocorrelation purposes
if self._log_path is not None:
try:
log_path = Path(self._log_path)
if not log_path.name:
logging.warning("Cannot save drift corrector image: filename is empty in log path '%s'", self._log_path)
else:
exporter = dataio.find_fittest_converter(log_path.name)
fn = f"drift_{self._image_counter:05d}_{log_path.name}"
exporter.export(data, str(log_path.parent / fn))
self._image_counter += 1
except Exception:
logging.exception("Failed to save drift corrector image to '%s'", self._log_path)
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/odemis/acq/drift/__init__.py` around lines 150 - 159, The drift
debug-save path in the acquisition flow should not let filename validation or
`exporter.export` failures interrupt the ongoing scan. Update the image export
block in `drift/__init__.py` (around the logic using `self._log_path`,
`dataio.find_fittest_converter`, and `exporter.export`) to catch errors from
both the empty filename case and any export I/O failure, log or ignore them as
debug-only issues, and continue the acquisition without raising.


# TODO: allow to record just every Nth image, and separately record the
# drift after every measurement
# In the mean time, we only save the 1st, 2nd and last two images
Expand Down
4 changes: 3 additions & 1 deletion src/odemis/acq/leech.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ def __init__(self, scanner, detector):
self._detector = detector
self._dc_estimator = None
self._period_acq = None # number of acq left until next drift correction is performed
self.log_path = None

# roi: the anchor region, it must be set to something different from
# UNDEFINED_ROI to run.
Expand Down Expand Up @@ -280,7 +281,8 @@ def series_start(self):
self._dc_estimator = drift.AnchoredEstimator(self._scanner,
self._detector,
self.roi.value,
self.dwellTime.value)
self.dwellTime.value,
log_path=self.log_path)

# First acquisition of anchor area
self._dc_estimator.acquire()
Expand Down
Loading