[MSD-262][feature] Pass correct reference image to fibsemOS #3304
Conversation
ilyushkin
commented
Dec 17, 2025
ilyushkin
commented
- Pass the same reference image from Odemis to fibsemOS that is used for placing milling markers in the Odemis FIBSEM tab.
- The reference image expected by fibsemOS should be correctly cropped to the size of the reduced area used for beam shift calculation and should contain correct metadata.
- Skip reference image alignment in Odemis, as it is instead performed by fibsemOS using the passed reference image.
📝 WalkthroughWalkthroughThe pull request refactors the FibSEM milling workflow to integrate CryoFeature objects as the primary source of reference images. The Sequence DiagramsequenceDiagram
participant Caller as Caller
participant Manager as FibsemOSMillingTaskManager
participant Feature as CryoFeature
participant Converter as from_odemis_image
participant Structures as fibsem.structures
participant Microscope as FibsemMicroscope
Caller->>Manager: async_run(tasks, feature, path)
Manager->>Feature: _get_reference_image(feature)
Feature-->>Manager: reference_image (DataArray)
Manager->>Converter: from_odemis_image(reference_image)
Converter-->>Manager: FibsemImage
Manager->>Structures: Set ref_img.metadata.image_settings.path
Structures-->>Manager: Updated FibsemImage
Manager->>Manager: _crop_to_reduced_area(stage.alignment.rect)
Manager->>Microscope: mill_stages([stage], ref_img)
Microscope-->>Manager: Milling complete
Manager-->>Caller: Future result
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/odemis/acq/milling/fibsemos.py (2)
211-230: Critical: Missingfeatureparameter in__init__signature.The docstring at line 217 documents a
featureparameter, and line 230 assignsself.feature = feature, butfeatureis not declared in the method signature. This will cause aNameErrorat runtime.Apply this diff to add the missing parameter:
def __init__(self, future: futures.Future, tasks: List[MillingTaskSettings], + feature: CryoFeature, path: Optional[str] = None): """ :param future: the future that will be executing the task :param tasks: The milling tasks to run (in order) :param feature: Cryo feature for milling :param path: The path to save the images (optional) """
314-326: Critical: Missingfeatureparameter inrun_milling_tasks_fibsemossignature.The docstring at line 319 documents a
featureparameter, but it's not in the function signature (lines 314-315), and it's not passed toFibsemOSMillingTaskManagerat line 326.Apply this diff to add the missing parameter:
def run_milling_tasks_fibsemos(tasks: List[MillingTaskSettings], + feature: CryoFeature, path: Optional[str] = None) -> futures.Future: """ Run multiple milling tasks in order via fibsemOS. :param tasks: List of milling tasks to be executed in order :param feature: Cryo feature for milling :param path: The path to save the images :return: ProgressiveFuture """ # Create a progressive future with running sub future future = model.ProgressiveFuture() # create milling task - millmng = FibsemOSMillingTaskManager(future, tasks, path) + millmng = FibsemOSMillingTaskManager(future, tasks, feature, path)
🧹 Nitpick comments (1)
src/odemis/acq/milling/fibsemos.py (1)
43-51: Several imported types appear unused.
FibsemImageMetadata,BeamType,ImageSettings, andMicroscopeStateare imported but not used in the current code. Consider removing them if they're not needed for type hints or future use.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/odemis/acq/feature.py(1 hunks)src/odemis/acq/milling/fibsemos.py(5 hunks)src/odemis/acq/milling/millmng.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/odemis/acq/milling/fibsemos.py (1)
src/odemis/acq/feature.py (1)
CryoFeature(144-240)
🪛 GitHub Actions: Linting
src/odemis/acq/feature.py
[error] 1-1: PNG metadata check detected forbidden metadata chunks in file during PNG metadata validation step (Contains forbidden metadata chunks).
src/odemis/acq/milling/millmng.py
[error] 1-1: PNG metadata check detected forbidden metadata chunks in file during PNG metadata validation step (Contains forbidden metadata chunks).
src/odemis/acq/milling/fibsemos.py
[error] 1-1: PNG metadata check detected forbidden metadata chunks in file during PNG metadata validation step (Contains forbidden metadata chunks).
🪛 Ruff (0.14.8)
src/odemis/acq/milling/fibsemos.py
230-230: Undefined name feature
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (1)
src/odemis/acq/feature.py (1)
346-346: LGTM!Simple typo fix in the docstring - "featuers" → "features".
ilyushkin
force-pushed
the
msd-262-pass-correct-reference-image-to-fibsemos
branch
from
2984f3c to
1b2a215
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/odemis/acq/milling/fibsemos.py`:
- Around line 268-279: from_odemis_image(self.feature.reference_image) can raise
if self.feature.reference_image is not hydrated; update the ref image loading
logic around where ref_img is created to first try
from_odemis_image(self.feature.reference_image), and if that returns None or
raises, attempt to load the saved image from self.feature.path (use the same
loader used elsewhere for persisted images) and assign it to ref_img, preserving
metadata.image_settings.path and reduced_area; if the saved image is also
missing, raise/raise a ValueError or RuntimeError with a clear message
indicating the feature ID/path and that the reference image is unavailable so
callers can handle the failure.
- Around line 267-292: The _run method currently dereferences self.feature and
self.path without setting them per-run; ensure self.feature and self.path are
bound from the run inputs at the start of each call to _run (or passed into _run
as parameters) before any use (e.g., before the logging line "Running milling
stage: {stage.name}" and before creating ref_img via from_odemis_image). Assign
the run-specific feature to self.feature and the run-specific path to self.path
(or use local variables and replace usages of self.feature/self.path in this
block) so that ref_img.metadata.image_settings.path and
from_odemis_image(self.feature.reference_image) always use the current run's
values and avoid stale metadata or AttributeError.
♻️ Duplicate comments (1)
src/odemis/acq/milling/millmng.py (1)
434-440: Pass feature/path into the fibsemOS runner.
run_milling_tasks_fibsemosnow relies on per-feature reference images and metadata path, so calling it withoutfeature(and the feature path) will either crash or produce incorrect metadata.✅ Proposed fix
- if fibsemos.FIBSEMOS_INSTALLED: - self._future.running_subf = run_milling_tasks_fibsemos(tasks=milling_tasks) + if fibsemos.FIBSEMOS_INSTALLED: + self._future.running_subf = run_milling_tasks_fibsemos( + tasks=milling_tasks, + feature=feature, + path=feature.path, + ) else: filename = self.get_filename(feature, "Milling-Tasks") self._future.running_subf = run_milling_tasks(tasks=milling_tasks, fib_stream=self.fib_stream, filename=filename)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/odemis/acq/milling/fibsemos.py`:
- Around line 75-80: The function _crop_to_reduced_area is missing a type
annotation for the rect parameter; update its signature to add an appropriate
type for rect (e.g., use typing.Any if the exact fibsemOS type isn't importable
or import and reference the specific MillingAlignment.rect type/class) so the
signature reads something like def _crop_to_reduced_area(ref_img: 'FibsemImage',
rect: Any) -> 'FibsemImage': and add the required import (from typing import
Any) if using Any.
🧹 Nitpick comments (8)
src/odemis/acq/milling/fibsemos.py (6)
21-21: Unused importREFERENCE_IMAGE_FILENAME.
REFERENCE_IMAGE_FILENAMEis imported but not used anywhere in this file.🔧 Suggested fix
-from odemis.acq.feature import CryoFeature, REFERENCE_IMAGE_FILENAME +from odemis.acq.feature import CryoFeature
43-51: Unused imports fromfibsem.structures.
FibsemImageMetadata,BeamType,ImageSettings, andMicroscopeStateare imported but not used in this file. OnlyFibsemMillingSettings,Point, andFibsemImageappear to be needed.🔧 Suggested fix
from fibsem.structures import ( FibsemMillingSettings, Point, FibsemImage, - FibsemImageMetadata, - BeamType, - ImageSettings, - MicroscopeState, )
91-99: Consider validating crop bounds produce a non-empty result.After clamping, if
x0 >= x1ory0 >= y1(due to invalid rect coordinates or extreme rounding), the crop will produce an empty array that could cause unexpected behavior downstream.🔧 Suggested fix
# clamp to valid range just in case of rounding x0 = max(0, min(w, x0)) x1 = max(0, min(w, x1)) y0 = max(0, min(h, y0)) y1 = max(0, min(h, y1)) + if x0 >= x1 or y0 >= y1: + raise ValueError(f"Invalid crop region: ({x0}, {y0}) to ({x1}, {y1})") + # crop along the last two axes, DataArray slicing behaves like numpy ref_img.data = ref_img.data[..., y0:y1, x0:x1]
252-262: Consider initializingself.pathandself.featurein__init__.These attributes are set in
async_runbut not initialized in__init__. While_runis only invoked viaasync_run, initializing them toNonewould make the class state explicit and prevent potentialAttributeErrorif these attributes are accessed before a run starts.🔧 Suggested fix
# per-run state (set in async_run) self.milling_stages: List["FibsemMillingStage"] = [] self._future: Optional[futures.Future] = None + self.path: Optional[str] = None + self.feature: Optional[CryoFeature] = None
316-322: Add docstring parameters forfeatureandpath.The docstring on line 322 should document the new
featureandpathparameters per coding guidelines (reStructuredText style).🔧 Suggested fix
def async_run(self, *, future: futures.Future, tasks: List[MillingTaskSettings], feature: CryoFeature, path: Optional[str] = None) -> futures.Future: - """Prepare and start a milling run asynchronously (one run at a time).""" + """Prepare and start a milling run asynchronously (one run at a time). + + :param future: The future to track progress. + :param tasks: The milling tasks to execute. + :param feature: The CryoFeature providing the reference image. + :param path: Optional output path for fibsemOS metadata/images. + :return: The future tracking the milling run. + """
352-360: Add docstring parameters forfeatureandpath.The docstring should document the new
featureandpathparameters.🔧 Suggested fix
def run_milling_tasks_fibsemos(tasks: List[MillingTaskSettings], feature: CryoFeature, path: Optional[str] = None) -> futures.Future: - """Run the given milling tasks asynchronously using a persistent fibsemOS manager.""" + """Run the given milling tasks asynchronously using a persistent fibsemOS manager. + + :param tasks: The milling tasks to execute. + :param feature: The CryoFeature providing the reference image. + :param path: Optional output path for fibsemOS metadata/images. + :return: A future tracking the milling run. + """src/odemis/acq/test/fibsemos_reference_image_test.py (2)
1-9: Remove unused imports.
osandREFERENCE_IMAGE_FILENAMEare imported but not used in this test file.🔧 Suggested fix
-import os import tempfile import unittest import numpy from odemis import model -from odemis.acq.feature import CryoFeature, REFERENCE_IMAGE_FILENAME +from odemis.acq.feature import CryoFeature from odemis.acq.milling.fibsemos import _get_reference_image
21-43: Replacesetattrwith direct assignment and remove redundant assignments.Per static analysis (B010),
setattrwith constant attribute names should be replaced with direct assignment. Additionally,reference_imageis already initialized toNoneinCryoFeature.__init__, so explicitly setting it is unnecessary.Also note that
test_raises_clear_error_if_path_missingandtest_raises_clear_error_if_file_missingtest the same code path astest_raises_if_missing_in_memorysince_get_reference_imageonly checksreference_imageand doesn't usepath. Consider if these tests are necessary or if the names should be updated to reflect what they actually test.🔧 Suggested fix
def test_raises_if_missing_in_memory(self): feature = CryoFeature(name="f1", stage_position={}, fm_focus_position={}) - setattr(feature, "reference_image", None) + # reference_image is already None after __init__ with self.assertRaises(ValueError): _get_reference_image(feature) def test_raises_clear_error_if_path_missing(self): feature = CryoFeature(name="f1", stage_position={}, fm_focus_position={}) - setattr(feature, "reference_image", None) - setattr(feature, "path", None) + # reference_image and path are already None after __init__ with self.assertRaises(ValueError): _get_reference_image(feature) def test_raises_clear_error_if_file_missing(self): feature = CryoFeature(name="f1", stage_position={}, fm_focus_position={}) - setattr(feature, "reference_image", None) + # reference_image is already None after __init__ with tempfile.TemporaryDirectory() as tmpdir: feature.path = tmpdir with self.assertRaises(ValueError): _get_reference_image(feature)
ilyushkin
force-pushed
the
msd-262-pass-correct-reference-image-to-fibsemos
branch
from
9fb81be to
ef4a9f3
Compare
ilyushkin
force-pushed
the
msd-262-pass-correct-reference-image-to-fibsemos
branch
from
ef4a9f3 to
0649cc9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/odemis/acq/milling/test/fibsemos_reference_image_test.py`:
- Around line 27-42: The three tests (test_raises_clear_error_if_path_missing,
test_raises_clear_error_if_file_missing, test_raises_if_missing_in_memory) all
exercise the same code path in _get_reference_image (it raises when
feature.reference_image is None), so remove or consolidate the redundant ones
(keep a single clear test that asserts ValueError when
CryoFeature.reference_image is None) or rename them to indicate they are
variants of the same scenario; also stop using setattr and assign attributes
directly (e.g., feature.reference_image = None and feature.path = tmpdir) to
satisfy static analysis and make the intent clear.
🧹 Nitpick comments (2)
src/odemis/acq/milling/fibsemos.py (1)
76-100: Document the in-place mutation behavior more explicitly.The function mutates
ref_img.datain place and returns the same instance. While the docstring mentions "The same image instance with cropped data," consider clarifying this is an in-place mutation, as callers might not expect their original image to be modified.💡 Suggested docstring improvement
def _crop_to_reduced_area(ref_img: 'FibsemImage', rect: 'FibsemRectangle') -> 'FibsemImage': - """Crop a fibsemOS image to the provided reduced-area rectangle. + """Crop a fibsemOS image to the provided reduced-area rectangle (in-place). :param ref_img: The image to crop. :param rect: Rectangle with fractional coordinates (left, top, width, height). - :return: The same image instance with cropped data. + :return: The same image instance with its data attribute cropped in-place. """src/odemis/acq/milling/test/fibsemos_reference_image_test.py (1)
20-25: Replacesetattrwith direct attribute assignment.As flagged by static analysis (B010), using
setattrwith a constant attribute name provides no benefit over direct assignment and reduces readability.♻️ Suggested fix
def test_raises_if_missing_in_memory(self): feature = CryoFeature(name="f1", stage_position={}, fm_focus_position={}) - setattr(feature, "reference_image", None) + feature.reference_image = None with self.assertRaises(ValueError): _get_reference_image(feature)
ilyushkin
force-pushed
the
msd-262-pass-correct-reference-image-to-fibsemos
branch
2 times, most recently
from
15bd844 to
caede8f
Compare
ilyushkin
force-pushed
the
msd-262-pass-correct-reference-image-to-fibsemos
branch
from
caede8f to
4f5f353
Compare
3 participants
