[MSD-613][feat] Meteor: localization for FIB-view FM posture #3492
Open
tmoerkerken wants to merge 1 commit into
Open
[MSD-613][feat] Meteor: localization for FIB-view FM posture #3492tmoerkerken wants to merge 1 commit into
tmoerkerken wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds “FM MILLING” posture awareness across Meteor’s GUI and posture-management layer so users can localize and label acquisitions correctly when imaging at milling angle, while preventing unsupported actions (e.g., overview acquisition).
Changes:
- Introduces a new
FM_MILLINGposture with switch restrictions, transforms, and simulation config updates. - Updates the Cryo Chamber + Localization tabs to reflect current posture (read-only indicator) and enable/disable controls appropriately.
- Extends lens metadata with working distance (
MD_LENS_WD) and propagates it for FM@Milling compatibility checks.
Reviewed changes
Copilot reviewed 17 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| util/groom-img.py | Adds usage guidance note for running the script on subfolders. |
| src/odemis/odemisd/mdupdater.py | Propagates lens working distance VA into metadata. |
| src/odemis/model/_metadata.py | Adds MD_LENS_WD metadata key definition. |
| src/odemis/gui/xmlh/resources/panel_tab_localization.xrc | Adds “Current posture” indicator controls to the Localization tab UI. |
| src/odemis/gui/xmlh/resources/panel_tab_cryosecom_chamber.xrc | Adds FM MILLING posture button and adjusts grid layout. |
| src/odemis/gui/main_xrc.py | Regenerates embedded XRC resources and adds new icon assets/controls. |
| src/odemis/gui/cont/tabs/localization_tab.py | Enables Localization tab at FM_MILLING, adds posture indicator logic, and appends milling-angle suffix to acquired stream names. |
| src/odemis/gui/cont/tabs/cryo_chamber_tab.py | Adds FM_MILLING into Meteor posture buttons and uses posture-manager switch-allowed logic + tooltips. |
| src/odemis/gui/cont/acquisition/cryo_acq.py | Disables “Acquire Overview” when acquiring or at FM_MILLING and sets contextual tooltip. |
| src/odemis/gui/comp/stream_panel.py | Changes stream label ellipsizing to middle (better visibility of suffix/prefix). |
| src/odemis/driver/static.py | Adds optional wd parameter and workingDistance VA on OpticalLens. |
| src/odemis/acq/test/move_tfs3_test.py | Adds test coverage for Milling ↔ FM_MILLING switching rules (TFS3). |
| src/odemis/acq/test/move_tescan_test.py | Adds test coverage for Milling ↔ FM_MILLING switching rules (Tescan) and updates imports. |
| src/odemis/acq/move.py | Adds FM_MILLING, posture detection, switching restrictions, transforms, and lens-WD-based capability gating. |
| install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-sim.odm.yaml | Adds lens wd, fixes milling rz, and YAML formatting. |
| install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-full-sim.odm.yaml | Adjusts sim motion params/ranges, adds lens wd, fixes milling rz, and YAML formatting. |
| install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yaml | Adds lens wd and YAML formatting. |
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR introduces a new FIB_VIEW_FM posture to the METEOR microscope posture system. It establishes a working-distance data contract ( Sequence DiagramsequenceDiagram
participant User
participant CryoChamberTab
participant MeteorPostureManager
participant TescanManager
participant Stage
User->>CryoChamberTab: click btn_switch_fib_view_fm
CryoChamberTab->>MeteorPostureManager: is_posture_switch_allowed(current, FIB_VIEW_FM)
alt current != MILLING
MeteorPostureManager-->>CryoChamberTab: False → show tooltip, disable button
else current == MILLING
MeteorPostureManager-->>CryoChamberTab: True
CryoChamberTab->>TescanManager: cryoSwitchSamplePosition(FIB_VIEW_FM)
TescanManager->>MeteorPostureManager: calculate_milling_angle()
TescanManager->>MeteorPostureManager: _transform_from_milling_to_fib_view_fm(pos)
TescanManager->>Stage: moveAbs(target_pos)
Stage-->>TescanManager: done
TescanManager-->>CryoChamberTab: posture = FIB_VIEW_FM
CryoChamberTab->>MeteorPostureManager: get_current_posture_label()
MeteorPostureManager-->>CryoChamberTab: "FIB VIEW FM"
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/acq/move.py (1)
2590-2596:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore Tescan shutter automation after leaving FM_MILLING.
Line 2576 forces
self.shutter.value = Falsewhen enteringFM_MILLING, but this reset only runs when leavingFM_IMAGING. AFM_MILLING -> MILLINGtransition leaves the shutter pinned retracted/open instead of returning it to automatic mode.Proposed fix
- if target in [MILLING, SEM_IMAGING] and current_posture == FM_IMAGING and self.shutter is not None: + if target in [MILLING, SEM_IMAGING] and current_posture in [FM_IMAGING, FM_MILLING] and self.shutter is not None: if self.shutter.value is False: logging.info("Setting shutter to automatic for transition from FM imaging") self.shutter.value = None # None = automatic🤖 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/move.py` around lines 2590 - 2596, The shutter reset to automatic mode only occurs when transitioning from FM_IMAGING to MILLING or SEM_IMAGING, but not when transitioning from FM_MILLING. To fix this, extend the conditional check at lines 2593-2596 to also include FM_MILLING as a valid source posture alongside FM_IMAGING. Change the condition from checking only "current_posture == FM_IMAGING" to also accepting "current_posture == FM_MILLING", ensuring the shutter is properly restored to automatic mode for both FM posture transitions.
🧹 Nitpick comments (1)
src/odemis/driver/static.py (1)
46-58: ⚡ Quick winAnnotate the changed constructor parameter and keep the docstring type-free.
wdwas added to a public constructor, but the signature remains untyped and the docstring adds inline type/range syntax. Please type the changed signature, add the return annotation, and move range/unit details into reStructuredText prose without type information.As per coding guidelines,
**/*.py: “Always use type hints for function parameters and return types” and “Include docstrings for all functions and classes, following the reStructuredText style guide, without type information and without using inline formatting markers or backticks”.🤖 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/driver/static.py` around lines 46 - 58, The __init__ method signature lacks type hints for its parameters and return annotation, and the docstring includes inline type/range syntax that violates the coding guidelines. Add type annotations to all parameters in the __init__ method signature (including the newly added wd parameter) and add a return type annotation of None. Then refactor the docstring to remove all inline type information from parameter descriptions (such as the ranges in parentheses like "0.1e-3 < float < 100e-3") and instead present the parameter information in prose format following reStructuredText style, describing the units and valid ranges as narrative text without type markers or backticks.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/odemis/acq/move.py`:
- Around line 557-568: The FM_MILLING availability gate is not consistently
applied across all code paths that interact with FM milling functionality. The
`at_fm_milling_posture` method currently only checks if the milling metadata
exists but does not verify whether FM milling is actually supported on the
system via the `has_fm_milling_objective` check. You must add the
`has_fm_milling_objective` availability check to the `at_fm_milling_posture`
method alongside the existing metadata check, and apply similar availability
guards to all other locations where FM_MILLING is allowed or transitioned to,
ensuring that unsupported FM milling remains unreachable on systems where it
should be disabled by objective working distance constraints.
- Around line 854-896: The calculate_stage_tilt and calculate_milling_angle
methods perform arithmetic operations with self.pre_tilt without first verifying
it is not None, which causes a TypeError when these methods are called during
base initialization before the subclass assigns the pre_tilt fallback value. Add
defensive checks at the beginning of both methods to guard against None values
of self.pre_tilt before the arithmetic operations occur, either by raising an
informative error or returning early with a sensible default. Additionally,
ensure that the posture detection logic does not invoke these methods during
base __init__ before the subclass initialization has completed and assigned the
pre_tilt fallback metadata.
- Around line 1086-1094: The _transform_from_fm_milling_to_fib() method
incorrectly treats an FM_MILLING position as FM_IMAGING by calling
_transform_from_fm_to_milling(pos) as the first transformation step. Replace
this call with the correct method that uses the FM_MILLING-to-MILLING leg
(likely _transform_from_fm_milling_to_milling(pos)) to ensure the transform
chain properly handles FM_MILLING positions and produces the correct
intermediate position before transforming to FIB_IMAGING.
In `@src/odemis/acq/test/move_tescan_test.py`:
- Around line 320-321: Both test method definitions for
test_fm_milling_movements are missing return type annotations, violating the
repository's Python type-hint guidelines. In
src/odemis/acq/test/move_tescan_test.py at lines 320-321, add the return type
annotation to the test_fm_milling_movements method signature by changing it to
include -> None: before the colon. Apply the identical fix in
src/odemis/acq/test/move_tfs3_test.py at lines 356-357 to the
test_fm_milling_movements method signature by adding -> None: to match the
repository coding standards requiring type hints for all function signatures.
In `@src/odemis/gui/cont/acquisition/cryo_acq.py`:
- Around line 769-783: The method _update_overview_acquisition_button is missing
a return type annotation and has a placeholder docstring. Add the return type
hint `-> None` to the method signature after the closing parenthesis, and
replace the placeholder docstring (with ellipsis) with a meaningful docstring
that describes what the method does - specifically that it updates the enabled
state and tooltip of the overview acquisition button based on whether the system
is currently acquiring and the posture manager's current position.
In `@src/odemis/gui/cont/tabs/localization_tab.py`:
- Around line 500-506: The posture indicator update logic in the if-elif chain
only handles FM_MILLING and FM_IMAGING postures, leaving other positions with
stale UI state. Add an else branch after the elif condition that checks
at_fm_imaging_posture(pos) to explicitly set both self.panel.lbl_current_posture
and self.panel.bmp_current_posture to appropriate default or fallback values for
all other postures, ensuring the displayed posture state is always explicit and
up-to-date rather than showing leftover values from previous updates.
- Around line 451-454: The code does not guard against the case where
MD_STAGE_POSITION_RAW is absent from the metadata, causing pos to become None
and potentially causing the pm.at_fm_milling_posture(pos) call to fail at
runtime. Add a guard condition to check that pos is not None before calling
pm.at_fm_milling_posture(pos) and proceeding with the stream name update. When
the metadata is unavailable, the stream name should remain unchanged.
---
Outside diff comments:
In `@src/odemis/acq/move.py`:
- Around line 2590-2596: The shutter reset to automatic mode only occurs when
transitioning from FM_IMAGING to MILLING or SEM_IMAGING, but not when
transitioning from FM_MILLING. To fix this, extend the conditional check at
lines 2593-2596 to also include FM_MILLING as a valid source posture alongside
FM_IMAGING. Change the condition from checking only "current_posture ==
FM_IMAGING" to also accepting "current_posture == FM_MILLING", ensuring the
shutter is properly restored to automatic mode for both FM posture transitions.
---
Nitpick comments:
In `@src/odemis/driver/static.py`:
- Around line 46-58: The __init__ method signature lacks type hints for its
parameters and return annotation, and the docstring includes inline type/range
syntax that violates the coding guidelines. Add type annotations to all
parameters in the __init__ method signature (including the newly added wd
parameter) and add a return type annotation of None. Then refactor the docstring
to remove all inline type information from parameter descriptions (such as the
ranges in parentheses like "0.1e-3 < float < 100e-3") and instead present the
parameter information in prose format following reStructuredText style,
describing the units and valid ranges as narrative text without type markers or
backticks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc72bcca-f0e3-4ace-a19f-e02f4455d84e
⛔ Files ignored due to path filters (6)
src/odemis/gui/img/icon/ico_meteorimaging.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_green.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_milling.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_milling_green.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_milling_orange.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_orange.pngis excluded by!**/*.png
📒 Files selected for processing (17)
install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yamlinstall/linux/usr/share/odemis/sim/meteor-tescan-fibsem-full-sim.odm.yamlinstall/linux/usr/share/odemis/sim/meteor-tescan-fibsem-sim.odm.yamlsrc/odemis/acq/move.pysrc/odemis/acq/test/move_tescan_test.pysrc/odemis/acq/test/move_tfs3_test.pysrc/odemis/driver/static.pysrc/odemis/gui/comp/stream_panel.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/cont/tabs/localization_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/panel_tab_cryosecom_chamber.xrcsrc/odemis/gui/xmlh/resources/panel_tab_localization.xrcsrc/odemis/model/_metadata.pysrc/odemis/odemisd/mdupdater.pyutil/groom-img.py
Comment on lines
+1086
to
+1094
| def _transform_from_fm_milling_to_fib(self, pos: Dict[str, float]) -> Dict[str, float]: | ||
| """ | ||
| Transforms the stage position from fm milling to fib | ||
| :param pos: the current stage position | ||
| :return: the transformed stage position. | ||
| """ | ||
| # simple chain of fm milling->milling->fib | ||
| milling_pos = self._transform_from_fm_to_milling(pos) | ||
| return self._transform_from_milling_to_fib(milling_pos) |
There was a problem hiding this comment.
Use the FM_MILLING-to-MILLING leg in this transform chain.
_transform_from_fm_milling_to_fib() currently starts with _transform_from_fm_to_milling(pos), treating an FM_MILLING position as FM_IMAGING. The transform table exposes FM_MILLING -> FIB_IMAGING, so direct to_posture() callers can get the wrong intermediate position.
Proposed fix
"""
# simple chain of fm milling->milling->fib
- milling_pos = self._transform_from_fm_to_milling(pos)
+ milling_pos = self._transform_from_fm_milling_to_milling(pos)
return self._transform_from_milling_to_fib(milling_pos)🤖 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/move.py` around lines 1086 - 1094, The
_transform_from_fm_milling_to_fib() method incorrectly treats an FM_MILLING
position as FM_IMAGING by calling _transform_from_fm_to_milling(pos) as the
first transformation step. Replace this call with the correct method that uses
the FM_MILLING-to-MILLING leg (likely
_transform_from_fm_milling_to_milling(pos)) to ensure the transform chain
properly handles FM_MILLING positions and produces the correct intermediate
position before transforming to FIB_IMAGING.
Comment on lines
+320
to
+321
| def test_fm_milling_movements(self): | ||
| """Test that milling <> fm milling switches work as expected""" |
There was a problem hiding this comment.
Apply the Python type-hint guideline consistently to the new FM_MILLING test methods. Both added test functions omit return type annotations, which violates the repository rule requiring type hints for Python function signatures.
src/odemis/acq/test/move_tescan_test.py#L320-L321: change method signature todef test_fm_milling_movements(self) -> None:.src/odemis/acq/test/move_tfs3_test.py#L356-L357: change method signature todef test_fm_milling_movements(self) -> None:.
As per coding guidelines, **/*.py: Always use type hints for function parameters and return types in Python code.
📍 Affects 2 files
src/odemis/acq/test/move_tescan_test.py#L320-L321(this comment)src/odemis/acq/test/move_tfs3_test.py#L356-L357
🤖 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/test/move_tescan_test.py` around lines 320 - 321, Both test
method definitions for test_fm_milling_movements are missing return type
annotations, violating the repository's Python type-hint guidelines. In
src/odemis/acq/test/move_tescan_test.py at lines 320-321, add the return type
annotation to the test_fm_milling_movements method signature by changing it to
include -> None: before the colon. Apply the identical fix in
src/odemis/acq/test/move_tfs3_test.py at lines 356-357 to the
test_fm_milling_movements method signature by adding -> None: to match the
repository coding standards requiring type hints for all function signatures.
Source: Coding guidelines
tmoerkerken
force-pushed
the
fm-milling-localization
branch
from
0fe06af to
a1a5bb5
Compare
tmoerkerken
changed the title
tmoerkerken
force-pushed
the
fm-milling-localization
branch
from
a1a5bb5 to
f721287
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odemis/acq/move.py (1)
557-596: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate FIB-view FM detection and switches on actual availability.
at_fib_view_fm_posture()only checks milling metadata, andis_posture_switch_allowed()allowsMILLING -> FIB_VIEW_FMwhenever the transform key exists. On an objective that fails the working-distance gate, this can still enable/detect unsupported FIB-view FM; iffm_column_tiltisNone, Line 596 also falls back to the FIB column tilt throughcalculate_stage_tilt().Proposed availability guard
+ def _is_fib_view_fm_available(self) -> bool: + """Return whether FIB-view FM posture can be used.""" + stage_md = self.stage.getMetadata() + return ( + self.has_fib_view_fm_objective + and self.fm_column_tilt is not None + and model.MD_FAV_MILL_POS_ACTIVE in stage_md + ) + def at_fib_view_fm_posture(self, pos: Dict[str, float]) -> bool: @@ - stage_md = self.stage.getMetadata() - - if model.MD_FAV_MILL_POS_ACTIVE not in stage_md: + if not self._is_fib_view_fm_available(): return False @@ elif posture == FIB_VIEW_FM: + if not self._is_fib_view_fm_available(): + raise ValueError("FIB-view FM posture is not available for this objective") md = stage_md[model.MD_FAV_MILL_POS_ACTIVE] rx = self.calculate_stage_tilt(column_tilt=self.fm_column_tilt) return {"rx": rx, "rz": md["rz"]} @@ # For FIB-view FM we only allow to go to and from the milling posture. + elif FIB_VIEW_FM in (source_posture, target_posture) and not self._is_fib_view_fm_available(): + return False elif source_posture != MILLING and target_posture == FIB_VIEW_FM: return FalseAlso applies to: 1117-1135
🤖 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/move.py` around lines 557 - 596, The FIB-view FM detection and transition logic do not properly gate on actual availability of required parameters. In the `at_fib_view_fm_posture()` method, add a check to verify that `fm_column_tilt` is not None before returning True, since FIB_VIEW_FM posture requires this parameter. Similarly, in the `is_posture_switch_allowed()` method (referenced at lines 1117-1135), add a gate to prevent transitions from MILLING to FIB_VIEW_FM when `fm_column_tilt` is None. Additionally, in the `get_posture_orientation()` method, ensure the FIB_VIEW_FM case properly validates that `fm_column_tilt` is available before using it with `calculate_stage_tilt(column_tilt=self.fm_column_tilt)` to prevent falling back to unsupported FIB column tilt.
♻️ Duplicate comments (2)
src/odemis/acq/test/move_tescan_test.py (1)
320-320: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the missing return type annotation on this test method.
Please annotate this as
def test_fib_view_fm_movements(self) -> None:to satisfy the repository Python typing rule.
As per coding guidelines,**/*.py: Always use type hints for function parameters and return types in Python code.🤖 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/test/move_tescan_test.py` at line 320, The test method test_fib_view_fm_movements is missing a return type annotation required by the repository's Python typing guidelines. Add the return type annotation -> None to the method signature after the closing parenthesis to indicate that this test method does not return a value.Source: Coding guidelines
src/odemis/acq/test/move_tfs3_test.py (1)
356-356: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the missing return type annotation on this test method.
Please annotate this as
def test_fib_view_fm_movements(self) -> None:to satisfy the repository Python typing rule.
As per coding guidelines,**/*.py: Always use type hints for function parameters and return types in Python code.🤖 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/test/move_tfs3_test.py` at line 356, The test method test_fib_view_fm_movements is missing a return type annotation. Add the return type annotation `-> None:` to the method signature to comply with the repository's Python typing rules. Change `def test_fib_view_fm_movements(self):` to `def test_fib_view_fm_movements(self) -> None:` to explicitly indicate that this test method returns nothing.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/odemis/acq/move.py`:
- Line 2572: The cleanup code that resets the Tescan shutter to automatic mode
after leaving FIB-view mode only checks for current_posture == FM_IMAGING, but
now that FIB_VIEW_FM is also handled at line 2572 for shutter control, the
cleanup condition needs to be expanded. Find the cleanup code section (likely
later in the function) that handles resetting the shutter and modify the
condition to check for both FM_IMAGING and FIB_VIEW_FM in the same way line 2572
does with the target check, so the shutter is properly returned to automatic
mode when exiting either of these modes to MILLING.
In `@src/odemis/acq/test/move_tescan_test.py`:
- Line 339: In the test_CloseTool function, update the assert_pos_almost_equal
call to include an explicit atol parameter to match the pattern used in other
stage position tests throughout the file. Modify the call from
assert_pos_almost_equal(initial_pos, final_pos) to include atol with a value
like 1e-6 to reduce flakiness from hardware or simulator drift.
In `@src/odemis/acq/test/move_tfs3_test.py`:
- Line 376: The assert_pos_almost_equal function call comparing initial_pos and
final_pos is missing an explicit atol parameter, which can cause flaky test
results due to floating point precision and stage drift. Add an explicit atol
parameter to the assert_pos_almost_equal call to set an appropriate tolerance
value that accounts for expected floating point variations and stage movement
drift, making the test more robust and reliable.
In `@src/odemis/gui/cont/stream_bar.py`:
- Around line 2377-2379: The code retrieves metadata using a `.get()` call which
can return `None` if `MD_STAGE_POSITION_RAW` is absent, and then passes this
potentially `None` value directly to `pm.at_fib_view_fm_posture()`, which causes
rendering issues. Add a guard condition to check that `pos` is not `None` before
calling `pm.at_fib_view_fm_posture(pos)`, ensuring the posture detection only
runs when valid stage position metadata is available.
---
Outside diff comments:
In `@src/odemis/acq/move.py`:
- Around line 557-596: The FIB-view FM detection and transition logic do not
properly gate on actual availability of required parameters. In the
`at_fib_view_fm_posture()` method, add a check to verify that `fm_column_tilt`
is not None before returning True, since FIB_VIEW_FM posture requires this
parameter. Similarly, in the `is_posture_switch_allowed()` method (referenced at
lines 1117-1135), add a gate to prevent transitions from MILLING to FIB_VIEW_FM
when `fm_column_tilt` is None. Additionally, in the `get_posture_orientation()`
method, ensure the FIB_VIEW_FM case properly validates that `fm_column_tilt` is
available before using it with
`calculate_stage_tilt(column_tilt=self.fm_column_tilt)` to prevent falling back
to unsupported FIB column tilt.
---
Duplicate comments:
In `@src/odemis/acq/test/move_tescan_test.py`:
- Line 320: The test method test_fib_view_fm_movements is missing a return type
annotation required by the repository's Python typing guidelines. Add the return
type annotation -> None to the method signature after the closing parenthesis to
indicate that this test method does not return a value.
In `@src/odemis/acq/test/move_tfs3_test.py`:
- Line 356: The test method test_fib_view_fm_movements is missing a return type
annotation. Add the return type annotation `-> None:` to the method signature to
comply with the repository's Python typing rules. Change `def
test_fib_view_fm_movements(self):` to `def test_fib_view_fm_movements(self) ->
None:` to explicitly indicate that this test method returns nothing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82eec535-3137-431a-991c-ba19b4fe5996
⛔ Files ignored due to path filters (6)
src/odemis/gui/img/icon/ico_meteorimaging.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_green.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_milling.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_milling_green.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_milling_orange.pngis excluded by!**/*.pngsrc/odemis/gui/img/icon/ico_meteorimaging_orange.pngis excluded by!**/*.png
📒 Files selected for processing (19)
install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yamlinstall/linux/usr/share/odemis/sim/meteor-tescan-fibsem-full-sim.odm.yamlinstall/linux/usr/share/odemis/sim/meteor-tescan-fibsem-sim.odm.yamlsrc/odemis/acq/feature.pysrc/odemis/acq/move.pysrc/odemis/acq/test/move_tescan_test.pysrc/odemis/acq/test/move_tfs3_test.pysrc/odemis/driver/static.pysrc/odemis/gui/comp/stream_panel.pysrc/odemis/gui/cont/acquisition/cryo_acq.pysrc/odemis/gui/cont/stream_bar.pysrc/odemis/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/cont/tabs/localization_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/panel_tab_cryosecom_chamber.xrcsrc/odemis/gui/xmlh/resources/panel_tab_localization.xrcsrc/odemis/model/_metadata.pysrc/odemis/odemisd/mdupdater.pyutil/groom-img.py
✅ Files skipped from review due to trivial changes (3)
- util/groom-img.py
- src/odemis/acq/feature.py
- src/odemis/gui/comp/stream_panel.py
🚧 Files skipped from review as they are similar to previous changes (10)
- src/odemis/model/_metadata.py
- install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-sim.odm.yaml
- src/odemis/driver/static.py
- src/odemis/odemisd/mdupdater.py
- src/odemis/gui/xmlh/resources/panel_tab_localization.xrc
- src/odemis/gui/cont/tabs/localization_tab.py
- install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yaml
- install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-full-sim.odm.yaml
- src/odemis/gui/cont/acquisition/cryo_acq.py
- src/odemis/gui/cont/tabs/cryo_chamber_tab.py
| sub_moves.append((self.stage, filter_dict({'z'}, target_pos))) # Move the final Z | ||
|
|
||
| if target == FM_IMAGING: | ||
| if target in [FM_IMAGING, FIB_VIEW_FM]: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset the Tescan shutter after leaving FIB-view FM.
Line 2572 now retracts/opens the shutter when entering FIB_VIEW_FM, but the cleanup later only handles current_posture == FM_IMAGING. A FIB_VIEW_FM -> MILLING switch can leave the shutter manually retracted instead of returning it to automatic mode.
Proposed cleanup fix
- if target in [MILLING, SEM_IMAGING] and current_posture == FM_IMAGING and self.shutter is not None:
+ if (target in [MILLING, SEM_IMAGING]
+ and current_posture in [FM_IMAGING, FIB_VIEW_FM]
+ and self.shutter is not None):
if self.shutter.value is False:
logging.info("Setting shutter to automatic for transition from FM imaging")
self.shutter.value = None # None = automatic🤖 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/move.py` at line 2572, The cleanup code that resets the Tescan
shutter to automatic mode after leaving FIB-view mode only checks for
current_posture == FM_IMAGING, but now that FIB_VIEW_FM is also handled at line
2572 for shutter control, the cleanup condition needs to be expanded. Find the
cleanup code section (likely later in the function) that handles resetting the
shutter and modify the condition to check for both FM_IMAGING and FIB_VIEW_FM in
the same way line 2572 does with the target check, so the shutter is properly
returned to automatic mode when exiting either of these modes to MILLING.
| resulting_posture = self.pm.get_current_posture_label() | ||
| self.assertEqual(resulting_posture, MILLING) | ||
| final_pos = self.stage_bare.position.value | ||
| testing.assert_pos_almost_equal(initial_pos, final_pos) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use an explicit tolerance for stage position restore assertion.
testing.assert_pos_almost_equal(initial_pos, final_pos) should pass an explicit atol (for example 1e-6) like the other stage-position tests in this file, to reduce hardware/simulator drift flakes.
🤖 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/test/move_tescan_test.py` at line 339, In the test_CloseTool
function, update the assert_pos_almost_equal call to include an explicit atol
parameter to match the pattern used in other stage position tests throughout the
file. Modify the call from assert_pos_almost_equal(initial_pos, final_pos) to
include atol with a value like 1e-6 to reduce flakiness from hardware or
simulator drift.
| resulting_posture = self.pm.get_current_posture_label() | ||
| self.assertEqual(resulting_posture, MILLING) | ||
| final_pos = self.stage_bare.position.value | ||
| testing.assert_pos_almost_equal(initial_pos, final_pos) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use an explicit tolerance for the final position comparison.
testing.assert_pos_almost_equal(initial_pos, final_pos) should set an explicit atol to avoid flaky restores caused by tiny floating/stage drift.
🤖 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/test/move_tfs3_test.py` at line 376, The
assert_pos_almost_equal function call comparing initial_pos and final_pos is
missing an explicit atol parameter, which can cause flaky test results due to
floating point precision and stage drift. Add an explicit atol parameter to the
assert_pos_almost_equal call to set an appropriate tolerance value that accounts
for expected floating point variations and stage movement drift, making the test
more robust and reliable.
Comment on lines
+2377
to
+2379
| pos = s.getRawMetadata()[0].get(model.MD_STAGE_POSITION_RAW) | ||
| # Add milling angle suffix to stream's name if it was acquired at the milling angle. | ||
| if pm.at_fib_view_fm_posture(pos): |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard posture detection against missing stage-position metadata
At Line 2377, MD_STAGE_POSITION_RAW can be absent; passing None to pm.at_fib_view_fm_posture(...) can break stream rendering. Add a metadata guard before posture detection.
Suggested patch
- pos = s.getRawMetadata()[0].get(model.MD_STAGE_POSITION_RAW)
+ pos = s.getRawMetadata()[0].get(model.MD_STAGE_POSITION_RAW)
# Add milling angle suffix to stream's name if it was acquired at the milling angle.
- if pm.at_fib_view_fm_posture(pos):
+ if pos is not None and pm.at_fib_view_fm_posture(pos):
s.name.value = f"{s.name.value} at {math.degrees(pm.milling_angle.value):0.0f}°"🤖 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/gui/cont/stream_bar.py` around lines 2377 - 2379, The code
retrieves metadata using a `.get()` call which can return `None` if
`MD_STAGE_POSITION_RAW` is absent, and then passes this potentially `None` value
directly to `pm.at_fib_view_fm_posture()`, which causes rendering issues. Add a
guard condition to check that `pos` is not `None` before calling
`pm.at_fib_view_fm_posture(pos)`, ensuring the posture detection only runs when
valid stage position metadata is available.
pieleric
requested changes
Jun 30, 2026
tmoerkerken
force-pushed
the
fm-milling-localization
branch
from
f721287 to
50fb4c9
Compare
tmoerkerken
force-pushed
the
fm-milling-localization
branch
from
50fb4c9 to
d77cb49
Compare
tmoerkerken
commented
Jul 6, 2026
tmoerkerken
force-pushed
the
fm-milling-localization
branch
from
d77cb49 to
666bef4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add FIB-view FM support to the localization tab:
Screenshots
Read-only indicator in the localization tab
When image is acquired at FM Milling, we indicate that via the "at " suffix
Overview acquisition disabled when at FIB-view FM posture