[MSD_219][fix] METEOR: rename MD_FAV_MILL_POS_ACTIVE["rx"] -> ["mill_angle"] #3309
Conversation
"rx" is too easy to confuse with the stage-bare tilt. So rename the key to "mill_angle", and add a "upgrade" code that automatically updates the metadata of the stage-bare if a backend was started with an old configuration.
pieleric
changed the title
📝 WalkthroughWalkthroughThis pull request renames the milling angle field key from 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: 0
🧹 Nitpick comments (1)
src/odemis/gui/cont/tabs/fibsem_tab.py (1)
190-191: Consider renaming variable for clarity.The code correctly reads "mill_angle" from metadata, but stores it in a variable named
rx, which is misleading. Consider renaming tomill_anglefor better code clarity:- rx = self.pm.stage.getMetadata()[model.MD_FAV_MILL_POS_ACTIVE]["mill_angle"] - self.panel.ctrl_milling_angle.SetValue(math.degrees(rx)) + mill_angle = self.pm.stage.getMetadata()[model.MD_FAV_MILL_POS_ACTIVE]["mill_angle"] + self.panel.ctrl_milling_angle.SetValue(math.degrees(mill_angle))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yaml(1 hunks)install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-full-sim.odm.yaml(1 hunks)install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-sim.odm.yaml(1 hunks)install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yaml(1 hunks)src/odemis/acq/move.py(6 hunks)src/odemis/gui/cont/tabs/fibsem_tab.py(2 hunks)
🧰 Additional context used
🧬 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.8)
src/odemis/acq/move.py
352-353: 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). (4)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (python)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (11)
src/odemis/acq/move.py (6)
342-356: LGTM! Well-designed upgrade path for backward compatibility.The automatic conversion from "rx" to "mill_angle" ensures smooth upgrades for existing installations. The check for conflicting keys (both "rx" and "mill_angle") is a good safeguard.
506-511: LGTM! Correct usage of mill_angle for posture orientation.The code properly reads "mill_angle" from metadata and converts it to stage tilt "rx" for the MILLING posture orientation.
1699-1708: LGTM! Transformation matrices correctly use mill_angle.The milling transformation matrix computation now properly reads "mill_angle" from metadata.
1916-1923: LGTM! Correct transformation logic using mill_angle.The code properly pops "mill_angle" from the metadata, converts it to stage tilt "rx", and updates the position accordingly.
1964-1968: LGTM! Reverse transformation correctly uses mill_angle.The code properly reads "mill_angle" from metadata for the milling-to-SEM transformation.
2060-2068: LGTM! Proper persistence of milling angle when leaving MILLING posture.The code correctly calculates the milling angle from the current stage tilt and updates the metadata to preserve the user's setting.
src/odemis/gui/cont/tabs/fibsem_tab.py (1)
410-418: LGTM! Correct metadata update using mill_angle.The code properly writes "mill_angle" to the metadata while preserving the "rz" value, consistent with the backend changes.
install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yaml (1)
51-51: LGTM! Configuration correctly updated to use mill_angle.The FAV_MILL_POS_ACTIVE key has been properly renamed from "rx" to "mill_angle", with the numeric value and "rz" preserved. The comment accurately describes mill_angle as the angle between the ion beam column and sample plane.
install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-sim.odm.yaml (1)
137-138: LGTM! Configuration correctly updated to use mill_angle.The FAV_MILL_POS_ACTIVE key has been properly renamed from "rx" to "mill_angle", preserving both the numeric value and "rz". The comment explaining the tilt angle calculation remains accurate.
install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yaml (1)
96-96: LGTM! Configuration correctly updated to use mill_angle.The FAV_MILL_POS_ACTIVE key has been properly renamed from "rx" to "mill_angle", with the numeric value (14°) and "rz" preserved. The comment correctly describes it as the milling angle.
install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-full-sim.odm.yaml (1)
137-138: LGTM! Configuration correctly updated to use mill_angle.The FAV_MILL_POS_ACTIVE key has been properly renamed from "rx" to "mill_angle", preserving the milling angle value (10°) and "rz" rotation. The comment provides helpful context about the tilt angle calculation.
There was a problem hiding this comment.
Pull request overview
This PR renames the key "rx" to "mill_angle" in the MD_FAV_MILL_POS_ACTIVE metadata to improve clarity and avoid confusion with the stage tilt angle. The change includes an automatic upgrade path that converts old configurations to the new format when the backend starts.
Key changes:
- Added automatic upgrade code in
MeteorPostureManager.__init__()to convert "rx" to "mill_angle" in existing metadata - Updated all references to use "mill_angle" instead of "rx" throughout the codebase
- Updated configuration files to use the new key format
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/odemis/acq/move.py | Added upgrade path logic to automatically convert old "rx" key to "mill_angle", and updated all code references to use the new key name |
| src/odemis/gui/cont/tabs/fibsem_tab.py | Updated GUI code to read and write "mill_angle" instead of "rx" in the milling metadata |
| install/linux/usr/share/odemis/sim/meteor-tfs3-sim.odm.yaml | Updated configuration to use "mill_angle" key and improved comment clarity |
| install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-sim.odm.yaml | Updated configuration to use "mill_angle" key and removed outdated comment |
| install/linux/usr/share/odemis/sim/meteor-tescan-fibsem-full-sim.odm.yaml | Updated configuration to use "mill_angle" key and removed outdated comment |
| install/linux/usr/share/odemis/sim/meteor-fibsem-sim.odm.yaml | Updated configuration to use "mill_angle" key and improved comment clarity |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mill_angle = mill_pos_active.pop("rx") | ||
| mill_angle = mill_pos_active.pop("mill_angle") | ||
| rx_mill = calculate_stage_tilt_from_milling_angle(mill_angle, pre_tilt=self.pre_tilt, column_tilt=self.fib_column_tilt) | ||
| mill_pos_active["rx"] = rx_mill # update the computed rx based on the milling angle, see the note above |
There was a problem hiding this comment.
The comment "see the note above" is now outdated. The note that was previously above these lines (which explained that "rx" referred to the milling angle) has been removed in this PR. This comment should either be removed or updated to accurately reflect what the code is doing (computing the actual stage rx from the milling angle).
| mill_pos_active["rx"] = rx_mill # update the computed rx based on the milling angle, see the note above | |
| mill_pos_active["rx"] = rx_mill # update the stage rx (tilt) based on the computed milling-angle tilt |
| # From Odemis v3.8 onwards, the "mill_angle" key is used instead. | ||
| # As it should never have a "rx" angle defined, we automatically convert it here, for the | ||
| # running back-end. |
There was a problem hiding this comment.
The comment "As it should never have a "rx" angle defined" is confusing. The code is specifically checking for and handling the case where "rx" IS defined (from old configurations). Consider rephrasing to clarify that this refers to the stage-bare metadata in the new format, e.g., "As the stage-bare should use 'mill_angle' instead of 'rx' for the milling angle".
| # From Odemis v3.8 onwards, the "mill_angle" key is used instead. | |
| # As it should never have a "rx" angle defined, we automatically convert it here, for the | |
| # running back-end. | |
| # From Odemis v3.8 onwards, the "mill_angle" key is used instead for stage-bare metadata. | |
| # As the stage-bare metadata in the new format should use "mill_angle" instead of "rx" for | |
| # the milling angle, we automatically convert any legacy "rx"-based configuration here for | |
| # the running back-end. |
3 participants
"rx" is too easy to confuse with the stage-bare tilt. So rename the key
to "mill_angle", and add a "upgrade" code that automatically updates the
metadata of the stage-bare if a backend was started with an old
configuration.