[MSD-548][fix] posture manager TFS3: bring back option to use old choreography#3449
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restores the previous (more direct) TFSv3 posture-switch choreography as the default to improve compatibility with SEMs that dislike the intermediate “go to low Z” step, while keeping the newer choreography available when z_low is explicitly provided in stage calibration metadata.
Changes:
- Make TFSv3 posture switching use the “Hydra Bio” low-Z choreography only when
z_lowexists inMD_CALIB; otherwise fall back to the older direct choreography. - Update the TFSv3 simulator config to omit
z_lowby default so it exercises the old choreography.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/odemis/acq/move.py |
Adds conditional branching to select between old and new posture-switch choreography based on MD_CALIB["z_low"]. |
install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yaml |
Comments out z_low so the sim uses the old choreography by default. |
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes make the TFS3 stage choreography conditional on the presence of the stage calibration field 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: 1
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)
1144-1187:⚠️ Potential issue | 🟠 MajorValidate
z_lowby value and keep the safety Z move conditional.Using key presence enables the Hydra path even for
z_low: null, and Line 1164 always moves toz_lowwhile the reverse path correctly skips it when the current Z is already at/below the threshold.Suggested fix
- z_low: float = md_calib.get("z_low") # safe z to achieve before switching to SEM posture + z_low: Optional[float] = md_calib.get("z_low") # safe z to achieve before switching to SEM posture + use_z_low = z_low is not None @@ - if "z_low" in md_calib: + if use_z_low: @@ - sub_moves.append((self.stage, {"z": z_low})) + if current_pos["z"] > z_low: + sub_moves.append((self.stage, {"z": z_low})) @@ - if "z_low" in md_calib: + if use_z_low:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/odemis/acq/move.py` around lines 1144 - 1187, md_calib may contain z_low set to None, so change the z_low checks to validate the actual value (use the earlier z_low = md_calib.get("z_low") and test "if z_low is not None") and make the safety Z move conditional like the reverse path: before appending sub_moves.append((self.stage, {"z": z_low})) in the branch handling current_label in [SEM_IMAGING, MILLING, FIB_IMAGING] -> target == FM_IMAGING, only append that move when current_pos["z"] > z_low; do the same "if z_low is not None" conditional in the opposite branch (current_label == FM_IMAGING) so both paths consistently check the numeric z_low and skip the Z move when already at/below threshold.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yaml`:
- Line 43: The TFS3 simulator config has z_low commented out which prevents
metadata-driven testing of the Hydra choreography; re-enable the z_low fixture
in meteor-tfs3-sim.odm.yaml by uncommenting or adding the "z_low" metadata key
(matching the format used in meteor-fibsem-sim.odm.yaml) so tests can exercise
the Hydra branch instead of relying on the hardcoded fallback Z_LOW in move.py;
ensure the metadata value is realistic (e.g., 27.e-3) and that any tests or
fixtures that read z_low still find and use the metadata key.
---
Outside diff comments:
In `@src/odemis/acq/move.py`:
- Around line 1144-1187: md_calib may contain z_low set to None, so change the
z_low checks to validate the actual value (use the earlier z_low =
md_calib.get("z_low") and test "if z_low is not None") and make the safety Z
move conditional like the reverse path: before appending
sub_moves.append((self.stage, {"z": z_low})) in the branch handling
current_label in [SEM_IMAGING, MILLING, FIB_IMAGING] -> target == FM_IMAGING,
only append that move when current_pos["z"] > z_low; do the same "if z_low is
not None" conditional in the opposite branch (current_label == FM_IMAGING) so
both paths consistently check the numeric z_low and skip the Z move when already
at/below threshold.
🪄 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: b5ca7be8-3aaa-4969-a35a-2509b499d273
📒 Files selected for processing (2)
install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yamlsrc/odemis/acq/move.py
pieleric
changed the title
Some SEMs don't like going to a little low Z before moving. So, to keep compatibility with the current systems, revert to use the old, more direct, choreography by default. The new choreograph can be activated by adding explicitly a z_low in the CALIB of the TFSv3 stage-bare metadata.
pieleric
force-pushed
the
fix-posture-manager-tfs3-bring-back-option-to-use-old-choreography
branch
from
9e66c77 to
1a3c491
Compare
Some SEMs don't like going to a little low Z before moving. So, to keep
compatibility with the current systems, revert to use the old, more direct, choreography by
default.
The new choreograph can be activated by adding explicitly a z_low
in the CALIB of the TFSv3 stage-bare metadata.