[MSD-40][refactor] Milling angle as VA with automatic sample stage matrix recalculation #3313
Conversation
📝 WalkthroughWalkthroughMILLING_RANGE was converted from degrees to radians. Posture managers expose a public milling_angle FloatContinuous sourced from stage metadata key MD_FAV_MILL_POS_ACTIVE["mill_angle"] (default 0) and persist changes via a new _set_milling_angle that updates metadata while preserving rz. Transformation initialization and update APIs were simplified to _initialise_transformation(angle) and _update_conversion(pre_tilt), and milling_angle.subscribe(..., init=True) wires metadata changes into transformation setup. to_posture avoids the fast-path when target posture is MILLING. FibsemTab sets milling_angle through the posture manager and initiates asynchronous posture switches tracked by a future; tests updated to validate milling-angle stability. Sequence Diagram(s)sequenceDiagram
participant UI as FibsemTab UI
participant PM as PostureManager
participant Stage as Stage Metadata
participant Transform as Transformation System
UI->>PM: set milling_angle.value (rad)
activate PM
PM->>Stage: write MD_FAV_MILL_POS_ACTIVE (set mill_angle, preserve rz)
PM->>PM: publish milling_angle change
PM->>Transform: _initialise_transformation(angle)
activate Transform
Transform->>Transform: recompute axes/tilt using angle and linked_axes
deactivate Transform
PM-->>UI: milling_angle change notification
deactivate PM
UI->>UI: refresh features / UI
sequenceDiagram
participant User as User
participant UI as FibsemTab UI
participant Driver as Stage Driver
participant PM as PostureManager
User->>UI: click move-to-milling-posture
UI->>Driver: cryoSwitchSamplePosition(MILLING) (returns Future)
UI->>UI: store future in _posture_switch_future
UI->>UI: attach completion callback (_on_move_complete)
activate Driver
Driver->>Driver: perform async posture switch
Driver-->>UI: future completes
deactivate Driver
UI->>UI: _on_move_complete(future)
alt success
UI->>Driver: refresh stage position
UI->>UI: update buttons / layout
else cancelled / error
UI->>UI: log error / refresh as needed
end
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: 2
🧹 Nitpick comments (3)
src/odemis/acq/test/move_tescan_test.py (1)
126-156: Clarify the commented-out code or remove it.The test methodology is sound and properly validates milling angle stability. However, lines 129-132 contain commented-out code for setting a default milling angle.
Either uncomment this code if it's necessary for the test setup, or remove it entirely to keep the test clean.
🔎 Suggested cleanup
If the commented code is not needed:
def test_milling_angle_stable_pos(self): sample_stage = self.posture_manager.sample_stage - # Set default milling angle - # milling_angle = math.radians(15) - # current_md = self.stage.getMetadata() - # self.stage.updateMetadata({model.MD_FAV_MILL_POS_ACTIVE: {'rx': milling_angle, - # "rz": current_md[model.MD_FAV_MILL_POS_ACTIVE]["rz"]}}) # Transform to milling posture self.posture_manager.cryoSwitchSamplePosition(MILLING).result()src/odemis/gui/cont/tabs/fibsem_tab.py (1)
416-454: Remove commented-out code, otherwise LGTM.The logic correctly:
- Uses the
milling_angleVA setter to trigger transformation updates- Updates existing feature positions with the new milling angle
- Automatically re-applies the milling posture when the angle changes
However, lines 432-439 contain commented-out code about user confirmation for updating features. Since the decision was made to always update (line 440), this commented code should be removed for clarity.
🔎 Suggested cleanup
- # # changing milling angle, causes previously defined features at milling angle to be "seen" as SEM_IMAGING - # # QUERY: should we update the features to the new milling angle? - # box = wx.MessageDialog(self.main_frame, - # message=f"Do you want to update existing feature positions with the updated milling angle ({math.degrees(milling_angle):.2f}°)?", - # caption="Update existing feature positions?", style=wx.YES_NO | wx.ICON_QUESTION | wx.CENTER) - # - # ans = box.ShowModal() # Waits for the window to be closed - # if ans == wx.ID_YES: logging.debug(f"Updating existing feature positions with the updated milling angle ({math.degrees(milling_angle):.2f}°)")src/odemis/acq/move.py (1)
1640-1654: Clarify theangleparameter usage or refactor the subscription.The
milling_angleVA subscribes to_initialise_transformationwithinit=True(line 1642), passing the angle value as a parameter. However, the_initialise_transformationmethod (line 1648) receives but doesn't use thisangleparameter, instead relying onself.pre_tilt(line 1653) and metadata lookups in_update_conversion(lines 1688-1692).This works because
_set_milling_anglealready updated the metadata before the subscription fires, but the flow is confusing.Options:
- Remove the parameter and make the subscription trigger without arguments
- Use the parameter by passing it through to
_update_conversioninstead of reading from metadata🔎 Option 1: Remove unused parameter
- def _initialise_transformation(self, angle): + def _initialise_transformation(self, _angle=None): """ Initializes the transformation parameters that allows conversion between stage-bare and sample plane. """🔎 Option 2: Use the angle parameter
Refactor to pass the angle through to
_update_conversionand avoid the metadata lookup for rx_mill. This would require more substantial changes to track the milling angle explicitly rather than reading from metadata each time.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/odemis/acq/move.pysrc/odemis/acq/test/move_tescan_test.pysrc/odemis/gui/cont/tabs/fibsem_tab.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-19T08:30:13.018Z
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3200
File: src/odemis/gui/cont/tabs/fibsem_tab.py:357-369
Timestamp: 2025-08-19T08:30:13.018Z
Learning: In the FIBSEM tab GUI event handling system, warning dialogs for stage movement are informational only and should not prevent event propagation. The actual movement prevention is handled through canvas abilities (CAN_MOVE_STAGE) rather than stopping events at the handler level, as dragging and double-clicking are processed by lower-level handlers that need to receive the events.
Applied to files:
src/odemis/gui/cont/tabs/fibsem_tab.py
📚 Learning: 2025-09-25T16:47:08.868Z
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.
Applied to files:
src/odemis/acq/move.py
🧬 Code graph analysis (3)
src/odemis/acq/test/move_tescan_test.py (2)
src/odemis/acq/move.py (1)
cryoSwitchSamplePosition(136-151)src/odemis/util/testing.py (1)
assert_pos_almost_equal(172-183)
src/odemis/gui/cont/tabs/fibsem_tab.py (5)
src/odemis/driver/tescan.py (1)
CancelledError(69-71)src/odemis/model/_futures.py (1)
InstantaneousFuture(235-282)src/odemis/gui/comp/text.py (1)
SetValueRange(837-839)src/odemis/gui/util/__init__.py (1)
call_in_wx_main(41-60)src/odemis/acq/move.py (3)
getCurrentPostureLabel(127-134)getCurrentPostureLabel(390-415)cryoSwitchSamplePosition(136-151)
src/odemis/acq/move.py (1)
src/odemis/model/_vattributes.py (1)
FloatContinuous(1072-1082)
🪛 Ruff (0.14.10)
src/odemis/gui/cont/tabs/fibsem_tab.py
455-455: Unused method argument: evt
(ARG002)
461-461: Unused method argument: evt
(ARG002)
477-477: Unused method argument: future
(ARG002)
src/odemis/acq/move.py
1648-1648: Unused method argument: angle
(ARG002)
⏰ 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 (10)
src/odemis/acq/test/move_tescan_test.py (2)
25-25: LGTM! Import additions support the new milling angle test.The MILLING constant and testing utilities are appropriately imported for the new test method.
Also applies to: 27-27
33-33: LGTM! Config path updated for FIBSEM simulation.The config file name change clarifies this is for FIBSEM testing with milling support.
src/odemis/gui/cont/tabs/fibsem_tab.py (5)
25-25: LGTM! Imports support async posture switching.CancelledError and BTN_TOGGLE_PROGRESS are properly used in the new asynchronous posture management flow.
Also applies to: 46-46
188-188: LGTM! Proper initialization of async state and UI range.The
_posture_switch_futureinitialization withInstantaneousFuture()ensures initial state checks work correctly (always.done()). The milling angle range conversion from radians to integer degrees is appropriate for UI display.Also applies to: 194-195
198-199: LGTM! Improved naming and thread safety.The rename from "position" to "posture" better reflects the semantic meaning. The
@call_in_wx_maindecorator ensures_on_viewupdates the UI safely from any thread.Also applies to: 202-202
400-414: LGTM! UI updates properly deferred during posture transitions.The check
if self._posture_switch_future.done()prevents UI state updates while a posture switch is in progress, avoiding flicker and displaying intermediate states. The button enable/disable logic correctly restricts controls to SEM_IMAGING and MILLING postures.
455-464: LGTM! Async posture switching methods properly implemented.The methods correctly:
- Initiate non-blocking posture switches via
cryoSwitchSamplePosition- Store the future for state tracking
- Register completion callbacks
The
evtparameter warnings from static analysis are false positives—these parameters are required by the wx.EVT_BUTTON binding signature.src/odemis/acq/move.py (3)
86-86: LGTM! MILLING_RANGE correctly converted to radians.The conversion from degrees to radians aligns with internal angle handling conventions and the FloatContinuous VA unit specification.
522-529: LGTM! Milling angle setter correctly updates metadata.The setter properly:
- Preserves the existing
rzvalue from metadata- Updates
MD_FAV_MILL_POS_ACTIVEwith the new milling angle- Returns the angle value as required by the VA setter contract
742-743: LGTM! Special MILLING handling enables dynamic angle updates.The condition
if posture != MILLING and position_posture == posturebypasses the early exit when the target is MILLING, even if already at that posture. This is necessary to recalculate positions when the milling angle changes (supporting the MILLING→MILLING transformation at line 371).
tmoerkerken
changed the title
tmoerkerken
force-pushed
the
milling-angle-as-va
branch
2 times, most recently
from
59e4c65 to
27d508e
Compare
tmoerkerken
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/odemis/gui/cont/tabs/fibsem_tab.py:
- Around line 447-455: Existing posture-switch futures are replaced without
cancellation, leaving orphaned background tasks; before assigning a new future
in _move_to_milling_posture and _move_to_sem_posture, check for an existing
self._posture_switch_future and call its cancel() (and optionally only if it’s
not done/cancelled) to stop the previous operation before creating the new
future, then proceed to assign the new future and add_done_callback as before.
♻️ Duplicate comments (1)
src/odemis/gui/cont/tabs/fibsem_tab.py (1)
468-470: Add exception handling to_on_sem_move_complete.The SEM posture completion handler lacks exception handling, unlike
_on_milling_move_complete(lines 458-466). This inconsistency means errors or cancellations during SEM posture switches are silently ignored without logging or user feedback.🔎 Proposed fix for consistent exception handling
@call_in_wx_main def _on_sem_move_complete(self, future): + try: + future.result() + except CancelledError: + logging.info("Posture switch was cancelled") + except Exception: + logging.exception("Failed to switch posture") + self._on_stage_pos(self.pm.stage.position.value)Based on past review comments, this issue was previously identified but remains unaddressed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/odemis/gui/cont/tabs/fibsem_tab.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T08:30:13.018Z
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3200
File: src/odemis/gui/cont/tabs/fibsem_tab.py:357-369
Timestamp: 2025-08-19T08:30:13.018Z
Learning: In the FIBSEM tab GUI event handling system, warning dialogs for stage movement are informational only and should not prevent event propagation. The actual movement prevention is handled through canvas abilities (CAN_MOVE_STAGE) rather than stopping events at the handler level, as dragging and double-clicking are processed by lower-level handlers that need to receive the events.
Applied to files:
src/odemis/gui/cont/tabs/fibsem_tab.py
🧬 Code graph analysis (1)
src/odemis/gui/cont/tabs/fibsem_tab.py (3)
src/odemis/driver/tescan.py (1)
CancelledError(69-71)src/odemis/model/_futures.py (1)
InstantaneousFuture(235-282)src/odemis/acq/move.py (1)
cryoSwitchSamplePosition(136-151)
🪛 Ruff (0.14.10)
src/odemis/gui/cont/tabs/fibsem_tab.py
447-447: Unused method argument: evt
(ARG002)
453-453: Unused method argument: evt
(ARG002)
469-469: Unused method argument: future
(ARG002)
⏰ 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 (7)
src/odemis/gui/cont/tabs/fibsem_tab.py (7)
25-26: LGTM! Import additions support async posture switching.The additions of
CancelledErrorandBTN_TOGGLE_PROGRESScorrectly support the new asynchronous posture switching logic with proper cancellation handling and UI state indication.Also applies to: 46-46
188-188: LGTM! Appropriate initial state for posture switch tracking.Initializing with
InstantaneousFuture()correctly represents "no switch in progress" since it's immediately done, allowing initial UI updates to proceed.
194-195: LGTM! Proper unit conversion for UI.The conversion from
MILLING_RANGE(radians) to integer degrees usingnumpy.rad2degand rounding is appropriate for the tilt angle UI control, preventing excessive decimal places.
198-199: LGTM! Method renames and thread-safety decorator.The renames to
_move_to_milling_postureand_move_to_sem_posturebetter reflect posture-based terminology. Adding@call_in_wx_mainto_on_viewensures thread-safe UI updates when this subscriber callback is invoked.Also applies to: 202-202
400-414: LGTM! Guarded UI updates prevent intermediate states.Gating button enable/disable and value updates on
_posture_switch_future.done()correctly prevents UI flickering or incorrect states during ongoing posture transitions. Completion callbacks will trigger a final refresh.
416-445: LGTM! Milling angle update logic correctly handles posture coordination.The refactored flow properly:
- Updates
milling_anglevia the posture manager VA (triggering internal MD updates)- Propagates the resulting
stage_tiltto existing feature positions- Triggers a posture switch when already at MILLING to apply the new angle immediately
The check for
evt is Noneappropriately skips UI interactions during initialization.
447-455: LGTM! Correct asynchronous posture switch pattern.Both methods properly initiate non-blocking posture switches via
cryoSwitchSamplePosition, store the future, and attach completion callbacks. Theevtparameter is unused but required by thewx.EVT_BUTTONhandler signature.
tmoerkerken
force-pushed
the
milling-angle-as-va
branch
3 times, most recently
from
92f477c to
b8966e1
Compare
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 522-529: The setter _set_milling_angle accesses
model.MD_FAV_MILL_POS_ACTIVE["rz"] without checking presence, causing KeyError
on non-milling systems; fix by reading metadata = self.stage.getMetadata(),
retrieving fav = metadata.get(model.MD_FAV_MILL_POS_ACTIVE) and use
fav.get("rz") if fav and "rz" in fav else a safe default (e.g., 0.0) before
building rotations = {'rx': angle, 'rz': rz_value}, then call
self.stage.updateMetadata(...).
In @src/odemis/gui/cont/tabs/fibsem_tab.py:
- Around line 446-454: The handlers _move_to_milling_posture and
_move_to_sem_posture replace self._posture_switch_future without cancelling any
previous task; before assigning a new future, check if
self._posture_switch_future exists and is not done, call its cancel() (and
optionally ignore CancellationError in its callback), then assign the new future
from pm.cryoSwitchSamplePosition(MILLING) /
pm.cryoSwitchSamplePosition(SEM_IMAGING) and attach the appropriate
add_done_callback (_on_milling_move_complete or _on_sem_move_complete) after
assignment to avoid orphaned async tasks.
- Around line 468-469: The SEM move completion handler _on_sem_move_complete
currently calls _on_stage_pos without any error handling; wrap its body in a
try/except similar to the milling handler: catch asyncio.CancelledError (or
concurrent.futures.CancelledError) and return quietly, and catch Exception to
log the error (use the same logger used elsewhere, e.g., self._logger.exception
or self._logger.error) so unexpected exceptions from the SEM posture switch
don’t propagate; keep the existing call to
self._on_stage_pos(self.pm.stage.position.value) inside the try block.
🧹 Nitpick comments (1)
src/odemis/acq/move.py (1)
1649-1655: Consider passing the angle parameter through for clarity.The
angleparameter from themilling_anglesubscriber is unused. Instead,_update_conversionreads the updated value from stage metadata (line 1691). While functional, passing the angle directly would make the data flow clearer and reduce metadata access.🔎 Optional refactor for clarity
-def _initialise_transformation(self, angle): +def _initialise_transformation(self, angle: float): """ Initializes the transformation parameters that allows conversion between stage-bare and sample plane. + :param angle: milling angle in radians (from the VA subscriber) """ self._axes_dep = {"x": self.linked_axes[0], "y": self.linked_axes[1]} - self._update_conversion(self.pre_tilt) + # Note: _update_conversion will read the milling angle from stage metadata, + # which was already updated by the VA setter + self._update_conversion(self.pre_tilt) # angle is implicitly used via metadata self._initialise_offset()Alternatively, refactor to pass the angle to
_update_conversionand avoid the metadata re-read, but this would require broader changes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/odemis/acq/move.pysrc/odemis/acq/test/move_tescan_test.pysrc/odemis/gui/cont/tabs/fibsem_tab.py
🧰 Additional context used
🧠 Learnings (3)
📓 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-08-19T08:30:13.018Z
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3200
File: src/odemis/gui/cont/tabs/fibsem_tab.py:357-369
Timestamp: 2025-08-19T08:30:13.018Z
Learning: In the FIBSEM tab GUI event handling system, warning dialogs for stage movement are informational only and should not prevent event propagation. The actual movement prevention is handled through canvas abilities (CAN_MOVE_STAGE) rather than stopping events at the handler level, as dragging and double-clicking are processed by lower-level handlers that need to receive the events.
Applied to files:
src/odemis/gui/cont/tabs/fibsem_tab.py
📚 Learning: 2025-09-25T16:47:08.868Z
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.
Applied to files:
src/odemis/acq/move.py
🧬 Code graph analysis (3)
src/odemis/gui/cont/tabs/fibsem_tab.py (5)
src/odemis/driver/tescan.py (1)
CancelledError(69-71)src/odemis/model/_futures.py (1)
InstantaneousFuture(235-282)src/odemis/gui/comp/text.py (1)
SetValueRange(837-839)src/odemis/gui/util/__init__.py (1)
call_in_wx_main(41-60)src/odemis/acq/move.py (1)
cryoSwitchSamplePosition(136-151)
src/odemis/acq/test/move_tescan_test.py (4)
src/odemis/acq/test/move_tfs1_test.py (1)
TestMeteorTFS1Move(37-292)src/odemis/acq/move.py (1)
cryoSwitchSamplePosition(136-151)src/odemis/util/testing.py (1)
assert_pos_almost_equal(172-183)src/odemis/model/_components.py (1)
model(570-571)
src/odemis/acq/move.py (1)
src/odemis/model/_vattributes.py (1)
FloatContinuous(1072-1082)
🪛 Ruff (0.14.10)
src/odemis/gui/cont/tabs/fibsem_tab.py
446-446: Unused method argument: evt
(ARG002)
452-452: Unused method argument: evt
(ARG002)
468-468: Unused method argument: future
(ARG002)
src/odemis/acq/move.py
1649-1649: Unused method argument: angle
(ARG002)
⏰ 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 (1)
126-154: LGTM! Well-structured test for milling angle stability.The test correctly verifies that:
- Sample stage position remains stable across SEM ↔ MILLING transitions
- Updating the milling angle doesn't unexpectedly shift the sample position
- Position consistency is maintained after the angle change
The test structure is clear and uses appropriate tolerances.
src/odemis/gui/cont/tabs/fibsem_tab.py (2)
188-188: LGTM! Good addition of posture switch tracking.Initializing
_posture_switch_futurewithInstantaneousFuture()provides a clean way to track asynchronous posture switches and check completion status before updating UI state.
194-195: LGTM! Correct conversion for UI display.The conversion from radians to integer degrees is appropriate for the UI control, preventing excessive decimal places while maintaining the correct range.
src/odemis/acq/move.py (3)
86-86: LGTM! Correct conversion to radians.Converting
MILLING_RANGEto radians aligns with the internal representation used throughout the codebase for angular values.
335-340: Safe initialization, but verify setter guard.The VA initialization safely defaults to 0 for non-milling systems. However, ensure the setter (line 527) has appropriate guards—see separate comment on that method.
742-744: LGTM! Correct handling of dynamic milling angle.Skipping the fast-path when the target posture is
MILLINGcorrectly handles the case where the milling angle has changed, requiring re-transformation even when already at the milling posture.
| def _move_to_milling_posture(self, evt: wx.Event): | ||
| self._posture_switch_future = self.pm.cryoSwitchSamplePosition(MILLING) | ||
|
|
||
| # Do NOT call f.result(). Instead, add a callback: | ||
| self._posture_switch_future.add_done_callback(self._on_milling_move_complete) | ||
|
|
||
| def _move_to_sem_posture(self, evt: wx.Event): | ||
| self._posture_switch_future = self.pm.cryoSwitchSamplePosition(SEM_IMAGING) | ||
| self._posture_switch_future.add_done_callback(self._on_sem_move_complete) |
There was a problem hiding this comment.
Cancel the previous posture switch before initiating a new one.
If a user rapidly clicks between posture buttons, the previous _posture_switch_future is replaced without cancellation, leaving an orphaned async task running. Add cancellation logic before assigning a new future.
🔎 Proposed fix
def _move_to_milling_posture(self, evt: wx.Event):
+ if self._posture_switch_future and not self._posture_switch_future.done():
+ self._posture_switch_future.cancel()
self._posture_switch_future = self.pm.cryoSwitchSamplePosition(MILLING)
# Do NOT call f.result(). Instead, add a callback:
self._posture_switch_future.add_done_callback(self._on_milling_move_complete)
def _move_to_sem_posture(self, evt: wx.Event):
+ if self._posture_switch_future and not self._posture_switch_future.done():
+ self._posture_switch_future.cancel()
self._posture_switch_future = self.pm.cryoSwitchSamplePosition(SEM_IMAGING)
self._posture_switch_future.add_done_callback(self._on_sem_move_complete)🧰 Tools
🪛 Ruff (0.14.10)
446-446: Unused method argument: evt
(ARG002)
452-452: Unused method argument: evt
(ARG002)
🤖 Prompt for AI Agents
In @src/odemis/gui/cont/tabs/fibsem_tab.py around lines 446 - 454, The handlers
_move_to_milling_posture and _move_to_sem_posture replace
self._posture_switch_future without cancelling any previous task; before
assigning a new future, check if self._posture_switch_future exists and is not
done, call its cancel() (and optionally ignore CancellationError in its
callback), then assign the new future from pm.cryoSwitchSamplePosition(MILLING)
/ pm.cryoSwitchSamplePosition(SEM_IMAGING) and attach the appropriate
add_done_callback (_on_milling_move_complete or _on_sem_move_complete) after
assignment to avoid orphaned async tasks.
tmoerkerken
force-pushed
the
milling-angle-as-va
branch
from
d76391a to
b633423
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
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/test/move_tescan_test.py (1)
23-29: Remove duplicate import oftesting.The
testingmodule is imported twice: at line 23 and again at line 29.🔧 Proposed fix
import logging import math import os import unittest from odemis.util import testing import odemis from odemis import model from odemis.acq.move import (FM_IMAGING, LOADING, MILLING, SEM_IMAGING, UNKNOWN) from odemis.acq.test.move_tfs1_test import TestMeteorTFS1Move -from odemis.util import testing
🤖 Fix all issues with AI agents
In `@src/odemis/acq/test/move_tescan_test.py`:
- Around line 168-195: The test test_milling_angle_stable_pos is updating
MD_FAV_MILL_POS_ACTIVE with legacy 'rx' key; change the updateMetadata call on
self.stage to set the milling angle under the new 'mill_angle' key (keeping the
existing "rz" value) instead of 'rx' so the code path that uses
calculate_stage_tilt_from_milling_angle() is exercised; update the dict passed
to self.stage.updateMetadata({model.MD_FAV_MILL_POS_ACTIVE: {'mill_angle':
milling_angle, "rz": current_md[model.MD_FAV_MILL_POS_ACTIVE]["rz"]}}) and leave
the rest of the test unchanged.
In `@src/odemis/gui/cont/tabs/fibsem_tab.py`:
- Around line 399-413: _on_stage_pos accesses self._posture_switch_future before
it's guaranteed to exist, causing an AttributeError on init; initialize
self._posture_switch_future in __init__ (e.g. set to a completed/placeholder
future or None) or change the _on_stage_pos code to use getattr(self,
"_posture_switch_future", None) / hasattr guard before calling .done(); update
references in _on_stage_pos that rely on .done() to handle a None/placeholder
value and still enable/disable or set button states safely; this touches
__init__, _on_stage_pos, and the methods
_move_to_milling_posture/_move_to_sem_posture which currently create the future.
♻️ Duplicate comments (1)
src/odemis/gui/cont/tabs/fibsem_tab.py (1)
445-453: Cancel previous posture switch before starting a new one.If a user rapidly clicks between posture buttons, the previous
_posture_switch_futureis replaced without cancellation, leaving an orphaned async task running. This could lead to unexpected behavior or race conditions.🔎 Proposed fix
def _move_to_milling_posture(self, evt: wx.Event): + if hasattr(self, '_posture_switch_future') and not self._posture_switch_future.done(): + self._posture_switch_future.cancel() self._posture_switch_future = self.pm.cryoSwitchSamplePosition(MILLING) # Do NOT call f.result(). Instead, add a callback: self._posture_switch_future.add_done_callback(self._on_move_complete) def _move_to_sem_posture(self, evt: wx.Event): + if hasattr(self, '_posture_switch_future') and not self._posture_switch_future.done(): + self._posture_switch_future.cancel() self._posture_switch_future = self.pm.cryoSwitchSamplePosition(SEM_IMAGING) self._posture_switch_future.add_done_callback(self._on_move_complete)
🧹 Nitpick comments (1)
src/odemis/acq/move.py (1)
1664-1678: Unusedangleparameter in_initialise_transformation.The
angleparameter passed via the subscription callback at line 1666 is not used in the method body. While this is technically correct (the method readsself.pre_tiltdirectly), the signature should either use the parameter or indicate it's intentionally unused.♻️ Suggested fix: use underscore prefix to indicate intentionally unused parameter
- def _initialise_transformation(self, angle): + def _initialise_transformation(self, _angle): """ Initializes the transformation parameters that allows conversion between stage-bare and sample plane. """Alternatively, if the angle should be used:
def _initialise_transformation(self, angle): """ Initializes the transformation parameters that allows conversion between stage-bare and sample plane. + :param angle: milling angle in radians (from subscription callback, triggers re-initialization) """ self._axes_dep = {"x": self.linked_axes[0], "y": self.linked_axes[1]} - self._update_conversion(self.pre_tilt) + self._update_conversion(self.pre_tilt) # Note: angle triggers re-init but pre_tilt is always used self._initialise_offset()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/odemis/acq/move.pysrc/odemis/acq/test/move_tescan_test.pysrc/odemis/gui/cont/tabs/fibsem_tab.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always use type hints for function parameters and return types in Python code
Include docstrings for all functions and classes, following the reStructuredText style guide (without type information)
Ensure code is valid for Python 3.10 and above
Clean up code at the end of a task using autopep8 with the command:autopep8 --in-place --select W291,W292,W293,W391
Files:
src/odemis/acq/move.pysrc/odemis/acq/test/move_tescan_test.pysrc/odemis/gui/cont/tabs/fibsem_tab.py
🧠 Learnings (4)
📓 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-09-25T16:47:08.868Z
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.
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.pysrc/odemis/acq/test/move_tescan_test.pysrc/odemis/gui/cont/tabs/fibsem_tab.py
📚 Learning: 2025-08-19T08:30:13.018Z
Learnt from: tmoerkerken
Repo: delmic/odemis PR: 3200
File: src/odemis/gui/cont/tabs/fibsem_tab.py:357-369
Timestamp: 2025-08-19T08:30:13.018Z
Learning: In the FIBSEM tab GUI event handling system, warning dialogs for stage movement are informational only and should not prevent event propagation. The actual movement prevention is handled through canvas abilities (CAN_MOVE_STAGE) rather than stopping events at the handler level, as dragging and double-clicking are processed by lower-level handlers that need to receive the events.
Applied to files:
src/odemis/gui/cont/tabs/fibsem_tab.py
🧬 Code graph analysis (1)
src/odemis/acq/test/move_tescan_test.py (2)
src/odemis/acq/move.py (1)
cryoSwitchSamplePosition(136-151)src/odemis/util/testing.py (1)
assert_pos_almost_equal(172-183)
🪛 Ruff (0.14.11)
src/odemis/acq/move.py
1672-1672: Unused method argument: angle
(ARG002)
src/odemis/acq/test/move_tescan_test.py
29-29: Redefinition of unused testing from line 23: testing redefined here
Remove definition: testing
(F811)
src/odemis/gui/cont/tabs/fibsem_tab.py
445-445: Unused method argument: evt
(ARG002)
451-451: Unused method argument: evt
(ARG002)
🔇 Additional comments (8)
src/odemis/gui/cont/tabs/fibsem_tab.py (2)
187-188: Metadata update uses incorrect key'rx'instead of'mill_angle'.At line 187, the test code at
test_milling_angle_stable_pos(line 187-188 in test file) uses'rx'key to update the milling angle metadata. However, based on the changes inmove.py(lines 341-356), the metadata has been migrated from'rx'to'mill_angle'. The_update_milling_anglemethod correctly usesself.pm.milling_angle.valuewhich triggers_set_milling_anglethat stores'mill_angle'.Looking at line 421 in
_update_milling_angle:self.pm.milling_angle.value = milling_angle— this is the correct approach through the VA setter.Also applies to: 415-443
455-464: LGTM: Consolidated move completion handler.The
_on_move_completemethod properly handles bothCancelledErrorand general exceptions with appropriate logging, and refreshes the stage position UI state afterward. This consolidates the previous separate handlers into a single, consistent implementation.src/odemis/acq/move.py (5)
86-86: LGTM: MILLING_RANGE correctly converted to radians.The constant is now expressed in radians
(math.radians(5), math.radians(30))which is consistent with the internal representation used throughout the posture manager. The UI layer (fibsem_tab.py) properly converts to/from degrees for display.
341-363: LGTM: Metadata upgrade path and milling_angle VA initialization.The upgrade path from legacy
'rx'to'mill_angle'key is well-implemented with:
- Clear error if both keys exist (line 352-353)
- Automatic migration with logging (lines 354-356)
- Safe fallback to 0 if metadata doesn't exist (line 360)
The
FloatContinuousVA is properly initialized with the range fromMILLING_RANGEand uses the setter for persistence.
539-549: LGTM: Guard added for non-milling systems.The setter now properly checks for
MD_FAV_MILL_POS_ACTIVEpresence and logs a warning instead of crashing on systems without milling support. This addresses the previous review concern about KeyError on non-milling systems.
765-766: LGTM: MILLING posture recalculation on angle change.The condition
posture != MILLING and position_posture == posturecorrectly allows the milling transformation to be recalculated when the milling angle changes, even when already at the MILLING posture. The comment at line 765 explains the rationale well.
1716-1728: The TODO at line 1716 is not fully resolved and should not be removed yet.While the transformation matrix
tf_millis correctly recalculated whenmilling_anglechanges via the subscription mechanism (line 1666), the TODO comments at lines 621, 652, and 1716 indicate this was intentionally marked as incomplete. The current implementation uses an indirect approach: the angle parameter passed to_initialise_transformationis unused, and instead_update_conversionreads directly from metadata. More critically, the related TODO at line 652 shows that the milling offset is also not being updated when the angle changes. The unusedangleparameter and multiple unresolved TODOs suggest this feature requires further work beyond just the transformation matrix update.src/odemis/acq/test/move_tescan_test.py (1)
168-195: Good test coverage for milling angle stability.The test
test_milling_angle_stable_posprovides valuable coverage for verifying that sample stage positions remain stable across posture transitions and milling angle changes. The test structure is logical:
- Start from LOADING
- Switch to MILLING, record position
- Switch to SEM_IMAGING, verify position unchanged
- Switch back to MILLING, verify position
- Change milling angle
- Repeat verification
Once the
'rx'vs'mill_angle'key issue is fixed, this will be a solid regression test.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def test_milling_angle_stable_pos(self): | ||
| # Make sure to start from a valid position | ||
| self.posture_manager.cryoSwitchSamplePosition(LOADING).result() | ||
| # Transform to milling posture | ||
| self.posture_manager.cryoSwitchSamplePosition(MILLING).result() | ||
| # Store shorthand for sample stage | ||
| sample_stage = self.posture_manager.sample_stage | ||
| # Take note of sample stage pos | ||
| initial_sample_stage_pos = sample_stage.position.value | ||
| # Switch to SEM posture | ||
| self.posture_manager.cryoSwitchSamplePosition(SEM_IMAGING).result() | ||
| # Compare sample stage pos to previous pos | ||
| testing.assert_pos_almost_equal(sample_stage.position.value, initial_sample_stage_pos, atol=1e-6) | ||
| # Switch back to milling posture | ||
| self.posture_manager.cryoSwitchSamplePosition(MILLING).result() | ||
| testing.assert_pos_almost_equal(sample_stage.position.value, initial_sample_stage_pos, atol=1e-6) | ||
| # Now change milling angle to check stability; sample stage pos should remain the same. | ||
| milling_angle = math.radians(30) | ||
| current_md = self.stage.getMetadata() | ||
| self.stage.updateMetadata({model.MD_FAV_MILL_POS_ACTIVE: {'rx': milling_angle, | ||
| "rz": current_md[model.MD_FAV_MILL_POS_ACTIVE]["rz"]}}) | ||
| # Switch to SEM posture | ||
| self.posture_manager.cryoSwitchSamplePosition(SEM_IMAGING).result() | ||
| # Compare sample stage pos to previous pos | ||
| testing.assert_pos_almost_equal(sample_stage.position.value, initial_sample_stage_pos, atol=1e-6) | ||
| # Switch back to milling posture | ||
| self.posture_manager.cryoSwitchSamplePosition(MILLING).result() | ||
| testing.assert_pos_almost_equal(sample_stage.position.value, initial_sample_stage_pos, atol=1e-6) |
There was a problem hiding this comment.
Test uses legacy 'rx' key instead of new 'mill_angle' key.
At lines 187-188, the test directly updates MD_FAV_MILL_POS_ACTIVE with 'rx' key:
self.stage.updateMetadata({model.MD_FAV_MILL_POS_ACTIVE: {'rx': milling_angle, ...}})However, based on the changes in move.py, the milling angle should now be stored under 'mill_angle' key, not 'rx'. The 'rx' key is computed dynamically from 'mill_angle' using calculate_stage_tilt_from_milling_angle(). This test might not correctly exercise the new milling angle flow.
🔧 Proposed fix: Use the milling_angle VA or correct key
# Now change milling angle to check stability; sample stage pos should remain the same.
milling_angle = math.radians(30)
- current_md = self.stage.getMetadata()
- self.stage.updateMetadata({model.MD_FAV_MILL_POS_ACTIVE: {'rx': milling_angle,
- "rz": current_md[model.MD_FAV_MILL_POS_ACTIVE]["rz"]}})
+ # Use the posture manager's milling_angle VA to properly trigger the update flow
+ self.posture_manager.milling_angle.value = milling_angleOr if directly updating metadata is intended for testing:
milling_angle = math.radians(30)
current_md = self.stage.getMetadata()
- self.stage.updateMetadata({model.MD_FAV_MILL_POS_ACTIVE: {'rx': milling_angle,
+ self.stage.updateMetadata({model.MD_FAV_MILL_POS_ACTIVE: {'mill_angle': milling_angle,
"rz": current_md[model.MD_FAV_MILL_POS_ACTIVE]["rz"]}})
+ # Trigger transformation re-initialization
+ self.posture_manager._initialise_transformation(milling_angle)🤖 Prompt for AI Agents
In `@src/odemis/acq/test/move_tescan_test.py` around lines 168 - 195, The test
test_milling_angle_stable_pos is updating MD_FAV_MILL_POS_ACTIVE with legacy
'rx' key; change the updateMetadata call on self.stage to set the milling angle
under the new 'mill_angle' key (keeping the existing "rz" value) instead of 'rx'
so the code path that uses calculate_stage_tilt_from_milling_angle() is
exercised; update the dict passed to
self.stage.updateMetadata({model.MD_FAV_MILL_POS_ACTIVE: {'mill_angle':
milling_angle, "rz": current_md[model.MD_FAV_MILL_POS_ACTIVE]["rz"]}}) and leave
the rest of the test unchanged.
| if self._posture_switch_future.done(): | ||
| self.panel.btn_switch_sem_imaging.Enable(posture in [SEM_IMAGING, MILLING]) | ||
| self.panel.btn_switch_milling.Enable(posture in [SEM_IMAGING, MILLING]) | ||
| self.panel.ctrl_milling_angle.Enable(posture in [SEM_IMAGING, MILLING]) | ||
|
|
||
| if posture == SEM_IMAGING: | ||
| self.panel.btn_switch_sem_imaging.SetValue(BTN_TOGGLE_COMPLETE) # BTN_TOGGLE_COMPLETE | ||
| if posture == MILLING: | ||
| self.panel.btn_switch_milling.SetValue(BTN_TOGGLE_COMPLETE) | ||
| if posture == SEM_IMAGING: | ||
| self.panel.btn_switch_sem_imaging.SetValue(BTN_TOGGLE_COMPLETE) | ||
| else: | ||
| self.panel.btn_switch_sem_imaging.SetValue(BTN_TOGGLE_OFF) | ||
| if posture == MILLING: | ||
| self.panel.btn_switch_milling.SetValue(BTN_TOGGLE_COMPLETE) | ||
| else: | ||
| self.panel.btn_switch_milling.SetValue(BTN_TOGGLE_OFF) | ||
|
|
||
| self.panel.Layout() | ||
| self.panel.Layout() |
There was a problem hiding this comment.
Potential AttributeError on first _on_stage_pos call.
_posture_switch_future is accessed at line 399 before it's ever assigned. The attribute is only created when _move_to_milling_posture or _move_to_sem_posture is called, but _on_stage_pos is subscribed with init=True at line 188, which means it will be called during __init__ before any posture switch future exists.
🐛 Proposed fix: Initialize the future in __init__
Add initialization near line 188:
+ self._posture_switch_future = model.InstantaneousFuture() # Placeholder, always "done"
self.pm.stage.position.subscribe(self._on_stage_pos, init=True)Or use hasattr/getattr with a fallback:
- if self._posture_switch_future.done():
+ if getattr(self, '_posture_switch_future', None) is None or self._posture_switch_future.done():🤖 Prompt for AI Agents
In `@src/odemis/gui/cont/tabs/fibsem_tab.py` around lines 399 - 413, _on_stage_pos
accesses self._posture_switch_future before it's guaranteed to exist, causing an
AttributeError on init; initialize self._posture_switch_future in __init__ (e.g.
set to a completed/placeholder future or None) or change the _on_stage_pos code
to use getattr(self, "_posture_switch_future", None) / hasattr guard before
calling .done(); update references in _on_stage_pos that rely on .done() to
handle a None/placeholder value and still enable/disable or set button states
safely; this touches __init__, _on_stage_pos, and the methods
_move_to_milling_posture/_move_to_sem_posture which currently create the future.
There was a problem hiding this comment.
Using self._posture_switch_future = model.InstantaneousFuture() in the init should be the right way. @tmoerkerken hadn't you done this? How come it's gone?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if self._posture_switch_future.done(): | ||
| self.panel.btn_switch_sem_imaging.Enable(posture in [SEM_IMAGING, MILLING]) | ||
| self.panel.btn_switch_milling.Enable(posture in [SEM_IMAGING, MILLING]) | ||
| self.panel.ctrl_milling_angle.Enable(posture in [SEM_IMAGING, MILLING]) | ||
|
|
||
| if posture == SEM_IMAGING: | ||
| self.panel.btn_switch_sem_imaging.SetValue(BTN_TOGGLE_COMPLETE) # BTN_TOGGLE_COMPLETE | ||
| if posture == MILLING: | ||
| self.panel.btn_switch_milling.SetValue(BTN_TOGGLE_COMPLETE) | ||
| if posture == SEM_IMAGING: | ||
| self.panel.btn_switch_sem_imaging.SetValue(BTN_TOGGLE_COMPLETE) | ||
| else: | ||
| self.panel.btn_switch_sem_imaging.SetValue(BTN_TOGGLE_OFF) | ||
| if posture == MILLING: | ||
| self.panel.btn_switch_milling.SetValue(BTN_TOGGLE_COMPLETE) | ||
| else: | ||
| self.panel.btn_switch_milling.SetValue(BTN_TOGGLE_OFF) | ||
|
|
||
| self.panel.Layout() | ||
| self.panel.Layout() |
There was a problem hiding this comment.
Using self._posture_switch_future = model.InstantaneousFuture() in the init should be the right way. @tmoerkerken hadn't you done this? How come it's gone?
| tf_sem = tf_reverse @ tf_tilt @ tf_sr | ||
| tf_sem_inv = numpy.linalg.inv(tf_sem) | ||
|
|
||
| # TODO: update MILLING transformations when changing milling angle |
| self._initialise_transformation(axes=["y", "z"], rotation=self.pre_tilt) | ||
|
|
||
| self.linked_axes = ["y", "z"] | ||
| self.milling_angle.subscribe(self._initialise_transformation, init=True) |
There was a problem hiding this comment.
Can you add a comment explain why re-initialising the transformation whenever the milling angle changes (and why it works although the function doesn't read the milling_angle VA anywhere.
| """ | ||
| pass | ||
|
|
||
| def _set_milling_angle(self, angle: float): |
There was a problem hiding this comment.
Add type annotation for the return value.
4 participants
Uh oh!
There was an error while loading. Please reload this page.