Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
30 changes: 5 additions & 25 deletions src/dodal/devices/aithre_lasershaping/goniometer.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import asyncio
import math

from ophyd_async.core import StandardReadable, derived_signal_rw
from ophyd_async.core import StandardReadable
from ophyd_async.epics.motor import Motor

from dodal.devices.motors import create_axis_perp_to_rotation


class Goniometer(StandardReadable):
"""The Aithre lab goniometer and the XYZ stage it sits on.
Expand All @@ -24,26 +23,7 @@ def __init__(self, prefix: str, name: str = "") -> None:
self.sampy = Motor(prefix + "SAMPY")
self.sampz = Motor(prefix + "SAMPZ")
self.omega = Motor(prefix + "OMEGA")
self.vertical_position = derived_signal_rw(
self._get,
self._set,
sampy=self.sampy,
sampz=self.sampz,
omega=self.omega,
self.vertical_position = create_axis_perp_to_rotation(
self.sampz, self.sampy, self.omega
)
super().__init__(name)

def _get(self, sampz: float, sampy: float, omega: float) -> float:
z_component = sampz * math.cos(math.radians(omega))
y_component = sampy * math.sin(math.radians(omega))
return z_component + y_component

async def _set(self, value: float) -> None:
omega = await self.omega.user_readback.get_value()
z_component = value * math.cos(math.radians(omega))
y_component = value * math.sin(math.radians(omega))
await asyncio.gather(
self.sampy.set(y_component),
self.sampz.set(z_component),
self.omega.set(omega),
)
82 changes: 81 additions & 1 deletion src/dodal/devices/motors.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,57 @@
from ophyd_async.core import StandardReadable
import asyncio
import math

from ophyd_async.core import StandardReadable, derived_signal_rw
from ophyd_async.epics.motor import Motor


def create_axis_perp_to_rotation(
parallel_to_0: Motor, parallel_to_minus_90: Motor, rotation: Motor
):
"""Create a new derived signal that moves perpendicular to a rotation axis.

The usual use case for this is translating from sample space to lab space. For
example, if you have a sample that is mounted on a goniometer to the right hand side
of an OAV view this can provide an axis that will move the sample up/down in that
view regardless of the omega orientation of the sample.

Args:
parallel_to_0 (Motor): this is the axis that, when the sample is at 0 deg rotation,
a move here is entirely parallel with the derived axis.
parallel_to_minus_90 (Motor): this is the axis that, when the sample is at 90 deg
Copy link
Contributor

Choose a reason for hiding this comment

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

"when the sample is at 90 deg" this is a confusing sentence and seemingly contradictory to the argument name.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I think this doc comment can be made clearer. For me, I find the naming of the args a bit confusing, because the frame of reference of the angle in the name isn't clear and is contradicted by the arg description (although I think I understand your intent with the sign flip, it is I think unnecessarily confusing).

I think it's better to describe the geometric assumptions in the doc comment, and label the axes neutrally, because this function is applicable to a variety of different geometries and labelling the axes things like whether any given axis is right- or left- handed, up or down etc are essentially not relevant, as long as the inputs conform to the documented requirements it should give the right answer.

While I was reviewing this, to make it easier for myself to follow the implementation, I renamed the args motor_i, motor_j and motor_theta. I tried to come up with some wording for the comment that would summarise less ambiguously what the function does, what I came up with was this:

Given a signal that controls a motor in a rotation axis and two other
signals controlling motors on a pair of orthogonal axes, these axes being in the
rotating frame of reference created by the first axis,
create a derived signal that is a projection of the two axes in the non-rotating
frame of reference.

The projection is onto the axis defined by i when the rotation angle is 0.

For me I think this is clearer, although it is quite technical and I think you should retain the illustrative text in the comment because not everyone prefers this kind of explanation and it is generally helpful. Also I think my sentence structure is still a bit clunky and overly long. Perhaps breaking it up into bullet points would help, I'm open to improvements in that regard. For me, the important facts are that:

  • There are two frames of reference, one is stationary, one is rotating
  • One input is a motor controls the angle between the two frames of reference
  • The other inputs are on orthogonal axes in the rotating frame, with the given orientations at specified angles of theta
  • The output is a motor which is a projection onto the stationary frame parallel to i when theta == 0

Also, I think it might be better if the rotation motor was moved to the first argument of the function, because it's the important thing that controls how the other two axes are interpreted.

@olliesilvester do you think my text above is helpful?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think your wording is a bit more clear. I would nitpick it slightly to

    Given a signal that controls a motor in a rotation axis and two other
    signals controlling motors on a pair of orthogonal axes, these axes being in the
    rotating frame of reference created by the first axis,
    create a derived signal that is a projection of the two orthogonal axes and is in the lab
    frame of reference.

    The projection is onto the axis defined by i when the rotation angle is 0.

This function is going to be hard to understand however good the wording is. Ideally we would have a diagram to explain it

Copy link
Contributor

Choose a reason for hiding this comment

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

Completely agree, sounds a lot better. Thanks!

rotation, a move here is entirely parallel with the
derived axis.
rotation (Motor): this is the rotation axis of the sample.
"""
# By convention use y/z internally as that is what is used on most beamlines but the
# function is actually indifferent to this
y_mot = parallel_to_0
z_mot = parallel_to_minus_90

def _get(z_val: float, y_val: float, rot_value: float) -> float:
y_component = y_val * math.cos(math.radians(rot_value))
z_component = z_val * math.sin(math.radians(rot_value))
return z_component + y_component

async def _set(vertical_value: float) -> None:
rot_value = await rotation.user_readback.get_value()
y_component = vertical_value * math.cos(math.radians(rot_value))
z_component = vertical_value * math.sin(math.radians(rot_value))
await asyncio.gather(
y_mot.set(y_component),
z_mot.set(z_component),
rotation.set(rot_value),
)

return derived_signal_rw(
_get,
_set,
y_val=y_mot,
z_val=z_mot,
rot_value=rotation,
)


class XYZPositioner(StandardReadable):
"""

Expand Down Expand Up @@ -41,6 +91,31 @@ def __init__(


class SixAxisGonio(XYZPositioner):
"""

Six-axis goniometer with a standard xyz stage and three axes of rotation: kappa, phi
and omega.

Parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please use standard google pydoc notation here: https://google.github.io/styleguide/pyguide.html#38-comments-and-docstrings? It helps the generated API docs format nicely.

You can also use doctest style examples: https://docs.python.org/3/library/doctest.html
The advantage is that the examples then get checked for correctness which helps make sure that the docstrings stay up to date. However although the formatting is nice if you don't want the tests to actually run, then you can use # doctest: +SKIP

----------
prefix:
EPICS PV (Common part up to and including :).
name:
name for the stage.
infix:
EPICS PV, default is the ("X", "Y", "Z", "KAPPA", "PHI", "OMEGA").
Notes
-----
Example usage::
async with init_devices():
xyz_stage = XYZPositioner("BLXX-MO-STAGE-XX:")
Or::
with init_devices():
xyz_stage = XYZPositioner("BLXX-MO-STAGE-XX:", infix = ("A", "B", "C", \
"KAPPA", "PHI", "OMEGA"))

"""

def __init__(
self,
prefix: str,
Expand All @@ -58,4 +133,9 @@ def __init__(
self.kappa = Motor(prefix + infix[3])
self.phi = Motor(prefix + infix[4])
self.omega = Motor(prefix + infix[5])

super().__init__(name=name, prefix=prefix, infix=infix[0:3])

self.vertical_in_lab_space = create_axis_perp_to_rotation(
self.y, self.z, self.omega
)
105 changes: 104 additions & 1 deletion tests/devices/unit_tests/test_motors.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import math
from unittest.mock import ANY

import pytest
Expand All @@ -6,12 +7,29 @@
from ophyd_async.testing import assert_reading

from dodal.devices.motors import SixAxisGonio
from dodal.devices.util.test_utils import patch_motor


@pytest.fixture
def six_axis_gonio(RE: RunEngine):
def six_axis_gonio(RE: RunEngine) -> SixAxisGonio:
with init_devices(mock=True):
gonio = SixAxisGonio("")
patch_motor(gonio.omega)
patch_motor(gonio.z)
patch_motor(gonio.y)
patch_motor(gonio.x)

return gonio


Copy link
Contributor

Choose a reason for hiding this comment

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

These fixtures are the same?

@pytest.fixture
def y_up_six_axis_gonio(RE: RunEngine) -> SixAxisGonio:
with init_devices(mock=True):
gonio = SixAxisGonio("")
patch_motor(gonio.omega)
patch_motor(gonio.z)
patch_motor(gonio.y)
patch_motor(gonio.x)

return gonio

Expand Down Expand Up @@ -50,5 +68,90 @@ async def test_reading_six_axis_gonio(six_axis_gonio: SixAxisGonio):
"timestamp": ANY,
"alarm_severity": 0,
},
"gonio-_horizontal_stage_axis": {
"alarm_severity": 0,
"timestamp": ANY,
"value": 0.0,
},
"gonio-_vertical_stage_axis": {
"alarm_severity": 0,
"timestamp": ANY,
"value": 0.0,
},
},
)


@pytest.mark.parametrize(
"set_value, omega_set_value, expected_horz, expected_vert",
[
[2, 60, math.sqrt(3), 1],
[-10, 390, -5, -5 * math.sqrt(3)],
[0.5, -135, -math.sqrt(2) / 4, -math.sqrt(2) / 4],
[1, 0, 0, 1],
],
)
async def test_vertical_in_lab_space_for_default_axes(
six_axis_gonio: SixAxisGonio,
set_value: float,
omega_set_value: float,
expected_horz: float,
expected_vert: float,
):
await six_axis_gonio.omega.set(omega_set_value)
await six_axis_gonio.vertical_in_lab_space.set(set_value)

assert await six_axis_gonio.z.user_readback.get_value() == pytest.approx(
expected_horz
)
assert await six_axis_gonio.y.user_readback.get_value() == pytest.approx(
expected_vert
)

await six_axis_gonio.vertical_in_lab_space.set(set_value * 2)
assert await six_axis_gonio.z.user_readback.get_value() == pytest.approx(
expected_horz * 2
)
assert await six_axis_gonio.y.user_readback.get_value() == pytest.approx(
expected_vert * 2
)


@pytest.mark.parametrize(
"set_value, omega_set_value, expected_horz, expected_vert",
[
[2, 60, 1, math.sqrt(3)],
[-10, 390, -5 * math.sqrt(3), -5],
[0.5, -135, -math.sqrt(2) / 4, -math.sqrt(2) / 4],
[1, 0, 1, 0],
],
)
async def test_j_signal_for_rotated_axes(
y_up_six_axis_gonio: SixAxisGonio,
set_value: float,
omega_set_value: float,
expected_horz: float,
expected_vert: float,
):
await y_up_six_axis_gonio.omega.set(omega_set_value)
await y_up_six_axis_gonio.vertical_in_lab_space.set(set_value)

assert await y_up_six_axis_gonio.y.user_readback.get_value() == pytest.approx(
expected_horz
)
assert await y_up_six_axis_gonio.z.user_readback.get_value() == pytest.approx(
expected_vert
)

await y_up_six_axis_gonio.vertical_in_lab_space.set(set_value * 2)
assert await y_up_six_axis_gonio.y.user_readback.get_value() == pytest.approx(
expected_horz * 2
)
assert await y_up_six_axis_gonio.z.user_readback.get_value() == pytest.approx(
expected_vert * 2
)


async def test_get_j(y_up_six_axis_gonio: SixAxisGonio):
await y_up_six_axis_gonio.vertical_in_lab_space.set(5)
assert await y_up_six_axis_gonio.vertical_in_lab_space.get_value() == 5
Loading