[METEOR-1481] [feat] Add METEOR support for JEOL JIB-4700F#3248
Conversation
Provided by Damien McGrouther on behalf of JEOL.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the JEOL JIB-4700F microscope in the METEOR system by implementing the MeteorJeol1PostureManager class and providing a complete simulator configuration.
Key Changes:
- Implements
MeteorJeol1PostureManagerwith JEOL-specific stage transformation logic between SEM and FM imaging positions - Adds simulator configuration file
meteor-jeol.odm.yamlwith hardware component definitions and calibration parameters
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/odemis/acq/move.py | Adds MeteorJeol1PostureManager class with JEOL-specific stage transformations and posture switching logic |
| install/linux/usr/share/odemis/sim/meteor-jeol.odm.yaml | Provides complete simulator configuration for JEOL JIB-4700F including stage, camera, light source, and optical components |
Comments suppressed due to low confidence (1)
src/odemis/acq/move.py:1
- The comment references 'tescan' but this method is in the
MeteorJeol1PostureManagerclass. Update the comment to reference JEOL instead of Tescan.
# -*- coding: utf-8 -*-
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds JEOL1 support by introducing MeteorJeol1PostureManager, a SampleStage wrapper, and JEOL-specific SEM⇄Meteor coordinate transformations and posture handling. Wires the new posture manager into the MicroscopePostureManager factory for stage_version "jeol_1". Includes a simulator YAML config for METEOR JEOL hardware and a unit test suite (TestMeteorJeol1Move) validating posture switches, grid/area target computations, and the SEM↔Meteor transform behavior. Sequence Diagram(s)sequenceDiagram
participant User
participant PM as MeteorJeol1PostureManager
participant SS as SampleStage
participant Stage
User->>PM: request posture switch (e.g., FM_IMAGING)
activate PM
PM->>PM: getTargetPosition(target_label)
Note over PM: compute target from SEM grid centers\nor use current stage-bare position
PM->>PM: _transformFromSEMToMeteor(pos)
Note over PM: apply tilt corrections\nmap SEM (x,y,z,rx,rz) → Meteor/FM axes
PM->>SS: update sample-stage coordinates
PM->>Stage: issue sub-move to stage (returns Future)
Stage-->>PM: future handle
PM-->>User: posture switch future
deactivate PM
%% Transformation examples
User->>PM: transform SEM→Meteor (sem_pos)
PM->>PM: _transformFromSEMToMeteor(sem_pos)
PM-->>User: meteor_pos
User->>PM: transform Meteor→SEM (meteor_pos)
PM->>PM: _transformFromMeteorToSEM(meteor_pos)
PM-->>User: sem_pos
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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: 2
🧹 Nitpick comments (2)
src/odemis/acq/move.py (2)
2017-2034: Consider parking focus before stage moves (safety consistency)Other posture managers park the focus before large stage moves to avoid collisions. Even if JEOL software adds interlocks, parking reduces risk and aligns behavior.
Example:
@@ - # Move stage directly (the JEOL software takes care of the safety aspects) + # Optionally park focus for safety consistency (like other vendors) + try: + focus = model.getComponent(role='focus') + focus_md = focus.getMetadata() + focus_deactive = focus_md[model.MD_FAV_POS_DEACTIVE] + if not isNearPosition(focus.position.value, focus_deactive, focus.axes): + self._run_sub_move(future, focus, focus_deactive) + except Exception: + logging.debug("Focus component not available; continuing without parking.") + + # Move stage (JEOL software provides additional safeties) target_position = self.getTargetPosition(target_posture) self._run_sub_move(future, self.stage, target_position)
2035-2044: Docstring mentions Tescan in JEOL methodThis JEOL-specific method says “For tescan…”, which is misleading. Replace with a JEOL-specific note that the method is unused and intentionally unimplemented.
- """Transform the shift from stage bare to chamber coordinates. - Used for moving the stage vertically in the chamber. - For tescan, the z-axis is already aligned with the chamber axis, - so this function returns the input. + """JEOL: chamber-vertical transform not used. + Left unimplemented intentionally for JEOL1 configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
install/linux/usr/share/odemis/sim/meteor-jeol.odm.yaml(1 hunks)src/odemis/acq/move.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/odemis/acq/move.py (1)
src/odemis/driver/actuator.py (1)
axes(3130-3132)
🪛 Ruff (0.14.1)
src/odemis/acq/move.py
1878-1878: Avoid specifying long messages outside the exception class
(TRY003)
1928-1930: Avoid specifying long messages outside the exception class
(TRY003)
1942-1942: Avoid specifying long messages outside the exception class
(TRY003)
2024-2024: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (3)
src/odemis/acq/move.py (2)
98-99: Factory dispatch: JEOL1 wired correctlySelecting MeteorJeol1PostureManager when CALIB.version == "jeol_1" looks correct and keeps behavior consistent with other vendors.
1934-2016: No critical issues found; recommendation stands but cannot be fully verified in sandboxThe transform methods are newly implemented for JEOL systems and follow the same mathematical pattern as the tested Tescan implementation (which validates bidirectional consistency in
test_switching_consistencywith places=4 tolerance). Mathematical symmetry is correct: forward/reverse operations properly invert signs for dx/dy/dz, and tilt corrections are consistently inverted.However, no JEOL-specific fixture tests exist. Your recommendation to validate with JEOL calibration constants and grid center ranges is sound—the Tescan equivalent has such tests. Since the odemis simulator is unavailable in this environment, manual verification using the fixture script (or creating a JEOL-specific test class mirroring
TestMeteorTescan1Move.test_switching_consistency) would confirm grid positions map correctly between SEM/FM imaging areas.install/linux/usr/share/odemis/sim/meteor-jeol.odm.yaml (1)
50-58: Version tag aligns with factory dispatchCALIB.version "jeol_1" correctly triggers MeteorJeol1PostureManager selection. LGTM.
pieleric
changed the title
K4rishma
left a comment
K4rishma
left a comment
There was a problem hiding this comment.
Should we add test cases for this driver in move_test.py ?
pieleric
force-pushed
the
feat-add-meteor-support-for-jeol-jib-4700f
branch
from
6cc58ba to
c07ab88
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
install/linux/usr/share/odemis/sim/meteor-jeol-sim.odm.yaml (2)
77-77: FAV_POS_DEACTIVE.z violates stage range constraints.The FAV_POS_DEACTIVE position has
z: 0.0, which is outside the valid stage z range (either [0, 41.5e-3] or [4e-3, 41.5e-3] depending on line 38). If the minimum is 4e-3 as suggested by SEM_IMAGING_RANGE (line 70), then this loading position is invalid and could cause movement failures or detection issues.Based on learnings from past review comments.
Apply this diff to place the loading position within valid range:
- FAV_POS_DEACTIVE: {"x": 0.0, "y": 0.0, "z": 0.0, "rx": 0.0, "rz": 0.0 } + FAV_POS_DEACTIVE: {"x": 0.0, "y": 0.0, "z": 8.5e-3, "rx": 0.0, "rz": 0.0 }Using
8.5e-3(the z_0 reference value) is recommended as it matches the GRID centers and provides a safe loading height.
33-41: Stage z range minimum inconsistency.The stage z range on line 38 starts at
0.e-3, but the SEM_IMAGING_RANGE on line 70 starts at4.e-3. This inconsistency should be resolved. Based on the context and the SEM imaging requirements, the stage z minimum should likely be4.e-3.Apply this diff to correct the stage z range:
- "z": [0.e-3, 41.5e-3], # m + "z": [4.e-3, 41.5e-3], # msrc/odemis/acq/move.py (1)
1911-1913: Useelifinstead of secondiffor clearer control flow.The second
ifon line 1913 should beelifto make it clear that these are mutually exclusive branches. While the current logic happens to work correctly (because whentarget_pos_lbl == LOADING, none of the nested conditions on lines 1914-1923 will match), usingelifmakes the intent explicit and prevents potential issues if the code is modified later.Apply this diff:
if target_pos_lbl == LOADING: end_pos = stage_md[model.MD_FAV_POS_DEACTIVE] - if current_position in [LOADING, SEM_IMAGING]: + elif current_position in [LOADING, SEM_IMAGING]: if target_pos_lbl in [SEM_IMAGING, GRID_1]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
install/linux/usr/share/odemis/sim/meteor-jeol-sim.odm.yaml(1 hunks)src/odemis/acq/move.py(2 hunks)src/odemis/acq/test/move_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/odemis/acq/move.py (1)
src/odemis/driver/actuator.py (1)
axes(3130-3132)
src/odemis/acq/test/move_test.py (1)
src/odemis/acq/move.py (11)
MicroscopePostureManager(81-291)MeteorPostureManager(294-779)getCurrentPostureLabel(110-117)getCurrentPostureLabel(361-386)getCurrentGridLabel(388-422)_transformFromSEMToMeteor(483-491)_transformFromSEMToMeteor(875-915)_transformFromSEMToMeteor(1112-1142)_transformFromSEMToMeteor(1337-1386)_transformFromSEMToMeteor(1644-1691)_transformFromSEMToMeteor(1939-1980)
🪛 Ruff (0.14.1)
src/odemis/acq/move.py
1878-1878: Avoid specifying long messages outside the exception class
(TRY003)
1933-1935: Avoid specifying long messages outside the exception class
(TRY003)
1947-1947: Avoid specifying long messages outside the exception class
(TRY003)
2029-2029: Avoid specifying long messages outside the exception class
(TRY003)
src/odemis/acq/test/move_test.py
407-407: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
676-676: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (3)
- GitHub Check: lint
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (5)
src/odemis/acq/move.py (5)
98-99: LGTM! Factory wiring correctly implemented.The JEOL 1 stage version is properly wired to instantiate
MeteorJeol1PostureManager, following the established pattern for other stage versions.
1939-1980: LGTM! Tilt-based transformation correctly implemented.The SEM-to-Meteor transformation properly accounts for tilt-dependent corrections using the z_offset and the difference in tilt angles between SEM and FM postures. The math follows the established pattern from other METEOR systems (similar to Tescan).
1982-2020: LGTM! Reverse transformation correctly implemented.The Meteor-to-SEM transformation correctly reverses the forward transformation, applying the inverse tilt corrections. The symmetry with
_transformFromSEMToMeteoris correct.
2022-2038: LGTM! Simple and appropriate delegation to JEOL software.The implementation correctly delegates safety handling to the JEOL control software by performing a direct stage move. This is an appropriate design choice for JEOL systems where the external controller manages collision avoidance and movement safety.
1888-1892: LGTM! Sample stage creation follows TFS3 pattern.The
create_sample_stagemethod correctly instantiates the SampleStage wrapper, consistent with the TFS3 implementation. This enables proper coordinate transformations for the sample plane.
Yes! I've just added some now. |
pieleric
force-pushed
the
feat-add-meteor-support-for-jeol-jib-4700f
branch
from
c07ab88 to
7622f76
Compare
Make sure the target position that SwitchSamplePosition() goes to is the same as reported by getTargetPosition(). Especially, in FM IMAGING mode, if the user switched GRID, it would go to SEM IMAGING. Make use of the sample stage. So the microscope file can be simplified.
pieleric
force-pushed
the
feat-add-meteor-support-for-jeol-jib-4700f
branch
from
7622f76 to
533d35b
Compare
Provided by Damien McGrouther on behalf of JEOL.
Some adjustments to fix some small issues, and get to use the new SampleStage functionality.