[MSD-237][fix] Tescan Posture Manager: FM sample stage move should depend on SEM rotation #3308
Conversation
pieleric
changed the title
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughScan-rotation compensation was added to the Tescan posture manager: _get_scan_rotation now returns 0 with info-level logging when beams are unavailable, and MeteorTescan1PostureManager._update_conversion computes a scan-rotation (sr), sets scanner rotation metadata via _set_scanner_rotation_cor(sr), and composes tf_sr with existing tilt transforms so fm/sem/milling transforms incorporate scan-rotation compensation (tf_fm becomes tf_tilt @ tf_sr). Redundant recomputation of sr was removed. Logging for unavailable e-/ion-beams was downgraded from warning to info. Tests were extended with setup scaffolding and a new test validating relative FM posture movements. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (1)
src/odemis/acq/test/move_tescan_test.py (1)
123-131: Consider addingstrict=Truetozip()for safer iteration.The static analysis tool flagged this
zip()call without an explicitstrict=parameter. While both lists currently have the same length, addingstrict=Truewould catch accidental mismatches if the test data is modified in the future.🔎 Proposed fix
- for m_sample, m_bare in zip(sample_stage_moves, stage_bare_moves): + for m_sample, m_bare in zip(sample_stage_moves, stage_bare_moves, strict=True):
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/acq/move.py(2 hunks)src/odemis/acq/test/move_tescan_test.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/odemis/acq/test/move_tescan_test.py (5)
src/odemis/acq/test/move_jeol_test.py (1)
setUpClass(49-58)src/odemis/acq/test/move_tfs1_test.py (1)
setUpClass(45-57)src/odemis/acq/test/move_util_test.py (1)
setUpClass(48-52)src/odemis/acq/move.py (2)
getCurrentPostureLabel(127-134)getCurrentPostureLabel(384-409)src/odemis/util/testing.py (1)
assert_pos_almost_equal(172-183)
src/odemis/acq/move.py (1)
src/odemis/util/transform.py (1)
get_rotation_transforms(1302-1325)
🪛 Ruff (0.14.8)
src/odemis/acq/test/move_tescan_test.py
123-123: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
⏰ 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). (4)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (python)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (3)
src/odemis/acq/test/move_tescan_test.py (1)
46-55: Good test isolation pattern.The
setUpClasscaptures initial metadata andsetUprestores it before each test, ensuring test isolation. This follows the established pattern in other test files.src/odemis/acq/move.py (2)
639-640: Appropriate logging level for expected configuration.Downgrading from
warningtoinfois correct here since missing e-beam/ion-beam is an expected configuration on older systems, not an error condition. This aligns with the PR objective of ensuring older systems continue to work correctly.
1664-1673: Scan rotation compensation looks correct.The implementation correctly:
- Retrieves scan rotation from the beams (falling back to 0° if unavailable)
- Propagates the rotation correction to scanners via
_set_scanner_rotation_cor- Creates a Z-axis rotation transform with negated angle (
rz=-sr) to compensate- Composes with tilt transform for FM imaging (
tf_fm = tf_tilt @ tf_sr)This aligns with the PR objective that FM relative moves must be inverted when a 180° scan rotation is present.
| METEOR_TESCAN1_FIBSEM_CONFIG = CONFIG_PATH + "sim/meteor-tescan-fibsem-full-sim.odm.yaml" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "METEOR_TESCAN1_FIBSEM_CONFIG" --type pyRepository: delmic/odemis
Length of output: 189
Remove unused constant or use it in a test.
METEOR_TESCAN1_FIBSEM_CONFIG is defined on line 36 but never used in this file or elsewhere in the codebase. Either remove it or incorporate it into an actual test case.
🤖 Prompt for AI Agents
In src/odemis/acq/test/move_tescan_test.py around lines 36-37, the constant
METEOR_TESCAN1_FIBSEM_CONFIG is defined but unused; either remove the constant
to eliminate dead code, or update/add a test to reference it (for example pass
it into the test fixture/setup that loads CONFIG_PATH files or use it in a
parametrized test that validates loading/parsing of that YAML). If you choose to
use it, ensure the test asserts expected behavior after loading the file and
imports CONFIG_PATH or uses the same path-building logic so the constant is
actually exercised.
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where the FM (Fluorescence Microscope) sample stage movement didn't properly account for SEM (Scanning Electron Microscope) scan rotation. The FM image is transposed to match the SEM/FIB images, so when there's a 180° scan rotation, FM relative moves need to be inverted accordingly. This fix ensures correct movement behavior on both new systems (with scan rotation) and old systems (without scan rotation).
Key changes:
- Modified the transformation matrix calculation order to apply scan rotation compensation to FM imaging mode
- Changed FM transformation from
tf_reverse @ tf_tilttotf_tilt @ tf_srto include scan rotation - Added comprehensive tests to verify FM posture relative movements match expected hardware behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/odemis/acq/move.py | Updated _update_conversion to compute scan rotation earlier and apply it to FM transformation matrix; changed logging level from warning to info for missing beam components |
| src/odemis/acq/test/move_tescan_test.py | Added test infrastructure (setUp/setUpClass) to manage metadata state between tests; added new test test_rel_move_fm_posture to validate FM relative movements with scan rotation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CONFIG_PATH = os.path.dirname(odemis.__file__) + "/../../install/linux/usr/share/odemis/" | ||
| METEOR_TESCAN1_CONFIG = CONFIG_PATH + "sim/meteor-tescan-sim.odm.yaml" | ||
|
|
||
| METEOR_TESCAN1_FIBSEM_CONFIG = CONFIG_PATH + "sim/meteor-tescan-fibsem-full-sim.odm.yaml" |
There was a problem hiding this comment.
The constant METEOR_TESCAN1_FIBSEM_CONFIG is defined but never used in this file. Consider removing it if it's not needed, or add a comment explaining why it's defined for future use.
| METEOR_TESCAN1_FIBSEM_CONFIG = CONFIG_PATH + "sim/meteor-tescan-fibsem-full-sim.odm.yaml" |
src/odemis/acq/move.py
Outdated
| rx_fm = stage_md[model.MD_FAV_FM_POS_ACTIVE]["rx"] | ||
| tf_tilt = self._get_tilt_transformation(pre_tilt, rx_fm) | ||
| tf_fm = tf_reverse @ tf_tilt | ||
| tf_fm = tf_tilt @ tf_sr # No reverse... why?! |
There was a problem hiding this comment.
The comment "No reverse... why?!" is unclear and appears to be a question about the implementation rather than documentation. This should be either removed if the implementation is correct, or expanded to explain why tf_reverse is not applied to the FM transformation (unlike SEM and MILLING which both use tf_reverse). Based on the JEOL implementation, this appears intentional but should be documented clearly.
| tf_fm = tf_tilt @ tf_sr # No reverse... why?! | |
| # For FM, the stage axes follow the Odemis convention directly, so no tf_reverse is applied here, | |
| # unlike SEM and MILLING where the Tescan stage convention is opposite and requires tf_reverse. | |
| tf_fm = tf_tilt @ tf_sr |
…EM rotation The SEM scan rotation has an indirect effect on the FM movement: the FM image is always transposed to match the SEM (and FIB) image, by adjusting the microscope file. So if there is 180° scan rotation, the FM relative moves need to be inverted. This also ensure that old systems (where there is no scan rotation) do still move in the right direction when using the new sample stage code.
pieleric
force-pushed
the
fix-tescan-posture-manager-fm-sample-stage-move-should-depend-on-sem-rotation
branch
from
b20c904 to
87ff61d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/odemis/acq/test/move_tescan_test.py (1)
105-129: Consider addingstrict=Truetozip()for safety.The test correctly validates relative FM posture movements and uses hardware-verified expected values. However, for robustness, consider adding
strict=Trueto thezip()call on line 121 (Python 3.10+) to ensure both lists have the same length.🔎 Proposed refactor
- for m_sample, m_bare in zip(sample_stage_moves, stage_bare_moves): + for m_sample, m_bare in zip(sample_stage_moves, stage_bare_moves, strict=True): old_bare_pos = self.stage.position.value self.linked_stage.moveRel(m_sample).result() new_bare_pos = self.stage.position.value
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/odemis/acq/move.py(2 hunks)src/odemis/acq/test/move_tescan_test.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/odemis/acq/test/move_tescan_test.py (5)
src/odemis/acq/test/move_jeol_test.py (1)
setUpClass(49-58)src/odemis/acq/test/move_tfs1_test.py (1)
setUpClass(45-57)src/odemis/acq/test/move_util_test.py (1)
setUpClass(48-52)src/odemis/acq/move.py (4)
cryoSwitchSamplePosition(136-151)getCurrentPostureLabel(127-134)getCurrentPostureLabel(384-409)moveRel(2473-2483)src/odemis/util/testing.py (1)
assert_pos_almost_equal(172-183)
src/odemis/acq/move.py (2)
src/odemis/util/transform.py (1)
get_rotation_transforms(1302-1325)src/odemis/model/_components.py (1)
model(570-571)
🪛 Ruff (0.14.8)
src/odemis/acq/test/move_tescan_test.py
121-121: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
⏰ 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 (6)
src/odemis/acq/test/move_tescan_test.py (3)
23-24: LGTM!The
testingmodule import is properly added to support the new test's position comparison logic.
44-53: LGTM!The setup methods properly implement test isolation by capturing and restoring stage metadata. This ensures each test starts with a clean state, even if previous tests modified the metadata.
57-60: LGTM!The clarifying comments explain important behavioral differences between posture switching (which re-reads metadata) and sample stage moves (which use cached transformations). This context is valuable for understanding test setup requirements.
src/odemis/acq/move.py (3)
639-640: LGTM!The logging level change from warning to info is appropriate. When e-beam/ion-beam components are unavailable (e.g., in simulation or older systems), this is an expected condition, not a problem. Returning 0° scan rotation as default ensures backward compatibility with systems that don't have scan rotation.
1664-1678: LGTM! Scan rotation compensation properly implemented.The changes correctly introduce scan rotation compensation for FM imaging:
- Metadata sync (line 1666): Sets MD_ROTATION_COR on scanners to ensure total image rotation is 0°
- FM transform (line 1678): Now uses
tf_tilt @ tf_srinstead oftf_reverse @ tf_tilt- Rationale (lines 1673-1677): Clear explanation that although scan rotation doesn't directly affect FM optics, the camera image is transposed to match SEM orientation, so movements must account for it
The negative rotation
-srin line 1667 correctly compensates for the scan rotation. Systems without scan rotation (sr=0°) get an identity transform, ensuring backward compatibility.
1684-1695: LGTM! Consistent scan rotation compensation across postures.The scan rotation transform
tf_sris correctly applied to both SEM (line 1684) and MILLING (line 1695) postures, ensuring consistent movement behavior across all imaging modes. The transform composition order (tf_reverse @ tf_tilt @ tf_sr) properly accounts for:
- Tescan stage convention being opposite to Odemis (
tf_reverse)- Stage tilt geometry (
tf_tilt)- Scan rotation compensation (
tf_sr)
| for axis in sem_position.keys(): | ||
| self.assertAlmostEqual(sem_position[axis], current_stage_position[axis], places=4) | ||
|
|
||
| def test_rel_move_fm_posture(self): |
There was a problem hiding this comment.
Are these tests for checking the sample stage movement with scan rotation?
If I am correct, the default the scan rotation is zero if not mentioned.
In these test cases, is it for scan rotation 0. If yes, maybe we can make test cases for 180 degree?
| # Only the other hand, although the scan rotation has no direct effect on the FM image, in | ||
| # practice, the camera image is also "transposed" to match the SEM image orientation, in the | ||
| # microscope file. So also need to apply the scan rotation here. | ||
| tf_fm = tf_tilt @ tf_sr |
There was a problem hiding this comment.
This change is applicable for both TFS and Tescan, right?
The commit message shows it applicability to only Tescan.
|
|
||
| @classmethod | ||
| def setUpClass(cls): | ||
| """Set up the test case with Tescan 1 configuration""" |
There was a problem hiding this comment.
Maybe similar test cases to check sample stage movements with and without scan rotation with 0 and 180 can be added for TFS, if applicable.
3 participants
The SEM scan rotation has an indirect effect on the FM movement: the FM
image is always transposed to match the SEM (and FIB) image, by
adjusting the microscope file. So if there is 180° scan rotation, the
FM relative moves need to be inverted.
This also ensure that old systems (where there is no scan rotation) do
still move in the right direction when using the new sample stage code.