[MSD-612][feat] Meteor: Milling <> FM Milling posture switching (TESCAN/TFS)#3479
Open
tmoerkerken wants to merge 1 commit into
Open
[MSD-612][feat] Meteor: Milling <> FM Milling posture switching (TESCAN/TFS)#3479tmoerkerken wants to merge 1 commit into
tmoerkerken wants to merge 1 commit into
Conversation
tmoerkerken
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a new “FM Milling” posture for METEOR systems, including UI controls, posture-switch restrictions (only Milling ↔ FM Milling), simulation config updates, and test coverage for the new posture switching.
Changes:
- Introduce new posture constant
FM_MILLINGand integrate it into posture detection, transforms, and movement switching logic. - Add a new chamber-tab button/resources for “FM MILLING” and enable/disable logic based on allowed posture transitions.
- Extend METEOR TFS3 and Tescan posture-switch tests and update simulator microscope configs to include an FM-milling focuser position.
Reviewed changes
Copilot reviewed 10 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| util/groom-img.py | Adds usage notes for running the image grooming script. |
| src/odemis/gui/xmlh/resources/panel_tab_cryosecom_chamber.xrc | Adds an “FM MILLING” posture button and updates grid layout positions. |
| src/odemis/gui/main_xrc.py | Wires the new btn_switch_fm_milling control and embeds new icon resources. |
| src/odemis/gui/cont/tabs/cryo_chamber_tab.py | Adds FM_MILLING button handling and restricts METEOR posture switching via is_posture_switch_allowed(). |
| src/odemis/acq/test/move_tfs3_test.py | Adds a test for Milling ↔ FM Milling switching on TFS3 FIBSEM sim config. |
| src/odemis/acq/test/move_tescan_test.py | Adds a test for Milling ↔ FM Milling switching on Tescan sim config. |
| src/odemis/acq/move.py | Core implementation: new posture constant, posture detection, transforms, and switch-allowed logic for FM milling. |
| install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-sim.odm.yaml | Updates milling posture rotation to align with intended posture behavior. |
| install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-full-sim.odm.yaml | Adds focuser FAV_MILL_POS_ACTIVE and reformats portions of the sim config. |
| install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yaml | Adds focuser FAV_MILL_POS_ACTIVE for FM milling in the METEOR FIBSEM sim config. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds FM_MILLING posture support end-to-end: simulator YAMLs gain milling focus/rotation entries; src/odemis/acq/move.py adds the FM_MILLING constant, detection helpers, posture-orientation handling, posture-transform routing, tilt/angle helpers, and allowed-transition rules; TFS1/TFS3/Tescan managers are updated (Tescan receives full transform math and milling-angle bookkeeping); GUI gains an FM MILLING button and per-button enablement; tests cover FM_MILLING transitions; a small utility comment is updated. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
🧹 Nitpick comments (3)
src/odemis/acq/move.py (2)
1014-1017: 💤 Low valueMinor formatting: remove extra space before
False.Lines 1015 and 1017 have an extra space before
False. This is a minor style inconsistency.✨ Proposed fix
if target_posture == FM_MILLING and source_posture != MILLING: - move_allowed = False + move_allowed = False elif source_posture == FM_MILLING and target_posture != MILLING: - move_allowed = False + move_allowed = False🤖 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 1014 - 1017, The conditional assignments to move_allowed use an extra space before False on the lines checking target_posture and source_posture; update the two statements inside the block that reference target_posture, FM_MILLING, source_posture, and MILLING so they assign move_allowed = False (remove the stray space before False) while keeping the existing logic and variable names intact.
547-558: 💤 Low valueSimplify conditional logic.
The
elseclause on line 556 is unnecessary since line 555 already returnsTruewhen the condition is met. All other paths naturally fall through toreturn False.♻️ Proposed simplification
if model.MD_FAV_MILL_POS_ACTIVE in focus_md: stage_fm_milling = self.get_posture_orientation(FM_MILLING) if isNearPosition( pos, stage_fm_milling, self.rotational_axes, atol_rotation=math.radians(3) ): return True - else: - return False + return False🤖 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 547 - 558, The else block is redundant: inside the method handling the MD_FAV_MILL_POS_ACTIVE check (using model.MD_FAV_MILL_POS_ACTIVE) call get_posture_orientation(FM_MILLING) and use isNearPosition(...) to return True when matched; remove the surrounding else and let the function fall through to a single return False at the end so only the matching branch returns True and all other paths return False. Ensure you update the block around get_posture_orientation(FM_MILLING) and isNearPosition to eliminate the unnecessary else.src/odemis/acq/test/move_tescan_test.py (1)
310-322: 💤 Low valueInconsistent posture-assertion approach within the test.
The first switch is validated via
self.pm.current_posture.value(Line 313) while the subsequent two useget_current_posture_label()(Lines 316, 320). Using one approach throughout makes the test clearer and exercises a consistent contract.♻️ Optional consistency tweak
self.pm.cryo_switch_sample_position(MILLING).result() - self.assertEqual(self.pm.current_posture.value, MILLING) + self.assertEqual(self.pm.get_current_posture_label(), MILLING)🤖 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 310 - 322, The test_fm_milling_movements uses two different ways to assert posture (directly reading self.pm.current_posture.value for the first switch and get_current_posture_label() for the later checks); make them consistent by switching the first assertion to use self.pm.get_current_posture_label() (or alternatively use current_posture.value for all three) after calling self.pm.cryo_switch_sample_position(...).result(), so all three assertions reference the same contract (refer to cryo_switch_sample_position, current_posture.value, and get_current_posture_label to locate the lines to change).
🤖 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 35: The file contains an unused import "from sympy import true" in
src/odemis/acq/move.py; remove that import line to eliminate an unnecessary
dependency and clean up the module (no other code changes are required).
- Line 342: The assignment to self.fm_column_tilt uses
stage_md.get(model.MD_FAV_FM_POS_ACTIVE)["rx"] before metadata is validated,
which can raise unclear KeyError/TypeError; update the code (in the same
initializer/constructor or wherever self.fm_column_tilt is set) to either defer
computing self.fm_column_tilt until after check_stage_metadata() runs or
explicitly guard the access: verify that stage_md contains
model.MD_FAV_FM_POS_ACTIVE and that the returned dict has an "rx" key, and if
not raise a clear, descriptive exception (e.g., "missing stage metadata:
MD_FAV_FM_POS_ACTIVE.rx") so callers get a helpful error instead of a raw
KeyError/TypeError. Ensure references to self.pre_tilt, stage_md,
model.MD_FAV_FM_POS_ACTIVE, and the check_stage_metadata() call are used to
locate and implement this fix.
- Line 1912: The assignment to self.fm_column_tilt uses
stage_md.get(model.MD_FAV_FM_POS_ACTIVE)["rx"] which can raise a confusing
KeyError if MD_FAV_FM_POS_ACTIVE is missing or has no "rx"; change it to safely
retrieve and validate the metadata (e.g., fav =
stage_md.get(model.MD_FAV_FM_POS_ACTIVE); rx = fav.get("rx") if fav else None)
and only compute self.fm_column_tilt = self.pre_tilt + rx when rx is present,
otherwise handle the missing value consistently (log/raise or use a default)
following the same guard pattern used earlier around line 342.
In `@util/groom-img.py`:
- Around line 8-9: Update the comment in groom-img.py to explicitly state that
the issue occurs when the directory argument passed to the script is the
repository root (e.g., “.”), not when running the script from the repo root;
note that the script’s default target is src/ and recommend calling it on a
subfolder (or passing a subfolder path) to avoid the known issues, aligning this
phrasing with doc/develop/extending.rst.
---
Nitpick comments:
In `@src/odemis/acq/move.py`:
- Around line 1014-1017: The conditional assignments to move_allowed use an
extra space before False on the lines checking target_posture and
source_posture; update the two statements inside the block that reference
target_posture, FM_MILLING, source_posture, and MILLING so they assign
move_allowed = False (remove the stray space before False) while keeping the
existing logic and variable names intact.
- Around line 547-558: The else block is redundant: inside the method handling
the MD_FAV_MILL_POS_ACTIVE check (using model.MD_FAV_MILL_POS_ACTIVE) call
get_posture_orientation(FM_MILLING) and use isNearPosition(...) to return True
when matched; remove the surrounding else and let the function fall through to a
single return False at the end so only the matching branch returns True and all
other paths return False. Ensure you update the block around
get_posture_orientation(FM_MILLING) and isNearPosition to eliminate the
unnecessary else.
In `@src/odemis/acq/test/move_tescan_test.py`:
- Around line 310-322: The test_fm_milling_movements uses two different ways to
assert posture (directly reading self.pm.current_posture.value for the first
switch and get_current_posture_label() for the later checks); make them
consistent by switching the first assertion to use
self.pm.get_current_posture_label() (or alternatively use current_posture.value
for all three) after calling self.pm.cryo_switch_sample_position(...).result(),
so all three assertions reference the same contract (refer to
cryo_switch_sample_position, current_posture.value, and
get_current_posture_label to locate the lines to change).
🪄 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: 94fe23cb-5cd2-4497-921a-bdab3016ac1f
⛔ 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 (10)
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/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/panel_tab_cryosecom_chamber.xrcutil/groom-img.py
tmoerkerken
force-pushed
the
fm-milling-posture
branch
4 times, most recently
from
4d871e8 to
8c00a8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-sim.odm.yaml`:
- Line 146: The focus metadata entry MD_FAV_MILL_POS_ACTIVE is missing for the
Optical Focus, so the simple Tescan simulator cannot enter FM_MILLING even
though the stage advertises FAV_MILL_POS_ACTIVE; add a matching
MD_FAV_MILL_POS_ACTIVE key under the Optical Focus.metadata with the same
value/object as FAV_MILL_POS_ACTIVE (e.g., {"mill_angle": 0.174532925, "rz":
1.815142}) so MeteorTescan1PostureManager will expose FM_MILLING when both stage
and focus provide that metadata.
In `@src/odemis/acq/move.py`:
- Around line 1903-1909: Ensure self.fm_column_tilt is always initialized for
Tescan startup: in the constructor/initialization path where pre-tilt may come
from MD_CALIB["pre-tilt"] (so the existing if self.pre_tilt is None branch can
be skipped), explicitly set self.fm_column_tilt using the available sources—use
self.pre_tilt if present, otherwise fall back to model.getComponent("Linked
YZ").getMetadata()[model.MD_ROTATION_COR], and then add
stage_md.get(model.MD_FAV_FM_POS_ACTIVE)["rx"] as currently done; update the
initialization logic so that _update_conversion() and
milling_angle.subscribe(..., init=True) never see an unset fm_column_tilt
(ensuring FM_MILLING-enabled Tescan does not raise AttributeError).
- Around line 994-1015: The posture-allowance check in is_posture_switch_allowed
is not being used by backend operations, so calls in get_target_position and
cryo_switch_sample_position can bypass workflow restrictions (e.g., SEM_IMAGING
-> FM_MILLING); update those functions to call
is_posture_switch_allowed(source_posture, target_posture) early and raise/return
an error (or refuse the transition) when it returns False before using
_posture_transforms or proceeding with metadata/path logic; ensure both
get_target_position and cryo_switch_sample_position reference the same error
handling behavior so unsupported FM_MILLING transitions are blocked
consistently.
- Around line 2291-2330: The bug is that rx_milling is incorrectly set from the
source pos ("rx") so the transform doesn't apply the FIB-column tilt difference;
replace the assignment to rx_milling so it computes the actual milling-stage
tilt (use calculate_stage_tilt_from_milling_angle) rather than copying pos["rx"]
— update the rx_milling variable (and keep rx_fm_milling as computed) and ensure
the returned dict writes that computed rx_milling as "rx" so the stage is set to
the milling tilt after _transform_from_fm_milling_to_milling.
🪄 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: 5f78717b-50f4-4b18-926a-583e69280ccc
⛔ 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 (10)
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/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/panel_tab_cryosecom_chamber.xrcutil/groom-img.py
✅ Files skipped from review due to trivial changes (1)
- util/groom-img.py
🚧 Files skipped from review as they are similar to previous changes (4)
- install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yaml
- src/odemis/acq/test/move_tescan_test.py
- src/odemis/gui/xmlh/resources/panel_tab_cryosecom_chamber.xrc
- src/odemis/gui/cont/tabs/cryo_chamber_tab.py
tmoerkerken
force-pushed
the
fm-milling-posture
branch
from
8c00a8d to
7b97fea
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/test/move_tescan_test.py`:
- Around line 310-324: The test method test_fm_milling_movements is missing type
annotations; update its signature to include parameter and return type hints
(e.g., annotate self as unittest.TestCase or the appropriate test class type and
add -> None) and add any required imports for the annotation if missing so the
function reads like: def test_fm_milling_movements(self: unittest.TestCase) ->
None; keep the body unchanged and ensure imports are added at the top if
necessary.
In `@src/odemis/acq/test/move_tfs3_test.py`:
- Around line 356-370: The test method test_fm_milling_movements is missing a
return type annotation; update its signature from def
test_fm_milling_movements(self): to include the standard test return annotation
(i.e., -> None) so it reads def test_fm_milling_movements(self) -> None: to
comply with the repository's typing guidelines.
🪄 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: 32ba78ab-5098-4718-80dd-85e697ef3d12
⛔ 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 (10)
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/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/panel_tab_cryosecom_chamber.xrcutil/groom-img.py
✅ Files skipped from review due to trivial changes (1)
- install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-full-sim.odm.yaml
- src/odemis/gui/cont/tabs/cryo_chamber_tab.py
- src/odemis/gui/main_xrc.py
- src/odemis/acq/move.py
tepals
requested changes
Jun 5, 2026
tmoerkerken
force-pushed
the
fm-milling-posture
branch
from
7b97fea to
64a8904
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/odemis/acq/move.py (3)
2332-2370:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix swapped variable assignments in FM_MILLING → MILLING transform.
Lines 2340-2341 have the variable assignments backwards.
pos["rx"]contains the source FM_MILLING tilt, but it's assigned torx_millingand then used in the output (line 2367), so the transform returns the FM_MILLING tilt unchanged instead of computing the target MILLING tilt. Swap the assignments sorx_millingreceives the computed MILLING tilt andrx_fm_millingreceives the input tilt frompos["rx"].🐛 Proposed fix
stage_md = self.stage.getMetadata() # Define values that are used more than once - rx_milling = pos["rx"] - rx_fm_milling = self.get_stage_tilt_from_milling_angle() + rx_fm_milling = pos["rx"] # Input from FM_MILLING posture + rx_milling = self.get_stage_tilt_from_milling_angle() # Computed for MILLING posture ... transformed_pos = { "x": milling_reference_pos_x + (fm_milling_reference_pos_x - pos["x"]), "y": milling_reference_pos_y + (fm_milling_reference_pos_y - pos["y"]), "z": milling_reference_pos_z + (pos["z"] - fm_milling_reference_pos_z), "rx": rx_milling, # Now correctly uses the computed MILLING tilt "rz": pos["rz"], }🤖 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 2332 - 2370, In _transform_from_fm_milling_to_milling swap the two tilt assignments: assign rx_fm_milling from the incoming pos["rx"] (the source FM_MILLING tilt) and compute rx_milling via get_stage_tilt_from_milling_angle() (the target MILLING tilt) so the returned "rx" uses the computed milling tilt; update the variables rx_milling and rx_fm_milling accordingly wherever they are used in the function.
340-343:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard fm_column_tilt initialization against missing metadata.
The assignment
stage_md.get(model.MD_FAV_FM_POS_ACTIVE)["rx"]will raiseKeyErrororTypeErrorifMD_FAV_FM_POS_ACTIVEis missing or lacks the"rx"key, and this executes beforecheck_stage_metadata()validates the metadata in subclasses. Consider either deferring this initialization to subclasses after metadata validation, or adding an explicit guard that produces a clear error message.🛡️ Proposed fix
if self.pre_tilt is not None: - self.fm_column_tilt = self.pre_tilt + stage_md.get(model.MD_FAV_FM_POS_ACTIVE)["rx"] + fm_pos = stage_md.get(model.MD_FAV_FM_POS_ACTIVE) + if fm_pos and "rx" in fm_pos: + self.fm_column_tilt = self.pre_tilt + fm_pos["rx"] + else: + self.fm_column_tilt = None # Will be validated/set in subclass if needed else: self.fm_column_tilt = None🤖 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 340 - 343, The fm_column_tilt computation uses stage_md.get(model.MD_FAV_FM_POS_ACTIVE)["rx"] which can raise KeyError/TypeError before subclasses run check_stage_metadata; to fix, guard that lookup: when self.pre_tilt is not None, first retrieve fav = stage_md.get(model.MD_FAV_FM_POS_ACTIVE) and verify fav is a dict-like and contains "rx" (or raise a clear ValueError with context), then set self.fm_column_tilt = self.pre_tilt + fav["rx"]; alternatively move this initialization out of the base class into subclass code that calls check_stage_metadata() first (referencing self.pre_tilt, self.fm_column_tilt, stage_md, model.MD_FAV_FM_POS_ACTIVE and check_stage_metadata).
1957-1957:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard metadata access for fm_column_tilt.
Similar to line 341,
stage_md.get(model.MD_FAV_FM_POS_ACTIVE)["rx"]can raiseKeyErrorif the metadata dict lacks the"rx"key. Althoughcheck_stage_metadata()on line 1944 validates thatMD_FAV_FM_POS_ACTIVEexists, it doesn't verify the"rx"subkey. Add a guard or validate the complete structure.🛡️ Proposed fix
- self.fm_column_tilt = self.pre_tilt + stage_md.get(model.MD_FAV_FM_POS_ACTIVE)["rx"] + fm_pos = stage_md[model.MD_FAV_FM_POS_ACTIVE] # Validated to exist by check_stage_metadata + if "rx" not in fm_pos: + raise ValueError("Stage metadata MD_FAV_FM_POS_ACTIVE must contain 'rx' key") + self.fm_column_tilt = self.pre_tilt + fm_pos["rx"]🤖 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 1957, The assignment to self.fm_column_tilt uses stage_md.get(model.MD_FAV_FM_POS_ACTIVE)["rx"] which can KeyError if the "rx" subkey is missing; update the code in the same function where pre_tilt, stage_md and fm_column_tilt are used to first retrieve fav = stage_md.get(model.MD_FAV_FM_POS_ACTIVE) and validate that fav is a dict and contains "rx" (or use fav.get("rx") and handle None), and if missing log or raise a clear ValueError (or return a safe default) before computing self.fm_column_tilt = self.pre_tilt + rx; keep reference to check_stage_metadata to indicate existing metadata checks should be reinforced to validate the nested "rx" key as well.
🤖 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 838-854: The docstring for get_stage_tilt_from_milling_angle
incorrectly documents milling_angle and pre_tilt as parameters even though the
method only accepts column_tilt and reads milling_angle and pre_tilt from self;
update the docstring to list only column_tilt as an optional parameter (in
radians, defaulting to self.fib_column_tilt if None), note that milling angle is
taken from self.milling_angle.value and pre-tilt from self.pre_tilt, and that
the method returns the stage tilt in radians (include the equation comment if
desired for clarity).
---
Duplicate comments:
In `@src/odemis/acq/move.py`:
- Around line 2332-2370: In _transform_from_fm_milling_to_milling swap the two
tilt assignments: assign rx_fm_milling from the incoming pos["rx"] (the source
FM_MILLING tilt) and compute rx_milling via get_stage_tilt_from_milling_angle()
(the target MILLING tilt) so the returned "rx" uses the computed milling tilt;
update the variables rx_milling and rx_fm_milling accordingly wherever they are
used in the function.
- Around line 340-343: The fm_column_tilt computation uses
stage_md.get(model.MD_FAV_FM_POS_ACTIVE)["rx"] which can raise
KeyError/TypeError before subclasses run check_stage_metadata; to fix, guard
that lookup: when self.pre_tilt is not None, first retrieve fav =
stage_md.get(model.MD_FAV_FM_POS_ACTIVE) and verify fav is a dict-like and
contains "rx" (or raise a clear ValueError with context), then set
self.fm_column_tilt = self.pre_tilt + fav["rx"]; alternatively move this
initialization out of the base class into subclass code that calls
check_stage_metadata() first (referencing self.pre_tilt, self.fm_column_tilt,
stage_md, model.MD_FAV_FM_POS_ACTIVE and check_stage_metadata).
- Line 1957: The assignment to self.fm_column_tilt uses
stage_md.get(model.MD_FAV_FM_POS_ACTIVE)["rx"] which can KeyError if the "rx"
subkey is missing; update the code in the same function where pre_tilt, stage_md
and fm_column_tilt are used to first retrieve fav =
stage_md.get(model.MD_FAV_FM_POS_ACTIVE) and validate that fav is a dict and
contains "rx" (or use fav.get("rx") and handle None), and if missing log or
raise a clear ValueError (or return a safe default) before computing
self.fm_column_tilt = self.pre_tilt + rx; keep reference to check_stage_metadata
to indicate existing metadata checks should be reinforced to validate the nested
"rx" key as well.
🪄 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: 4a125e3b-634c-4a80-8361-696c47d7d627
⛔ 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 (10)
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/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/panel_tab_cryosecom_chamber.xrcutil/groom-img.py
✅ Files skipped from review due to trivial changes (1)
- util/groom-img.py
🚧 Files skipped from review as they are similar to previous changes (7)
- install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yaml
- src/odemis/acq/test/move_tfs3_test.py
- src/odemis/acq/test/move_tescan_test.py
- src/odemis/gui/xmlh/resources/panel_tab_cryosecom_chamber.xrc
- src/odemis/gui/main_xrc.py
- install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-full-sim.odm.yaml
- src/odemis/gui/cont/tabs/cryo_chamber_tab.py
tmoerkerken
force-pushed
the
fm-milling-posture
branch
from
64a8904 to
dbe4350
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/odemis/gui/cont/tabs/cryo_chamber_tab.py (2)
731-742: ⚡ Quick winConsider using
is_posture_switch_allowedfor old MIMAS.Old MIMAS unconditionally enables all posture buttons when not at
UNKNOWN(line 738), unlike METEOR's per-button approach usingposture_manager.is_posture_switch_allowed(lines 702–704). This means old MIMAS doesn't enforce FM_MILLING's transition restrictions (e.g., FM_MILLING can only switch to MILLING, GRID_1, or GRID_2). Users could attempt invalid transitions that would fail at runtime. Since old MIMAS is noted as legacy "to be replaced," this may be acceptable technical debt, but applying METEOR's pattern would improve consistency and UX.♻️ Suggested consistency refactor
- elif self._role == 'mimas': # Old mimas, not to be confused with the new mimas (to be replaced) - # enabling/disabling mimas buttons - for movement, button in self.position_btns.items(): - if current_posture == UNKNOWN: - # If at unknown position, only allow going to LOADING position - button.Enable(movement == LOADING) - else: - button.Enable(True) + elif self._role == 'mimas': # Old mimas, not to be confused with the new mimas (to be replaced) + # enabling/disabling mimas buttons + for button_posture, button in self.position_btns.items(): + switch_allowed = self.posture_manager.is_posture_switch_allowed(current_posture, button_posture) + button.Enable(switch_allowed)🤖 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/tabs/cryo_chamber_tab.py` around lines 731 - 742, The old MIMAS branch currently enables all posture buttons when current_posture != UNKNOWN (in the block using self._role == 'mimas'), which bypasses the per-button checks used by METEOR; modify that loop to call posture_manager.is_posture_switch_allowed for each movement (same pattern as lines where posture_manager.is_posture_switch_allowed is used) so each button in self.position_btns is only enabled when posture_manager.is_posture_switch_allowed(current_posture, movement) or when movement == LOADING for UNKNOWN state, ensuring FM_MILLING and other restricted transitions are enforced; keep the subsequent toggle of the current button via self._toggle_switch_buttons(btn).
229-236: ⚡ Quick winConsider adding posture filtering for old MIMAS.
Old MIMAS adds
FM_MILLINGtoposition_btnswithout the filtering logic used by METEOR (lines 212–214). IfFM_MILLINGis not supported on old MIMAS hardware, the button will still appear and be enabled (via lines 733–738), potentially causing a runtime error when clicked. Consider applying the same filtering pattern used by METEOR to remove unsupported postures from the button dict.♻️ Suggested defensive filtering
FM_MILLING: self.panel.btn_switch_fm_milling, } + # Remove the ones which are not supported on this system + self.position_btns = {posture: btn for posture, btn in self.position_btns.items() + if posture in main_data.posture_manager.postures}🤖 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/tabs/cryo_chamber_tab.py` around lines 229 - 236, The FM_MILLING entry in position_btns is added unconditionally; replicate the METEOR posture-filtering logic used elsewhere to exclude unsupported postures so the FM_MILLING button is not created/enabled on old MIMAS. Update the construction of position_btns (and/or apply a post-construction filter) to only include the FM_MILLING key when the same support check used for METEOR (e.g., self.supported_postures, self.supports_posture(...), or the existing METEOR filtering helper) indicates FM_MILLING is available; reference position_btns and panel.btn_switch_fm_milling when implementing the conditional inclusion.
🤖 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.
Nitpick comments:
In `@src/odemis/gui/cont/tabs/cryo_chamber_tab.py`:
- Around line 731-742: The old MIMAS branch currently enables all posture
buttons when current_posture != UNKNOWN (in the block using self._role ==
'mimas'), which bypasses the per-button checks used by METEOR; modify that loop
to call posture_manager.is_posture_switch_allowed for each movement (same
pattern as lines where posture_manager.is_posture_switch_allowed is used) so
each button in self.position_btns is only enabled when
posture_manager.is_posture_switch_allowed(current_posture, movement) or when
movement == LOADING for UNKNOWN state, ensuring FM_MILLING and other restricted
transitions are enforced; keep the subsequent toggle of the current button via
self._toggle_switch_buttons(btn).
- Around line 229-236: The FM_MILLING entry in position_btns is added
unconditionally; replicate the METEOR posture-filtering logic used elsewhere to
exclude unsupported postures so the FM_MILLING button is not created/enabled on
old MIMAS. Update the construction of position_btns (and/or apply a
post-construction filter) to only include the FM_MILLING key when the same
support check used for METEOR (e.g., self.supported_postures,
self.supports_posture(...), or the existing METEOR filtering helper) indicates
FM_MILLING is available; reference position_btns and panel.btn_switch_fm_milling
when implementing the conditional inclusion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a37f3ba3-fbf6-4e65-964d-98f2d26ddc3b
⛔ 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 (10)
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/gui/cont/tabs/cryo_chamber_tab.pysrc/odemis/gui/main_xrc.pysrc/odemis/gui/xmlh/resources/panel_tab_cryosecom_chamber.xrcutil/groom-img.py
✅ Files skipped from review due to trivial changes (1)
- util/groom-img.py
🚧 Files skipped from review as they are similar to previous changes (7)
- install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yaml
- install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-full-sim.odm.yaml
- install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-sim.odm.yaml
- src/odemis/gui/xmlh/resources/panel_tab_cryosecom_chamber.xrc
- src/odemis/acq/test/move_tfs3_test.py
- src/odemis/gui/main_xrc.py
- src/odemis/acq/move.py
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 support for Milling <> FM Milling posture switching for Tescan and TFS:
milling_to_fm_millingandmilling_fm_to_milling(both Tescan and TFS transforms checked by Jochem)fm_millingtransforms to facilitate sample stage computationScreencast.from.2026-06-03.14-58-36.webm
In the video we see that we are only allowed to go to the fm milling posture when currently in the milling posture. And when we are in the fm milling posture, we are only allowed to go back to the milling posture.