Skip to content

[MSD-40][refactor] Milling angle as VA with automatic sample stage matrix recalculation #3313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tmoerkerken wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[MSD-40][refactor] Milling angle as VA with automatic sample stage matrix recalculation #3313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 29 additions & 20 deletions src/odemis/acq/move.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@

# These values might differ per system and would then require a configuration option per system.
# Hardcoded for now. Note that these values correspond to the milling angle, and not the actual stage tilt.
MILLING_RANGE = (5, 30) # degrees
MILLING_RANGE = (math.radians(5), math.radians(30))

def filter_dict(keys: set, d: Dict[str, float]) -> Dict[str, float]:
"""
Expand Down Expand Up @@ -332,7 +332,6 @@ def __init__(self, microscope):
md_calib = stage_md.get(model.MD_CALIB, {})
self.pre_tilt = md_calib.get(model.MD_SAMPLE_PRE_TILT, None)
self.fib_column_tilt = TFS_FIB_COLUMN_TILT

# use_linked_sem_focus_compensation: when True, the SEM focus is restored to the eucentric
# focus when moving the stage. This is done on TFS systems to compensate
# for the SEM focus changing when the stage is moved in Z (due to stage linking).
Expand All @@ -356,6 +355,13 @@ def __init__(self, microscope):
mill_md["mill_angle"] = mill_md.pop("rx")
self.stage.updateMetadata({model.MD_FAV_MILL_POS_ACTIVE: mill_md})

# Initialize the milling angle. If not specified in the config, set it to 0
milling_angle = stage_md.get(model.MD_FAV_MILL_POS_ACTIVE, None)
milling_angle = milling_angle["mill_angle"] if milling_angle else 0
self.milling_angle = model.FloatContinuous(
milling_angle, (MILLING_RANGE[0], MILLING_RANGE[1]), unit="rad", setter=self._set_milling_angle
)
Comment on lines +358 to +363
Copy link

@coderabbitai coderabbitai bot Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the file structure and find the relevant sections
cat -n src/odemis/acq/move.py | head -100

Repository: delmic/odemis

Length of output: 4126

🏁 Script executed:

# Find MILLING_RANGE definition around line 86
sed -n '80,95p' src/odemis/acq/move.py

Repository: delmic/odemis

Length of output: 712

🏁 Script executed:

# Find the milling_angle initialization around lines 358-363
sed -n '355,365p' src/odemis/acq/move.py

Repository: delmic/odemis

Length of output: 631

🏁 Script executed:

# Search for FloatContinuous class definition to understand validation
rg "class FloatContinuous" -A 20

Repository: delmic/odemis

Length of output: 1351

🏁 Script executed:

# Look for where FloatContinuous is imported from
rg "from.*import.*FloatContinuous|import.*FloatContinuous" src/odemis/acq/move.py

Repository: delmic/odemis

Length of output: 39

🏁 Script executed:

# Find Continuous._check implementation
rg "class Continuous" src/odemis/model/ -A 10

Repository: delmic/odemis

Length of output: 717

🏁 Script executed:

# Also check the FloatVA class to understand the full validation chain
rg "class FloatVA" src/odemis/model/ -A 15

Repository: delmic/odemis

Length of output: 1073

🏁 Script executed:

# Check if there are any tests that might show what happens when FloatContinuous is initialized with out-of-range values
rg "FloatContinuous" --type=py -B 2 -A 2 | head -100

Repository: delmic/odemis

Length of output: 7491

🏁 Script executed:

# Look for error handling or validation in the model module
rg "_check.*value" src/odemis/model/_vattributes.py -B 3 -A 5

Repository: delmic/odemis

Length of output: 7046

🏁 Script executed:

# Get the full Continuous._check implementation
sed -n '/class Continuous/,/^class /p' src/odemis/model/_vattributes.py | head -60

Repository: delmic/odemis

Length of output: 1909

🏁 Script executed:

# Also look for the actual range check in Continuous
rg "start.*end.*self.range" src/odemis/model/_vattributes.py -B 3 -A 8

Repository: delmic/odemis

Length of output: 464

🏁 Script executed:

# Get the complete Continuous._check implementation
rg "class Continuous" src/odemis/model/_vattributes.py -A 80 | grep -A 30 "def _check"

Repository: delmic/odemis

Length of output: 1183

🏁 Script executed:

# Let's search for the actual range validation logic more specifically
sed -n '/class Continuous/,/^class /p' src/odemis/model/_vattributes.py | grep -A 25 "def _check"

Repository: delmic/odemis

Length of output: 1706

🏁 Script executed:

# Now let's verify if FloatContinuous actually raises an error when initialized with out-of-range value
# by checking if there are any tests or real usages that might show this behavior
rg "milling_angle" src/odemis/acq/move.py -B 2 -A 2

Repository: delmic/odemis

Length of output: 3623

🏁 Script executed:

# Let's also check if there are any test files that test this initialization
find . -name "*test*.py" -o -name "*_test.py" | xargs grep -l "milling_angle\|MeteorTescan1PostureManager" 2>/dev/null | head -5

Repository: delmic/odemis

Length of output: 98

🏁 Script executed:

# Let's also double-check what the actual values are
python3 << 'EOF'
import math
MILLING_RANGE = (math.radians(5), math.radians(30))
print(f"MILLING_RANGE in radians: {MILLING_RANGE}")
print(f"MILLING_RANGE[0] = {MILLING_RANGE[0]}")
print(f"Default value = 0")
print(f"Is 0 within range? {MILLING_RANGE[0] <= 0 <= MILLING_RANGE[1]}")
EOF

Repository: delmic/odemis

Length of output: 206

🏁 Script executed:

# Let's verify the order of _check calls during initialization by examining VigilantAttribute.__init__
sed -n '/class VigilantAttribute/,/^class /p' src/odemis/model/_vattributes.py | head -80

Repository: delmic/odemis

Length of output: 2890

🏁 Script executed:

# Let's check if there's a test file that covers this scenario
cat src/odemis/acq/test/move_tescan_test.py | head -100

Repository: delmic/odemis

Length of output: 5033

🏁 Script executed:

# Let's find the full VigilantAttribute.__init__ implementation
sed -n '/^class VigilantAttribute\(VigilantAttributeBase\)/,/^class /p' src/odemis/model/_vattributes.py | head -150

Repository: delmic/odemis

Length of output: 39

🏁 Script executed:

# Let's also check if _check is called during __init__ by looking at _set_value
rg "_set_value.*def" src/odemis/model/_vattributes.py -A 20 | head -80

Repository: delmic/odemis

Length of output: 39

🏁 Script executed:

# Let's find where _check is called in the initialization chain
rg "def __init__" src/odemis/model/_vattributes.py | head -20

Repository: delmic/odemis

Length of output: 1077

🏁 Script executed:

# Search for where _check is invoked
rg "_check\(" src/odemis/model/_vattributes.py | head -30

Repository: delmic/odemis

Length of output: 1098

🏁 Script executed:

# Let's look at the FloatVA and FloatContinuous more carefully
sed -n '/^class FloatVA/,/^class /p' src/odemis/model/_vattributes.py | head -50

Repository: delmic/odemis

Length of output: 545

🏁 Script executed:

# Let's see the full VigilantAttribute.__init__ implementation
sed -n '/^class VigilantAttribute(VigilantAttributeBase):/,/def [^_]/p' src/odemis/model/_vattributes.py | head -100

Repository: delmic/odemis

Length of output: 2030

🏁 Script executed:

# Let's trace the actual code - find where _check is called in VigilantAttribute.__init__
sed -n '/^class VigilantAttribute(VigilantAttributeBase):/,/^class /p' src/odemis/model/_vattributes.py | sed -n '/def __init__/,/def [^_]/p' | head -50

Repository: delmic/odemis

Length of output: 1766

🏁 Script executed:

# Let's verify that the _check is called during __init__ by examining the order
# and confirm that FloatContinuous initialization would indeed raise an error
python3 << 'EOF'
import math

# Simulate what happens during FloatContinuous initialization
MILLING_RANGE = (math.radians(5), math.radians(30))
value = 0  # the default from the code
start, end = MILLING_RANGE

print(f"MILLING_RANGE: {MILLING_RANGE}")
print(f"Default milling_angle value: {value}")
print(f"Range check: {start} <= {value} <= {end}")
print(f"Check result: {start <= value <= end}")
print()

# This is what Continuous._check does
check_result = all(mn <= v <= mx for v, mn, mx in [(value, start, end)])
print(f"all(mn <= v <= mx ...): {check_result}")
print(f"not all(...): {not check_result}")

if not check_result:
    print(f"ERROR: IndexError would be raised!")
    print(f"Message: Trying to assign value '{value}' outside of the range {start}->{end}.")
else:
    print("No error - value is within range")

print()
print(f"If we use MILLING_RANGE[0] instead of 0:")
value2 = MILLING_RANGE[0]
check_result2 = start <= value2 <= end
print(f"Range check: {start} <= {value2} <= {end}")
print(f"Check result: {check_result2}")
EOF

Repository: delmic/odemis

Length of output: 546

Default milling_angle of 0 violates the allowed range and will cause initialization to fail

The FloatContinuous validation during initialization checks that the value is within MILLING_RANGE (5–30° in radians, or ~0.087–0.524 rad). With a default of 0, this is outside the configured range and will raise IndexError on systems lacking MD_FAV_MILL_POS_ACTIVE in metadata.

🛠️ Proposed fix 
-        milling_angle = milling_angle["mill_angle"] if milling_angle else 0
+        milling_angle = milling_angle["mill_angle"] if milling_angle else MILLING_RANGE[0]
🤖 Prompt for AI Agents 
In `@src/odemis/acq/move.py` around lines 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.


# current posture va
self.current_posture = model.VigilantAttribute(UNKNOWN)
self.stage.position.subscribe(self._update_posture, init=True)
Expand Down Expand Up @@ -530,6 +536,18 @@ def _transformFromSEMToMeteor(self, pos: Dict[str, float]) -> Dict[str, float]:
"""
pass

def _set_milling_angle(self, angle: float) -> float:
"""
Set the milling angle of the stage
:param angle: (float) milling angle in radians
"""
if model.MD_FAV_MILL_POS_ACTIVE not in self.stage.getMetadata():
logging.warning("Trying to set a milling angle on a system that was not configured for milling")
return angle
rotations = {'mill_angle': angle, 'rz': self.stage.getMetadata()[model.MD_FAV_MILL_POS_ACTIVE]["rz"]}
self.stage.updateMetadata({model.MD_FAV_MILL_POS_ACTIVE: rotations})
return angle

def _transformFromMeteorToSEM(self, pos: Dict[str, float]) -> Dict[str, float]:
"""
Transforms the current stage position from the meteor/FM imaging area
Expand Down Expand Up @@ -744,7 +762,8 @@ def to_posture(self, pos: Dict[str, float], posture: int) -> Dict[str, float]:

logging.info(f"Position Posture: {POSITION_NAMES[position_posture]}, Target Posture: {POSITION_NAMES[posture]}")

if position_posture == posture:
# The milling angle can change, so we should handle the MILLING --> MILLING posture switch differently.
if posture != MILLING and position_posture == posture:
return pos

# validate the transformation
Expand Down Expand Up @@ -1642,30 +1661,21 @@ def __init__(self, microscope):
self.postures = [SEM_IMAGING, FM_IMAGING]
if model.MD_FAV_MILL_POS_ACTIVE in stage_md:
self.postures.append(MILLING)
self._initialise_transformation(axes=["y", "z"], rotation=self.pre_tilt)

self.linked_axes = ["y", "z"]
# The setter of self.milling_angle sets the milling angle metadata, which is used to update the transformation parameters.
self.milling_angle.subscribe(self._initialise_transformation, init=True)
self.create_sample_stage()

# Update the posture based on the actual metadata
self._update_posture(self.stage.position.value)

def _initialise_transformation(
self,
axes: Sequence[str],
rotation: float = 0,
scale: tuple = (1, 1),
translation: tuple = (0, 0),
shear: tuple = (0, 0),
):
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()
Comment on lines +1673 to 1679
Copy link

@coderabbitai coderabbitai bot Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, locate and read the file
head -n 1680 src/odemis/acq/move.py | tail -n 10

Repository: delmic/odemis

Length of output: 532

🏁 Script executed:

# Get more context around the function
sed -n '1665,1690p' src/odemis/acq/move.py

Repository: 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 50

Repository: 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 10

Repository: 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.


def _update_conversion(self,
Expand Down Expand Up @@ -1704,7 +1714,6 @@ def _update_conversion(self,
tf_sem = tf_reverse @ tf_tilt @ tf_sr
tf_sem_inv = numpy.linalg.inv(tf_sem)

# TODO: update MILLING transformations when changing milling angle
if model.MD_FAV_MILL_POS_ACTIVE in stage_md:
mill_pos_active = stage_md[model.MD_FAV_MILL_POS_ACTIVE]
rx_mill = calculate_stage_tilt_from_milling_angle(milling_angle=mill_pos_active["mill_angle"],
Expand Down
43 changes: 41 additions & 2 deletions src/odemis/acq/test/move_tescan_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

import odemis
from odemis import model
from odemis.acq.move import (FM_IMAGING, SEM_IMAGING, UNKNOWN, MicroscopePostureManager, LOADING)
from odemis.acq.move import (FM_IMAGING, SEM_IMAGING, UNKNOWN, MeteorTescan1PostureManager, LOADING, MILLING)
from odemis.acq.test.move_tfs1_test import TestMeteorTFS1Move
from odemis.acq.test.move_tfs3_test import TestMeteorTFS3Move
from odemis.util import testing
Expand Down Expand Up @@ -175,7 +175,7 @@ class TestMeteorTescan1FibsemMove(TestMeteorTFS3Move):
def setUpClass(cls):
testing.start_backend(cls.MIC_CONFIG)
cls.microscope = model.getMicroscope()
cls.pm = MicroscopePostureManager(microscope=cls.microscope)
cls.pm = MeteorTescan1PostureManager(microscope=cls.microscope)
Copy link
Contributor Author

@tmoerkerken tmoerkerken Feb 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pieleric was there a reason you decided not to use the Tescan posture manager? You foresee any problems changing it? I need it for the milling angle callback.


# get the stage components
cls.stage_bare = model.getComponent(role="stage-bare")
Expand Down Expand Up @@ -236,5 +236,44 @@ def test_rel_move_fm_posture(self):
exp_bare_pos[axis] += m_bare[axis]
testing.assert_pos_almost_equal(new_bare_pos, exp_bare_pos, atol=1e-6)

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)
Comment on lines 249 to 264
Copy link

@coderabbitai coderabbitai bot Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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_angle

Or 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.

# 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)

Comment on lines +239 to +276
Copy link

@coderabbitai coderabbitai bot Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Check if the file exists and examine the test methods
find . -name "move_tescan_test.py" -type f

Repository: 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
fi

Repository: 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
fi

Repository: 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
fi

Repository: 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.


if __name__ == "__main__":
unittest.main()
111 changes: 59 additions & 52 deletions src/odemis/gui/cont/tabs/fibsem_tab.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import logging
import math
from typing import Dict
from concurrent.futures import CancelledError

import numpy
import wx
Expand All @@ -42,7 +43,7 @@
StaticSEMStream,
)
from odemis.gui import conf
from odemis.gui.comp.buttons import BTN_TOGGLE_COMPLETE, BTN_TOGGLE_OFF
from odemis.gui.comp.buttons import BTN_TOGGLE_COMPLETE, BTN_TOGGLE_OFF, BTN_TOGGLE_PROGRESS
from odemis.gui.conf.licences import LICENCE_FIBSEM_ENABLED, LICENCE_MILLING_ENABLED
from odemis.gui.cont import milling, settings
from odemis.gui.cont.features import CryoFeatureController
Expand Down Expand Up @@ -188,15 +189,19 @@ def __init__(self, name, button, panel, main_frame, main_data):
self.pm.stage.position.subscribe(self._on_stage_pos, init=True)
self.panel = panel

self._posture_switch_future = model.InstantaneousFuture()

rx = self.pm.stage.getMetadata()[model.MD_FAV_MILL_POS_ACTIVE]["mill_angle"]
self.panel.ctrl_milling_angle.SetValue(math.degrees(rx))
self.panel.ctrl_milling_angle.SetValueRange(*MILLING_RANGE)
# Tilt field is in degrees, so convert from radians to integer degrees to prevent a lot of decimals from showing
self.panel.ctrl_milling_angle.SetValueRange(*numpy.round(numpy.rad2deg(MILLING_RANGE)).astype(int))
self.panel.ctrl_milling_angle.Bind(wx.EVT_COMMAND_ENTER, self._update_milling_angle)
self._update_milling_angle(None)
self.panel.btn_switch_milling.Bind(wx.EVT_BUTTON, self._move_to_milling_position)
self.panel.btn_switch_sem_imaging.Bind(wx.EVT_BUTTON, self._move_to_sem)
self.panel.btn_switch_milling.Bind(wx.EVT_BUTTON, self._move_to_milling_posture)
self.panel.btn_switch_sem_imaging.Bind(wx.EVT_BUTTON, self._move_to_sem_posture)
self.tab_data_model.streams.subscribe(self._on_acquired_streams)

@call_in_wx_main
def _on_view(self, view):
"""Hide/Disable milling controls when fib view is not selected"""
# is_fib_view = issubclass(view.stream_classes, FIBStream)
Expand Down Expand Up @@ -394,68 +399,70 @@ def _on_stage_pos(self, pos: Dict[str, float]) -> None:
logging.warning("Failed to update stage position label: %s", e)

# update the stage position buttons
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 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)
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)
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()

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")

Comment on lines 418 to 466
Copy link

@coderabbitai coderabbitai bot Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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
fi

Repository: 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.

def terminate(self):
self.main_data.stage.position.unsubscribe(self._on_stage_pos)
Expand Down