[feature] Safe stage switching between postures in TFS #3315
Conversation
📝 WalkthroughWalkthroughAdds a new CALIB parameter Sequence Diagram(s)sequenceDiagram
participant Caller
participant PostureManager
participant StageController
participant MetadataStore
Caller->>PostureManager: request cryo-switch(targetPosture)
PostureManager->>MetadataStore: read MD_CALIB / FM/SEM positions / z_low
alt TFS3: SEM_IMAGING -> FM_IMAGING
PostureManager->>StageController: move Z -> z_low_fm
StageController-->>PostureManager: ack
PostureManager->>StageController: rotate rx,rz
StageController-->>PostureManager: ack
PostureManager->>StageController: move X,Y toward FM sample pos
StageController-->>PostureManager: ack
PostureManager->>StageController: move Z -> final FM Z
StageController-->>PostureManager: ack
PostureManager->>MetadataStore: store FM deactive/recovery positions
else TFS3: FM_IMAGING -> SEM_IMAGING/GRID
PostureManager->>StageController: optionally move Z -> z_low_fm/z_low
StageController-->>PostureManager: ack
PostureManager->>StageController: rotate rx,rz and XY to SEM pos
StageController-->>PostureManager: ack
PostureManager->>MetadataStore: restore/store SEM/FM metadata
else TFS1: SEM<->FM transition
PostureManager->>StageController: validate axes/metadata, enforce Z constraints
StageController-->>PostureManager: ack
PostureManager->>StageController: perform staged moves (Z threshold, rotations, lateral)
StageController-->>PostureManager: ack
PostureManager->>MetadataStore: update/use MD_CALIB as needed
end
PostureManager-->>Caller: completed
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: 3
🤖 Fix all issues with AI agents
In @src/odemis/acq/move.py:
- Around line 838-840: Fix the typo in the comment above the constants Z_LOW and
DELTA_Z: change "Miniumum stare bare z height required to switch between FM and
SEM postures" to "Minimum stage bare z height required to switch between FM and
SEM postures" so the comment correctly describes the constants Z_LOW and
DELTA_Z.
- Around line 1120-1126: Fix the typo in the inline comment inside the move
handling block where current_label is compared to FM_IMAGING (and target in
[SEM_IMAGING, MILLING, FIB_IMAGING]); change "assesment" to "assessment" in the
comment that precedes the sub_moves appends (the block using self.stage, z_low,
target_pos, and filter_dict).
- Around line 1095-1097: The code in the MeteorTFS3PostureManager branch
incorrectly computes z_low_fm using the global Z_LOW instead of the calibrated
z_low from md_calib and also uses type(self) == MeteorTFS3PostureManager for
type checking; change the type check to use "is" (type(self) is
MeteorTFS3PostureManager) and compute z_low_fm from the calibrated value
(z_low_fm = z_low - DELTA_Z) after obtaining z_low = md_calib.get("z_low",
Z_LOW) so the FM transition uses the calibration-aware height.
🧹 Nitpick comments (1)
src/odemis/acq/move.py (1)
1082-1093: Useisfor type comparison instead of==.Per Python style guidelines, use
isfor type identity checks rather than==. SinceMeteorTFS3PostureManagerinherits fromMeteorTFS1PostureManager, usingisinstance()would match both, so direct type comparison is appropriate here, but should useis.♻️ Suggested fix
- if (type(self) == MeteorTFS1PostureManager): + if type(self) is MeteorTFS1PostureManager: if current_label == SEM_IMAGING and target == FM_IMAGING:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yamlsrc/odemis/acq/move.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pieleric
Repo: delmic/odemis PR: 3202
File: src/odemis/acq/move.py:1158-1159
Timestamp: 2025-09-25T16:47:08.868Z
Learning: In TFS3 PostureManager coordinate transformations, using `pos` instead of `transformed_pos` for the input coordinates improves readability. The original use of `transformed_pos` was defensive programming from when `pos` could be modified mid-way through transformations.
📚 Learning: 2025-10-20T10:01:17.192Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:367-369
Timestamp: 2025-10-20T10:01:17.192Z
Learning: In src/odemis/gui/cont/features.py, when constructing Target objects for sample coordinates, it is intentional to use x/y from feature_sample_stage (sample-stage frame) and z from feature_focus (focus-actuator frame), even though this mixes coordinate frames. This is the expected way sample coordinates are defined in the codebase.
Applied to files:
src/odemis/acq/move.py
🧬 Code graph analysis (1)
src/odemis/acq/move.py (1)
src/odemis/driver/actuator.py (5)
updateMetadata(446-450)updateMetadata(749-754)updateMetadata(2215-2223)updateMetadata(2819-2837)updateMetadata(3318-3327)
🪛 Ruff (0.14.10)
src/odemis/acq/move.py
1082-1082: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
1095-1095: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
⏰ 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 (3)
install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yaml (1)
41-48: LGTM!The new
z_lowcalibration parameter is properly placed underCALIBand the value (28.e-3 m) aligns with theZ_LOWconstant default inmove.py. This allows per-system configuration of the safe Z height for posture switching.src/odemis/acq/move.py (2)
1037-1043: LGTM!The metadata retrieval is properly structured with
md_calibaccessed from stage metadata, which is validated during initialization for TFS3 systems.
1099-1118: Verify behavior when current Z is already belowz_low_fm.The multi-step move sequence looks correct for the standard case. However, if the stage is already at a Z position below
z_low_fm, the first sub-move would move Z upward, which could be unexpected. Consider whether this edge case needs handling or if it's acceptable behavior.
K4rishma
force-pushed
the
safer_stage_switching_tfs3
branch
2 times, most recently
from
d54f44e to
182249d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/odemis/acq/move.py:
- Around line 838-840: Fix the typo in the comment above Z_LOW and DELTA_Z:
change "stare" to "stage" in the explanatory comment ("Minimum stage bare z
height required to switch between FM and SEM postures") so the comment correctly
references the microscope stage; ensure the symbols Z_LOW and DELTA_Z remain
unchanged.
- Around line 1037-1043: The stage metadata lookup unconditionally accesses
model.MD_CALIB which can raise KeyError for TFS1 systems; change the retrieval
of md_calib to use stage_md.get(model.MD_CALIB) with a sensible default (e.g.,
None) or move the md_calib lookup inside the TFS3-specific branch where it is
used, and ensure MeteorTFS1PostureManager (which only adds model.MD_POS_COR) is
not assumed to provide MD_CALIB.
🧹 Nitpick comments (3)
src/odemis/acq/move.py (3)
1082-1094: Useisinstead of==for type comparisons.Per Python convention (and static analysis hint E721), use
isfor type identity checks:Proposed fix
- if (type(self) == MeteorTFS1PostureManager): + if type(self) is MeteorTFS1PostureManager:Additionally, consider whether this type-dispatching logic would be cleaner as an overridden method in
MeteorTFS3PostureManagerrather than branching by type in the base class. This would better follow the Open/Closed principle.
1095-1098: Useisinstead of==for type comparisons.Same issue as the TFS1 branch:
Proposed fix
- elif (type(self) == MeteorTFS3PostureManager): + elif type(self) is MeteorTFS3PostureManager:
1099-1118: Verify multi-line condition formatting and move sequence.The condition at lines 1099-1101 has unusual indentation. Consider reformatting for clarity:
Suggested formatting
- if (current_label in [SEM_IMAGING, MILLING, FIB_IMAGING] - and target == FM_IMAGING - ): + if ( + current_label in [SEM_IMAGING, MILLING, FIB_IMAGING] + and target == FM_IMAGING + ):The move sequence (z → rx/rz → x/y → final) aligns with the PR objective of safe stage switching. The intermediate move to
z_low_fmbefore rotation ensures vertical clearance.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yamlsrc/odemis/acq/move.py
🚧 Files skipped from review as they are similar to previous changes (1)
- install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-20T10:01:17.192Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:367-369
Timestamp: 2025-10-20T10:01:17.192Z
Learning: In src/odemis/gui/cont/features.py, when constructing Target objects for sample coordinates, it is intentional to use x/y from feature_sample_stage (sample-stage frame) and z from feature_focus (focus-actuator frame), even though this mixes coordinate frames. This is the expected way sample coordinates are defined in the codebase.
Applied to files:
src/odemis/acq/move.py
📚 Learning: 2026-01-12T12:37:35.155Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/acq/align/z_localization.py:29-40
Timestamp: 2026-01-12T12:37:35.155Z
Learning: Maintain the debugging pattern of importing odemis.gui.conf and exporting TIFF files within acquisition/localization loops across all odemis Python sources. Do not remove or restructure this behavior in refactors if it serves debugging consistency; document the debugging purpose with comments and ensure the behavior remains consistent across modules (e.g., acquisition/localization loops such as src/odemis/acq/align/z_localization.py).
Applied to files:
src/odemis/acq/move.py
🪛 Ruff (0.14.10)
src/odemis/acq/move.py
1082-1082: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
1095-1095: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
⏰ 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/move.py (1)
1120-1129: Add explicit rotation sequencing to FM→SEM transition, or document why it differs from SEM→FM.The FM→SEM path (lines 1120-1129) doesn't explicitly sequence rotation before x/y movement like the SEM→FM path does (lines 1115-1118). The SEM→FM comments specify: z → rx/rz → x/y → final, while FM→SEM only does: (optional z) → x/y → final with rotation deferred to the last combined move. Either sequence rotations separately before lateral moves (matching SEM→FM), or add a comment explaining why the FM→SEM direction has different safety requirements.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/odemis/acq/move.py:
- Around line 838-840: Fix the comment above the constants Z_LOW and DELTA_Z:
change the word "stare" to "stage" so the comment reads "Minimum stage bare z
height required to switch between FM and SEM postures" (leave the constants
Z_LOW and DELTA_Z unchanged).
- Around line 1041-1042: Direct indexing of stage_md with model.MD_CALIB can
raise KeyError on TFS1 systems; change the access to use
stage_md.get(model.MD_CALIB) (or otherwise guard with an 'if model.MD_CALIB in
stage_md' check) and handle the None/missing case similarly to the pattern in
MeteorPostureManager.__init__; ensure this aligns with how required_keys are set
across MeteorTFS3PostureManager and MeteorTFS1PostureManager so TFS1 systems do
not crash when MD_CALIB is absent.
- Around line 1095-1134: The MeteorTFS3PostureManager branch omits handling
transitions involving LOADING so when target==LOADING (or when leaving LOADING
toward SEM/MILLING/FIB) no stage moves are appended; update the TFS3 branch in
the block guarded by (type(self) == MeteorTFS3PostureManager) to add symmetric
handling for LOADING: compute z_low and z_low_fm as done now, and for
transitions where target == LOADING append appropriate sub_moves (move to safe z
z_low_fm if needed, move r/t via filter_dict({'rx','rz'}, target_pos), move x/y,
then final target_pos) and for transitions where current_label == LOADING and
target in [SEM_IMAGING, MILLING, FIB_IMAGING] ensure you move z to z_low if
current_pos["z"] > z_low, then append filter_dict({'x','y'}, target_pos) and
target_pos; reference MeteorTFS3PostureManager, z_low/z_low_fm, current_label,
target, current_pos, target_pos, sub_moves, and filter_dict to locate insertion
points.
🧹 Nitpick comments (1)
src/odemis/acq/move.py (1)
1082-1094: Useisinstance()for type checks instead oftype() ==.Per static analysis (E721) and Python best practices, use
isinstance()for type comparisons. SinceMeteorTFS3PostureManagerinherits fromMeteorTFS1PostureManager, you need to check the subclass first to ensure correct branching.♻️ Suggested fix
- if (type(self) == MeteorTFS1PostureManager): + if isinstance(self, MeteorTFS3PostureManager): + # TFS3 logic handled below + pass + elif isinstance(self, MeteorTFS1PostureManager): if current_label == SEM_IMAGING and target == FM_IMAGING: # NOTE: with TFS1, no distinction was made between SEM and MILL positions... self.stage.updateMetadata({model.MD_FAV_SEM_POS_ACTIVE: {'rx': current_pos['rx'], 'rz': current_pos['rz']}}) # Move translation axes, then rotational ones sub_moves.append((self.stage, filter_dict({'x', 'y', 'z'}, target_pos))) sub_moves.append((self.stage, filter_dict({'rx', 'rz'}, target_pos))) - - elif (type(self) == MeteorTFS3PostureManager):Alternatively, consider overriding
_doCryoSwitchSamplePositioninMeteorTFS3PostureManagerto avoid runtime type checking altogether, which would be a cleaner OOP approach.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yamlsrc/odemis/acq/move.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-08T12:16:19.734Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/acquisition/cryo_acq.py:789-807
Timestamp: 2026-01-08T12:16:19.734Z
Learning: In the cryo workflow, `feature.get_posture_position(FM_IMAGING)` always returns a valid position dictionary and never returns None, so no guard is required when calling this method.
Applied to files:
src/odemis/acq/move.py
📚 Learning: 2025-10-20T10:01:17.192Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:367-369
Timestamp: 2025-10-20T10:01:17.192Z
Learning: In src/odemis/gui/cont/features.py, when constructing Target objects for sample coordinates, it is intentional to use x/y from feature_sample_stage (sample-stage frame) and z from feature_focus (focus-actuator frame), even though this mixes coordinate frames. This is the expected way sample coordinates are defined in the codebase.
Applied to files:
src/odemis/acq/move.py
📚 Learning: 2026-01-12T12:37:35.155Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/acq/align/z_localization.py:29-40
Timestamp: 2026-01-12T12:37:35.155Z
Learning: Maintain the debugging pattern of importing odemis.gui.conf and exporting TIFF files within acquisition/localization loops across all odemis Python sources. Do not remove or restructure this behavior in refactors if it serves debugging consistency; document the debugging purpose with comments and ensure the behavior remains consistent across modules (e.g., acquisition/localization loops such as src/odemis/acq/align/z_localization.py).
Applied to files:
src/odemis/acq/move.py
🧬 Code graph analysis (1)
src/odemis/acq/move.py (2)
src/odemis/model/_components.py (1)
model(570-571)src/odemis/driver/actuator.py (5)
updateMetadata(446-450)updateMetadata(749-754)updateMetadata(2215-2223)updateMetadata(2819-2837)updateMetadata(3318-3327)
🪛 Ruff (0.14.10)
src/odemis/acq/move.py
1082-1082: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
1095-1095: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
⏰ 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 (3)
install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yaml (1)
41-48: LGTM! Newz_lowcalibration parameter for safe posture switching.The
z_lowvalue of 28mm aligns with the defaultZ_LOWconstant inmove.pyand provides a system-specific override for the minimum safe Z height during posture transitions.src/odemis/acq/move.py (2)
842-856: LGTM! TFS1-specific posture manager initialization.The explicit metadata validation, axis requirements check, and transformation initialization for TFS1 are well-structured. The postures list
[SEM_IMAGING, FM_IMAGING]correctly defines the supported postures for this system variant.
1115-1123: Clear and well-documented multi-step movement sequence for TFS3 SEM→FM transition.The staged movement order (z_low_fm → rx/rz → x/y → final position) follows the TFS safety assessment for Hydra Bio systems. The inline comments explain the rationale clearly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/odemis/acq/move.py:
- Around line 1037-1042: The code directly indexes stage_md[model.MD_CALIB] when
building md_calib which can raise KeyError for TFS1 systems because
MeteorTFS1PostureManager does not require MD_CALIB; change to safely access the
key (e.g., md_calib = stage_md.get(model.MD_CALIB)) and ensure subsequent code
that reads md_calib handles a None/default value so TFS1 branches proceed
without raising an exception.
🧹 Nitpick comments (1)
src/odemis/acq/move.py (1)
1082-1094: Useisinstance()for type checks and consider refactoring the class hierarchy.The static analysis correctly flags the use of
type(self) == Classcomparisons. While the exact type check works here to distinguish TFS1 from TFS3, this pattern indicates a design smell where subclass-specific logic is embedded in the parent class.Consider either:
- Override
_doCryoSwitchSamplePositioninMeteorTFS3PostureManagerto handle TFS3-specific logic- Or use
isinstance()if exact type matching is truly needed:isinstance(self, MeteorTFS1PostureManager) and not isinstance(self, MeteorTFS3PostureManager)♻️ Suggested approach using isinstance()
- if (type(self) == MeteorTFS1PostureManager): + if isinstance(self, MeteorTFS1PostureManager) and not isinstance(self, MeteorTFS3PostureManager):However, the cleaner solution would be to move the TFS3-specific logic (lines 1095-1134) into an overridden
_doCryoSwitchSamplePositionmethod inMeteorTFS3PostureManager.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/odemis/acq/move.py
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-01-08T09:01:44.840Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/main_xrc.py:11968-11974
Timestamp: 2026-01-08T09:01:44.840Z
Learning: In the file `src/odemis/gui/main_xrc.py`, the `ctrl_target_z` UnitFloatCtrl uses meters (m) as its unit for Z-axis positioning.
Applied to files:
src/odemis/acq/move.py
📚 Learning: 2025-10-20T10:01:17.192Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:367-369
Timestamp: 2025-10-20T10:01:17.192Z
Learning: In src/odemis/gui/cont/features.py, when constructing Target objects for sample coordinates, it is intentional to use x/y from feature_sample_stage (sample-stage frame) and z from feature_focus (focus-actuator frame), even though this mixes coordinate frames. This is the expected way sample coordinates are defined in the codebase.
Applied to files:
src/odemis/acq/move.py
📚 Learning: 2026-01-09T15:17:06.048Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/acquisition/cryo_z_localization.py:164-198
Timestamp: 2026-01-09T15:17:06.048Z
Learning: In src/odemis/gui/cont/acquisition/cryo_z_localization.py, the cmb_targets combobox intentionally only lists TargetType.Fiducial targets, not POIs. POI and feature share values and are managed separately from the fiducial list. The currentTarget may be set to a POI without it appearing in the combobox, which is the intended design.
Applied to files:
src/odemis/acq/move.py
📚 Learning: 2025-10-20T10:02:02.606Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:360-379
Timestamp: 2025-10-20T10:02:02.606Z
Learning: In the cryo workflow, POIs (Points of Interest) are linked to features and should always be derived from the latest feature position and focus values when loading correlation data, rather than using any saved POI values from correlation_data.fm_pois. This ensures POIs reflect the current feature state.
Applied to files:
src/odemis/acq/move.py
📚 Learning: 2026-01-12T12:37:35.155Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/acq/align/z_localization.py:29-40
Timestamp: 2026-01-12T12:37:35.155Z
Learning: Maintain the debugging pattern of importing odemis.gui.conf and exporting TIFF files within acquisition/localization loops across all odemis Python sources. Do not remove or restructure this behavior in refactors if it serves debugging consistency; document the debugging purpose with comments and ensure the behavior remains consistent across modules (e.g., acquisition/localization loops such as src/odemis/acq/align/z_localization.py).
Applied to files:
src/odemis/acq/move.py
🧬 Code graph analysis (1)
src/odemis/acq/move.py (1)
src/odemis/driver/actuator.py (5)
updateMetadata(446-450)updateMetadata(749-754)updateMetadata(2215-2223)updateMetadata(2819-2837)updateMetadata(3318-3327)
🪛 Ruff (0.14.10)
src/odemis/acq/move.py
1082-1082: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
1095-1095: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
⏰ 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-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (5)
src/odemis/acq/move.py (5)
838-840: LGTM! Well-documented safety constants.The constants are appropriately defined with a clear comment explaining the purpose and origin (TFS Hydra Bio system). The fallback pattern
md_calib.get("z_low", Z_LOW)used later allows system-specific overrides while maintaining safe defaults.
842-855: LGTM! Proper initialization with validation.The initialization correctly validates required metadata and axes, retrieves the pre_tilt from the linked component, and initializes the transformation. The error message clearly indicates which axes are missing.
1107-1114: Z position recovery logic is well-designed.The approach of storing the Z position in sample-stage coordinates (via
MD_FM_POS_SAMPLE_DEACTIVE) for later recovery when returning to SEM is sound. Usingfix_fm_plane=Falseensures the raw transformed position is stored without the fixed plane correction.Note: This relies on the TFS3-specific
fix_fm_planeparameter in_transformFromSEMToMeteor. The type check at line 1095 ensures this parameter is only used whenselfis exactlyMeteorTFS3PostureManager.
1160-1182: TFS3 initialization looks correct.The initialization properly:
- Calls
MeteorPostureManager.__init__directly (bypassing TFS1's init)- Validates TFS3-specific calibration requirements
- Sets up the sample stage and FM position defaults
The class inherits from TFS1 for code reuse but configures itself independently, which is appropriate for this architecture.
1095-1134: The Z-safety logic is correct, no changes needed.The z_low boundary condition at line 1131 correctly uses
>(not>=) to check if the current Z is higher than the safe value. If z equals z_low exactly, no preliminary Z move occurs, which is intentional since the stage is already at a safe height. This matches the documented requirement: "If current z is more than safe value, move it to safe z value else skip it."The z_low_fm calculation properly handles the edge case where z_low is 0, applying an additional DELTA_Z safety margin only when z_low is non-zero. The staged movement sequence (z → rotate → translate → final for SEM→FM, and conditional z → translate → final for FM→SEM) is well-designed and safe.
K4rishma
force-pushed
the
safer_stage_switching_tfs3
branch
from
f0d0d27 to
583588c
Compare
Move to safe z value when moving between postures
K4rishma
force-pushed
the
safer_stage_switching_tfs3
branch
from
583588c to
0255e7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/odemis/acq/move.py (1)
1080-1092: Consider using method override instead of type checking.The
type(self) == ClassNamepattern is intentional to differentiate TFS1 from TFS3, but it creates a design smell where the base class has explicit knowledge of its derived class. This violates the Open/Closed Principle.Consider having
MeteorTFS3PostureManageroverride_doCryoSwitchSamplePositioninstead, or extract the posture-specific move sequencing into a protected method that each subclass can override:# In base class (TFS1) def _get_posture_switch_moves(self, current_label, target, current_pos, target_pos): """Returns sub_moves list for posture switching. Override in subclasses.""" sub_moves = [] # TFS1 specific logic... return sub_movesThis would also address the static analysis hint (E721) about using
isinstance()for type comparisons.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yamlsrc/odemis/acq/move.py
🚧 Files skipped from review as they are similar to previous changes (1)
- install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yaml
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:367-369
Timestamp: 2025-10-20T10:01:17.192Z
Learning: In src/odemis/gui/cont/features.py, when constructing Target objects for sample coordinates, it is intentional to use x/y from feature_sample_stage (sample-stage frame) and z from feature_focus (focus-actuator frame), even though this mixes coordinate frames. This is the expected way sample coordinates are defined in the codebase.
📚 Learning: 2025-10-20T10:01:17.192Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:367-369
Timestamp: 2025-10-20T10:01:17.192Z
Learning: In src/odemis/gui/cont/features.py, when constructing Target objects for sample coordinates, it is intentional to use x/y from feature_sample_stage (sample-stage frame) and z from feature_focus (focus-actuator frame), even though this mixes coordinate frames. This is the expected way sample coordinates are defined in the codebase.
Applied to files:
src/odemis/acq/move.py
📚 Learning: 2026-01-09T15:17:06.048Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/acquisition/cryo_z_localization.py:164-198
Timestamp: 2026-01-09T15:17:06.048Z
Learning: In src/odemis/gui/cont/acquisition/cryo_z_localization.py, the cmb_targets combobox intentionally only lists TargetType.Fiducial targets, not POIs. POI and feature share values and are managed separately from the fiducial list. The currentTarget may be set to a POI without it appearing in the combobox, which is the intended design.
Applied to files:
src/odemis/acq/move.py
📚 Learning: 2026-01-08T09:01:44.840Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/main_xrc.py:11968-11974
Timestamp: 2026-01-08T09:01:44.840Z
Learning: In the file `src/odemis/gui/main_xrc.py`, the `ctrl_target_z` UnitFloatCtrl uses meters (m) as its unit for Z-axis positioning.
Applied to files:
src/odemis/acq/move.py
📚 Learning: 2025-10-20T10:02:02.606Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/features.py:360-379
Timestamp: 2025-10-20T10:02:02.606Z
Learning: In the cryo workflow, POIs (Points of Interest) are linked to features and should always be derived from the latest feature position and focus values when loading correlation data, rather than using any saved POI values from correlation_data.fm_pois. This ensures POIs reflect the current feature state.
Applied to files:
src/odemis/acq/move.py
📚 Learning: 2026-01-12T12:37:35.155Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/acq/align/z_localization.py:29-40
Timestamp: 2026-01-12T12:37:35.155Z
Learning: Maintain the debugging pattern of importing odemis.gui.conf and exporting TIFF files within acquisition/localization loops across all odemis Python sources. Do not remove or restructure this behavior in refactors if it serves debugging consistency; document the debugging purpose with comments and ensure the behavior remains consistent across modules (e.g., acquisition/localization loops such as src/odemis/acq/align/z_localization.py).
Applied to files:
src/odemis/acq/move.py
📚 Learning: 2026-01-08T12:16:19.734Z
Learnt from: K4rishma
Repo: delmic/odemis PR: 3245
File: src/odemis/gui/cont/acquisition/cryo_acq.py:789-807
Timestamp: 2026-01-08T12:16:19.734Z
Learning: In the cryo workflow, `feature.get_posture_position(FM_IMAGING)` always returns a valid position dictionary and never returns None, so no guard is required when calling this method.
Applied to files:
src/odemis/acq/move.py
🪛 Ruff (0.14.10)
src/odemis/acq/move.py
1080-1080: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
1093-1093: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
⏰ 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-24.04)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (2)
src/odemis/acq/move.py (2)
838-840: LGTM!Clear documentation and sensible default values for the safe Z thresholds. The constants follow the existing naming conventions and unit standards (meters) used in the codebase.
1093-1134: Z-threshold sequencing logic is correct for safe posture switching.The multi-step movement sequence with Z-threshold checks correctly implements the safety requirements:
- SEM→FM: Lower to
z_low_fm(z_low - 1mm = 27mm) first, then rotate, then translate- FM→SEM: Conditionally lower to
z_low(28mm), translate, then final positionThe asymmetry between thresholds (z_low_fm for SEM→FM vs z_low for FM→SEM) is intentional—SEM→FM applies an extra safety margin per Hydra Bio TFS assessment, while FM→SEM follows Hydra Bio xt assessment. This differentiation is properly documented in the comments and matches the staged movement strategy for each direction.
src/odemis/acq/move.py
Outdated
| sub_moves.append((self.stage, {"z": z_low_fm})) | ||
| sub_moves.append((self.stage, filter_dict({'rx', 'rz'}, target_pos))) | ||
| sub_moves.append((self.stage, filter_dict({'x', 'y'}, target_pos))) | ||
| sub_moves.append((self.stage, filter_dict({'x', 'y', 'z'}, target_pos))) |
There was a problem hiding this comment.
Any reason you repeat x & y axes? Assuming the previous move has succeeded, it should be fine to move only z. Or do you have in mind some way to handle imprecision or incorrect move? In such case, please add a comment to explain.
Also, the TFS recommendation is different:
- move rx rz
- move x y
- move everything (again) at once
src/odemis/acq/move.py
Outdated
|
|
||
| elif (current_label == FM_IMAGING | ||
| and target in [SEM_IMAGING, MILLING, FIB_IMAGING]): | ||
| # Stage switched based on Hydra Bio xt assesment |
There was a problem hiding this comment.
As it's based on the same assessment as the code just above, it'd be clearer to write it down just once. I'd suggest to place this remark just above the z_low computation.
src/odemis/acq/move.py
Outdated
| sub_moves.append((self.stage, {"z": z_low})) | ||
| sub_moves.append((self.stage, filter_dict({'x', 'y'}, target_pos))) | ||
| sub_moves.append((self.stage, filter_dict({'rx', 'rz'}, target_pos))) | ||
| sub_moves.append((self.stage, filter_dict({'x', 'y', 'z'}, target_pos))) |
There was a problem hiding this comment.
Same comment here: why moving x & y twice to the same position?
|
|
||
|
|
||
| # Minimum stage bare z height required to switch between FM and SEM postures | ||
| Z_LOW = 0.028 # m, safe value provided by TFS on a Hydra Bio system |
There was a problem hiding this comment.
Hydra Bio -> Hydra Bio CX system, which should be also compatible on other systems
2 participants
Move to safe z value when moving between postures
To be Tested on Hardware - Opening as a draft PR