[MSD-40][refactor] Milling angle as VA with automatic sample stage matrix recalculation#3313
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughMILLING_RANGE was converted from degrees to radians. Posture managers expose a public milling_angle FloatContinuous wired to stage metadata MD_FAV_MILL_POS_ACTIVE["mill_angle"] and persist changes via a new _set_milling_angle(angle) that preserves rz. Transformation initialization API changed to _initialise_transformation(angle) and the update API to _update_conversion(pre_tilt); relevant posture managers set linked_axes = ["y", "z"] and subscribe milling_angle to initialise transformations. to_posture avoids the fast-path when switching to MILLING to accommodate milling-angle changes. FibsemTab initiates MILLING/SEM posture switches asynchronously and handles completion via _on_move_complete. Tests verify milling-angle callbacks and position 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🧪 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: 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.
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.
tmoerkerken
force-pushed
the
milling-angle-as-va
branch
from
b633423 to
f7b486c
Compare
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 26-29: Remove the duplicate import of numpy in the module
top-level imports of fibsem_tab.py: keep a single "import numpy" and delete the
redundant second occurrence; ensure any other imports remain unchanged and run
the linter/flake8 to confirm no unused-import warnings remain.
🧹 Nitpick comments (1)
src/odemis/acq/move.py (1)
1672-1678: Consider prefixing unusedangleparameter with underscore.The
angleparameter is required by the VA subscriber callback signature but isn't used directly (the method reads from metadata instead). Adding an underscore prefix would clarify this is intentional.🔧 Proposed fix
- def _initialise_transformation(self, angle): + def _initialise_transformation(self, _angle): """ Initializes the transformation parameters that allows conversion between stage-bare and sample plane. """
tmoerkerken
force-pushed
the
milling-angle-as-va
branch
2 times, most recently
from
7cbb126 to
50389a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/odemis/acq/move.py`:
- Around line 1673-1679: Update the _initialise_transformation method signature
to accept the unused callback parameter as _angle: float and add the return type
-> None, and update its docstring to document the _angle parameter and that the
method returns None; specifically modify def _initialise_transformation(self,
angle): to def _initialise_transformation(self, _angle: float) -> None, and
expand the docstring to describe the _angle parameter (noting it is unused) and
the return value, leaving the body (self._axes_dep, self._update_conversion,
self._initialise_offset) unchanged so the ruff ARG002 warning is resolved.
- Around line 358-363: The code initializes milling_angle to 0 when
model.MD_FAV_MILL_POS_ACTIVE is missing, but FloatContinuous(milling_angle,
(MILLING_RANGE...)) will reject values outside MILLING_RANGE and fail; change
the initialization so when stage_md.get(model.MD_FAV_MILL_POS_ACTIVE) is absent
you default to a value inside MILLING_RANGE (e.g., MILLING_RANGE[0] or the
midpoint) or clamp the extracted mill_angle into MILLING_RANGE before
constructing model.FloatContinuous; update references in this block that set
self.milling_angle and keep the setter=self._set_milling_angle unchanged.
In `@src/odemis/acq/test/move_tescan_test.py`:
- Line 238: Remove the trailing-whitespace blank line flagged by W293 in
src/odemis/acq/test/move_tescan_test.py by deleting the whitespace-only line (or
run autopep8 --in-place --select W291,W292,W293,W391 against that file) so the
file has no whitespace-only lines at the end; target the trailing blank line in
move_tescan_test.py and re-run the linter to confirm.
- Around line 239-276: Add reStructuredText docstrings and explicit return type
annotations "-> None" to the two test methods test_milling_angle_callback and
test_milling_angle_stable_pos: open the methods named
test_milling_angle_callback and test_milling_angle_stable_pos, prepend a short
reST-style docstring (triple-quoted, brief description of the test) as the first
statement in each, and update their signatures to include "-> None" (e.g., def
test_milling_angle_callback(self) -> None:) so both comply with the project's
docstring and typing guidelines.
In `@src/odemis/gui/cont/tabs/fibsem_tab.py`:
- Line 191: Remove the trailing whitespace-only blank line flagged by W293 in
the fibsem_tab.py module: open the fibsem_tab.py source (e.g., inside the
FibsemTab class or nearby helper functions) and delete the whitespace-only line
so no line ends contain extra spaces; then run the suggested autopep8 command
(autopep8 --in-place --select W291,W292,W293,W391) to ensure the fix and similar
issues are cleaned up.
- Around line 418-466: Add explicit return type annotations and reStructuredText
docstrings to _update_milling_angle, _move_to_milling_posture,
_move_to_sem_posture, and _on_move_complete; rename the unused event parameters
in _move_to_milling_posture and _move_to_sem_posture from evt to _evt to signal
they are intentionally unused. For each method, add a one-paragraph docstring in
reST describing purpose, parameters (evt or future) and return type, and
annotate signatures (e.g. -> None or -> NoneType as your codebase uses); keep
behavior and logic unchanged and ensure _on_move_complete retains the
`@call_in_wx_main` decorator and its future parameter name.
| def _initialise_transformation(self, angle): | ||
| """ | ||
| Initializes the transformation parameters that allows conversion between stage-bare and sample plane. | ||
| :param axes: stage axes which are used to calculate transformation parameters | ||
| :param rotation: rotation in radians from sample plane to stage | ||
| :param scale: scale from sample to stage | ||
| :param translation: translation from sample to stage | ||
| :param shear: shear from sample to stage | ||
| """ | ||
| self._axes_dep = {"x": axes[0], "y": axes[1]} # TODO: Should be called y, z... or even better: also take x as first axis | ||
| self._update_conversion(rotation) | ||
| self._axes_dep = {"x": self.linked_axes[0], "y": self.linked_axes[1]} # TODO: Should be called y, z... or even better: also take x as first axis | ||
| self._update_conversion(self.pre_tilt) | ||
| self._initialise_offset() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file
head -n 1680 src/odemis/acq/move.py | tail -n 10Repository: delmic/odemis
Length of output: 532
🏁 Script executed:
# Get more context around the function
sed -n '1665,1690p' src/odemis/acq/move.pyRepository: delmic/odemis
Length of output: 1363
🏁 Script executed:
# Check if there are type hints elsewhere in the file to understand the style
rg "def.*\(.*\)" src/odemis/acq/move.py -A 2 | head -n 50Repository: delmic/odemis
Length of output: 1705
🏁 Script executed:
# Check if ruff/linting reports are available or if we can verify the unused parameter
rg "_initialise_transformation" src/odemis/acq/move.py -B 2 -A 10Repository: delmic/odemis
Length of output: 4222
Add type hints, complete docstring, and mark unused callback parameter
The angle parameter is unused in this VA callback method (ruff ARG002). Rename to _angle: float, add return type -> None, and document the parameter in the docstring.
Suggested fix
- def _initialise_transformation(self, angle):
- """
- Initializes the transformation parameters that allows conversion between stage-bare and sample plane.
- """
+ def _initialise_transformation(self, _angle: float) -> None:
+ """
+ Initializes the transformation parameters that allows conversion between stage-bare and sample plane.
+ :param _angle: Current milling angle in radians (provided by VA callback).
+ """🧰 Tools
🪛 Ruff (0.14.14)
[warning] 1673-1673: Unused method argument: angle
(ARG002)
🤖 Prompt for AI Agents
In `@src/odemis/acq/move.py` around lines 1673 - 1679, Update the
_initialise_transformation method signature to accept the unused callback
parameter as _angle: float and add the return type -> None, and update its
docstring to document the _angle parameter and that the method returns None;
specifically modify def _initialise_transformation(self, angle): to def
_initialise_transformation(self, _angle: float) -> None, and expand the
docstring to describe the _angle parameter (noting it is unused) and the return
value, leaving the body (self._axes_dep, self._update_conversion,
self._initialise_offset) unchanged so the ruff ARG002 warning is resolved.
| for axis in m_bare.keys(): | ||
| exp_bare_pos[axis] += m_bare[axis] | ||
| testing.assert_pos_almost_equal(new_bare_pos, exp_bare_pos, atol=1e-6) | ||
|
|
There was a problem hiding this comment.
Remove trailing whitespace to satisfy W293
There’s a whitespace-only blank line flagged by the linter. Please strip it (or run the prescribed autopep8 command).
As per coding guidelines: "Clean up code at the end of a task using autopep8 with the command: autopep8 --in-place --select W291,W292,W293,W391".
🧰 Tools
🪛 GitHub Actions: Linting
[warning] 238-238: W293 blank line contains whitespace
🤖 Prompt for AI Agents
In `@src/odemis/acq/test/move_tescan_test.py` at line 238, Remove the
trailing-whitespace blank line flagged by W293 in
src/odemis/acq/test/move_tescan_test.py by deleting the whitespace-only line (or
run autopep8 --in-place --select W291,W292,W293,W391 against that file) so the
file has no whitespace-only lines at the end; target the trailing blank line in
move_tescan_test.py and re-run the linter to confirm.
| def test_milling_angle_callback(self): | ||
| initial_mill_pos_md = self.stage_md.get(model.MD_FAV_MILL_POS_ACTIVE) | ||
| self.assertIsNotNone(initial_mill_pos_md) | ||
| initial_mill_angle = math.radians(10) | ||
| self.stage.updateMetadata({model.MD_FAV_MILL_POS_ACTIVE: {"rx": initial_mill_angle, "rz": initial_mill_pos_md["rz"]}}) | ||
| # Now test that when altering the milling angle VA, the metadata is automatically updated. | ||
| self.pm.milling_angle.value = math.radians(22) | ||
| altered_mill_angle_md = self.pm.stage.getMetadata().get(model.MD_FAV_MILL_POS_ACTIVE)["mill_angle"] | ||
| self.assertNotAlmostEqual(initial_mill_angle, altered_mill_angle_md, places=3) | ||
|
|
||
| def test_milling_angle_stable_pos(self): | ||
| # Make sure to start from a valid position | ||
| self.pm.cryoSwitchSamplePosition(LOADING).result() | ||
| # Transform to milling posture | ||
| self.pm.cryoSwitchSamplePosition(MILLING).result() | ||
| # Store shorthand for sample stage | ||
| sample_stage = self.pm.sample_stage | ||
| # Take note of sample stage pos | ||
| initial_sample_stage_pos = sample_stage.position.value | ||
| # Switch to SEM posture | ||
| self.pm.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.pm.cryoSwitchSamplePosition(MILLING).result() | ||
| testing.assert_pos_almost_equal(sample_stage.position.value, initial_sample_stage_pos, atol=1e-6) | ||
| # TODO: uncomment the following section once all the sample stage problems are resolved | ||
| # # Now change milling angle to check stability; sample stage pos should remain the same. | ||
| # milling_angle = math.radians(30) | ||
| # self.pm.milling_angle.value = milling_angle | ||
| # # Switch to SEM posture | ||
| # self.pm.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.pm.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.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine the test methods
find . -name "move_tescan_test.py" -type fRepository: delmic/odemis
Length of output: 98
🏁 Script executed:
# Read the relevant section of the file to verify the current state
if [ -f "src/odemis/acq/test/move_tescan_test.py" ]; then
wc -l src/odemis/acq/test/move_tescan_test.py
fiRepository: delmic/odemis
Length of output: 100
🏁 Script executed:
# Get a broader view of the test methods in the file
if [ -f "src/odemis/acq/test/move_tescan_test.py" ]; then
sed -n '230,290p' src/odemis/acq/test/move_tescan_test.py
fiRepository: delmic/odemis
Length of output: 2863
🏁 Script executed:
# Check other test methods in the file to see if they follow the docstring/type hint pattern
if [ -f "src/odemis/acq/test/move_tescan_test.py" ]; then
rg "def test_" src/odemis/acq/test/move_tescan_test.py -A 2 | head -50
fiRepository: delmic/odemis
Length of output: 1731
Add docstrings and return type annotations for test methods
Both test_milling_angle_callback and test_milling_angle_stable_pos require reStructuredText docstrings and -> None return type hints to comply with the coding guidelines for Python files.
📝 Suggested update
- def test_milling_angle_callback(self):
+ def test_milling_angle_callback(self) -> None:
+ """Verify the milling-angle VA updates metadata."""
initial_mill_pos_md = self.stage_md.get(model.MD_FAV_MILL_POS_ACTIVE)
self.assertIsNotNone(initial_mill_pos_md)
initial_mill_angle = math.radians(10)
self.stage.updateMetadata({model.MD_FAV_MILL_POS_ACTIVE: {"rx": initial_mill_angle, "rz": initial_mill_pos_md["rz"]}})
# Now test that when altering the milling angle VA, the metadata is automatically updated.
self.pm.milling_angle.value = math.radians(22)
altered_mill_angle_md = self.pm.stage.getMetadata().get(model.MD_FAV_MILL_POS_ACTIVE)["mill_angle"]
self.assertNotAlmostEqual(initial_mill_angle, altered_mill_angle_md, places=3)
- def test_milling_angle_stable_pos(self):
+ def test_milling_angle_stable_pos(self) -> None:
+ """Ensure sample-stage position is stable across posture switches."""
# Make sure to start from a valid position
self.pm.cryoSwitchSamplePosition(LOADING).result()🤖 Prompt for AI Agents
In `@src/odemis/acq/test/move_tescan_test.py` around lines 239 - 276, Add
reStructuredText docstrings and explicit return type annotations "-> None" to
the two test methods test_milling_angle_callback and
test_milling_angle_stable_pos: open the methods named
test_milling_angle_callback and test_milling_angle_stable_pos, prepend a short
reST-style docstring (triple-quoted, brief description of the test) as the first
statement in each, and update their signatures to include "-> None" (e.g., def
test_milling_angle_callback(self) -> None:) so both comply with the project's
docstring and typing guidelines.
| # TODO: replace with current_posture? | ||
| self.pm.stage.position.subscribe(self._on_stage_pos, init=True) | ||
| self.panel = panel | ||
|
|
There was a problem hiding this comment.
Remove trailing whitespace to satisfy W293
There’s a whitespace-only blank line flagged by the linter. Please strip it (or run the prescribed autopep8 command).
As per coding guidelines: "Clean up code at the end of a task using autopep8 with the command: autopep8 --in-place --select W291,W292,W293,W391".
🧰 Tools
🪛 GitHub Actions: Linting
[warning] 191-191: W293 blank line contains whitespace
🤖 Prompt for AI Agents
In `@src/odemis/gui/cont/tabs/fibsem_tab.py` at line 191, Remove the trailing
whitespace-only blank line flagged by W293 in the fibsem_tab.py module: open the
fibsem_tab.py source (e.g., inside the FibsemTab class or nearby helper
functions) and delete the whitespace-only line so no line ends contain extra
spaces; then run the suggested autopep8 command (autopep8 --in-place --select
W291,W292,W293,W391) to ensure the fix and similar issues are cleaned up.
| def _update_milling_angle(self, evt: wx.Event): | ||
| # Check if already at milling posture | ||
| already_at_milling = self.pm.getCurrentPostureLabel() == MILLING | ||
|
|
||
| # update the metadata of the stage | ||
| milling_angle = math.radians(self.panel.ctrl_milling_angle.GetValue()) | ||
| current_md = self.pm.stage.getMetadata() | ||
| self.pm.stage.updateMetadata({ | ||
| model.MD_FAV_MILL_POS_ACTIVE: {"mill_angle": milling_angle, | ||
| "rz": current_md[model.MD_FAV_MILL_POS_ACTIVE]["rz"]} | ||
| }) | ||
|
|
||
| self.pm.milling_angle.value = milling_angle # this will call the setter in the move posture manager and handle md update | ||
| md = self.pm.get_posture_orientation(MILLING) | ||
| stage_tilt = md["rx"] | ||
| self.panel.ctrl_milling_angle.SetToolTip(f"A milling angle of {math.degrees(milling_angle):.2f}° " | ||
| f"corresponds to a stage tilt of {math.degrees(stage_tilt):.2f}°") | ||
|
|
||
| self._on_stage_pos(self.pm.stage.position.value) | ||
|
|
||
| # if the tab isn't shown, we don't want to ask the user | ||
| if evt is None: # if the event is None, it means this is the initial update, dont ask the user | ||
| # if the tab isn't shown, we don't want to perform a move or change features | ||
| if evt is None: | ||
| return | ||
|
|
||
| # 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}°)") | ||
| # NOTE: use stage_tilt, not milling_angle | ||
| for feature in self.main_data.features.value: | ||
| milling_position = feature.get_posture_position(MILLING) | ||
| if milling_position is not None: | ||
| milling_position["rx"] = stage_tilt | ||
| feature.set_posture_position(MILLING, milling_position) | ||
|
|
||
| def _move_to_milling_position(self, evt: wx.Event) -> None: | ||
| logging.info(f"MILLING ORIENTATION: {self.pm.get_posture_orientation(MILLING)}") | ||
| f = self.pm.cryoSwitchSamplePosition(MILLING) | ||
| f.result() | ||
|
|
||
| def _move_to_sem(self, evt: wx.Event) -> None: | ||
| f = self.pm.cryoSwitchSamplePosition(SEM_IMAGING) | ||
| f.result() | ||
| logging.debug(f"Updating existing feature positions with the updated milling angle ({math.degrees(milling_angle):.2f}°)") | ||
| # NOTE: use stage_tilt, not milling_angle | ||
| for feature in self.main_data.features.value: | ||
| milling_position = feature.get_posture_position(MILLING) | ||
| if milling_position is not None: | ||
| milling_position["rx"] = stage_tilt | ||
| feature.set_posture_position(MILLING, milling_position) | ||
|
|
||
| if already_at_milling: | ||
| # Normally this happens automatically when clicking the ProgressRadioButton, but since we are now not | ||
| # clicking it, but still want to show progress, we set the progress state of the button manually. | ||
| self.panel.btn_switch_milling.SetValue(BTN_TOGGLE_PROGRESS) | ||
| self._move_to_milling_posture(None) | ||
|
|
||
| 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_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_move_complete) | ||
|
|
||
| @call_in_wx_main | ||
| def _on_move_complete(self, future): | ||
| try: | ||
| future.result() | ||
| except CancelledError: | ||
| logging.info("Posture switch was cancelled") | ||
| except Exception: | ||
| logging.exception("Failed to switch posture") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if file exists and examine the specific lines
if [ -f "src/odemis/gui/cont/tabs/fibsem_tab.py" ]; then
echo "=== File exists. Examining lines 418-466 ==="
sed -n '418,466p' "src/odemis/gui/cont/tabs/fibsem_tab.py"
else
echo "File not found at src/odemis/gui/cont/tabs/fibsem_tab.py"
# Search for the file
find . -name "fibsem_tab.py" 2>/dev/null
fiRepository: delmic/odemis
Length of output: 2575
🏁 Script executed:
# Check file size to ensure we can read it
wc -l src/odemis/gui/cont/tabs/fibsem_tab.py 2>/dev/null || echo "File not found"Repository: delmic/odemis
Length of output: 99
Add return type hints and docstrings
All four methods lack return type annotations and reStructuredText docstrings as required by the coding guidelines. Additionally, the unused event parameters in _move_to_milling_posture and _move_to_sem_posture should be renamed to _evt to indicate they are intentionally unused.
📝 Suggested update
- def _update_milling_angle(self, evt: wx.Event):
+ def _update_milling_angle(self, evt: wx.Event) -> None:
+ """
+ Update milling angle metadata and associated feature positions.
+ :param evt: UI event triggering the update.
+ """
# Check if already at milling posture
already_at_milling = self.pm.getCurrentPostureLabel() == MILLING
@@
- def _move_to_milling_posture(self, evt: wx.Event):
+ def _move_to_milling_posture(self, _evt: wx.Event) -> None:
+ """
+ Start an asynchronous switch to milling posture.
+ :param _evt: UI event (unused).
+ """
self._posture_switch_future = self.pm.cryoSwitchSamplePosition(MILLING)
@@
- def _move_to_sem_posture(self, evt: wx.Event):
+ def _move_to_sem_posture(self, _evt: wx.Event) -> None:
+ """
+ Start an asynchronous switch to SEM posture.
+ :param _evt: UI event (unused).
+ """
self._posture_switch_future = self.pm.cryoSwitchSamplePosition(SEM_IMAGING)
self._posture_switch_future.add_done_callback(self._on_move_complete)
@@
- def _on_move_complete(self, future):
+ def _on_move_complete(self, future) -> None:
+ """
+ Handle completion of a posture switch future.
+ :param future: The completed future.
+ """
try:
future.result()🧰 Tools
🪛 Ruff (0.14.14)
[warning] 448-448: Unused method argument: evt
(ARG002)
[warning] 454-454: Unused method argument: evt
(ARG002)
🤖 Prompt for AI Agents
In `@src/odemis/gui/cont/tabs/fibsem_tab.py` around lines 418 - 466, Add explicit
return type annotations and reStructuredText docstrings to
_update_milling_angle, _move_to_milling_posture, _move_to_sem_posture, and
_on_move_complete; rename the unused event parameters in
_move_to_milling_posture and _move_to_sem_posture from evt to _evt to signal
they are intentionally unused. For each method, add a one-paragraph docstring in
reST describing purpose, parameters (evt or future) and return type, and
annotate signatures (e.g. -> None or -> NoneType as your codebase uses); keep
behavior and logic unchanged and ensure _on_move_complete retains the
`@call_in_wx_main` decorator and its future parameter name.
183e4a1 to
5637049
Compare
tmoerkerken
force-pushed
the
milling-angle-as-va
branch
from
5637049 to
56e2425
Compare
[fix] Posture switch button state to match move
tmoerkerken
force-pushed
the
milling-angle-as-va
branch
from
56e2425 to
ac1acb3
Compare
4 participants
Uh oh!
There was an error while loading. Please reload this page.