Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1929 +/- ##
=======================================
Coverage 99.08% 99.09%
=======================================
Files 319 320 +1
Lines 12408 12506 +98
=======================================
+ Hits 12295 12393 +98
Misses 113 113 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Sorry to jump in here, but I am working on a Secondly, I don't think you should be altering the existing |
jacob720
left a comment
There was a problem hiding this comment.
Looks good! Logic is clean and easy to follow
| self.omega = Motor(prefix + omega_infix) | ||
| real_motor = Motor(prefix + omega_infix) | ||
| self.omega = real_motor | ||
| self.omega_axis = WrappedAxis(self.omega) |
There was a problem hiding this comment.
Should something similar be added to XYZThetaStage or anything else with a rotational axis?
DominicOram
left a comment
There was a problem hiding this comment.
Thanks, some comments
| only come back after the motion on that axis finished. | ||
| """ | ||
| await self.defer_move.set(DeferMoves.ON) | ||
| # TODO something something i03 broken smargon serialise moves workaround |
There was a problem hiding this comment.
Should: Can this be an issue please?
| await axis.user_setpoint.get_value(), new_setpoint | ||
| ) | ||
| put_completion = await set_and_wait_for_value( | ||
| axis.user_setpoint, |
There was a problem hiding this comment.
Should: I don't really like the check against the name "omega" above, it would be much cleaner if we can make the omega object more like a thing that just handles the logic itself. I think if we change the phase signal to be user_setpoint in WrappedAxis then we can just remove the logic in here for getting the target value and we can set it like any other motor?
| self.omega = Motor(prefix + omega_infix) | ||
| real_motor = Motor(prefix + omega_infix) | ||
| self.omega = real_motor | ||
| self.omega_axis = WrappedAxis(self.omega) |
There was a problem hiding this comment.
Should: I agree with @oliwenmandiamond that we shouldn't be polluting the common XYZOmegaStage with this logic. We should do that overriding when we instantiate the smargon. Maybe the cleanest way to do this is to change these collection classes to protocols?
There was a problem hiding this comment.
I think having a protocol is overkill as were effectively defining the device twice, more boilerplate.
It should be done like this:
class MyCustomStage(XYZOmegaStage):
def __init__(
self,
prefix: str,
name: str = "",
x_infix: str = _X,
y_infix: str = _Y,
z_infix: str = _Z,
omega_infix: str = _OMEGA,
) -> None:
super().__init__(prefix, name, x_infix, y_infix, z_infix, omega_infix)
with self.add_children_as_readables():
self.omega_axis = WrappedAxis(self.omega)This is how every beamline has defined stage. If it is common and generic, it goes in dodal/devices/motors.py.
If it is beamline specific, it should be defined as dodal/devices/beamlines/iXX/my_device.py. For example i05, https://github.com/DiamondLightSource/dodal/blob/main/src/dodal/devices/beamlines/i05/i05_motors.py and i05_1 https://github.com/DiamondLightSource/dodal/blob/main/src/dodal/devices/beamlines/i05_1/i05_1_motors.py
If this is used mx wide, it should go in dodal/devices/my_mx_specific_device.py
There was a problem hiding this comment.
I think it should be a protocol 'cos I think we should be overriding omega completely, rather than creating a second wrapped axis. I think this helps downstream plans not worry so much about this logic and just use omega, making them more generic across MX
There was a problem hiding this comment.
Okay, I see. I was referring to the current implementation.
Yes a protocol could work. If this could wait for [StandardMovable](https://github.com/bluesky/ophyd-async/pull/1200), you could make the protocol
class XYZOmega(Protocol):
x: StandardMovable
y: StandardMovable
z: StandardMovable
omega: StandardMovableThen you could implement your own MovableLogic rather than use the MotorMovableLogic, then it doesn't matter what the implementation is for the plan.
There was a problem hiding this comment.
I think that making omega motor use a wrapped coordinate system is a Bad Idea - this would be a potentially breaking change for plans that use this motor, and the changes might not be obvious to users that are not consciously considering the implications.
I have updated the PR and now all the plans that are in use by Hyperion are wrapped-omega aware, so a change to make omega a wrapped axis would be relatively trivial. However there would potentially be an impact on other beamlines if they inherit from XYZOmegaStage.
I don't think that Motors with wrapped coordinate systems are appropriate, since speed limits and motor limits don't make sense within the wrapped coordinate space. Certainly it is not a drop-in replacement for the current unwrapped coordinate motor for all the reasons I have listed in wrapped_axis.py and I would not be in favour of it.
If you really want to replace omega with the wrapped equivalent, then I would strongly advise doing it in a device class that is not going to be used outside of MX to minimise the impact. Currently some of the MX plans are used by e.g. the aithre pin tip centring which do not have a standard gonio, therefore the base class we accept is XYZOmegaStage so at a minimum they would need to use whatever base device class we decide to use, and they would have to ensure their plans weren't affected by a change to a wrapped omega. Since having access to only the phase angle represents a loss of information, unless all impacted plans can be adapted to use only the phase angle, it is likely that it will still be necessary to provide access to the underlying unwrapped angle.
As it currently stands adding a new wrapped axis is essentially free to any device and has no implications other than requiring the underlying axis to have unconstrained rotation, and the possibility that wrapped-unaware plans calling wrapped-aware plans may sometimes find the axis 360 degrees from where they might have expected it, which for most cases will not be an issue.
On the other hand, it is also possible to argue that we could do away with WrappedAxis altogether since you can always compute everything from the unwrapped coordinate space. However I do think it is nice to be able to have the phase angle accessible at the device level as a convenience to set directly (since for the majority of moves, this is the coordinate space of interest), and to expose as a Readable in bluesky events, as well as making it more obvious in the code when we are considering phase angle as opposed to absolute angle.
| return value | ||
|
|
||
| @AsyncStatus.wrap | ||
| async def set(self, value: CombinedMove): |
There was a problem hiding this comment.
Should: I'm not sure I like that the co-ordinate system of this set is different to that of omega.set. I think it is possible to get the underlying motor object to always be moving based on phase.
There was a problem hiding this comment.
I think making the underlying motor object represent phase is a bad idea for reasons I will state elsewhere in this PR, however I agree that having the omega coordinate as the only wrapped value in CombinedMove is somewhat ugly. The way I see, it the options are:
CombinedMove.omegais unwrapped and the plan unwraps the coordinate before specifying it - also ugly, and means you have to read the current position in the plan.CombinedMove.omega_phasebecomes a thing and you can specify the phase there, specifyingomega_phase- We remove support for setting omega from
CombinedMove- following Smargon combined moves should perform omega moves separately #1998 it seems the consensus is that omega moves should be serialized anyway because we've decided there are likely hardware limitations that mean we can't reliably do simultaneous fast omega moves at the same time as other axes. I think this is what I will ultimately do in the follow-on PR - We implement a separate move_to_robot_load which just moves to 0,0,0,0,0,mod-360 - since this is the only time we need to do the combined move apart from moving to the next xtal. We could do this in conjunction with # 3 if we want to make this an explicit smargon feature, but I think it's not strictly necessary.
| class WrappedAxis(StandardReadable): | ||
| def __init__(self, real_motor: Motor, name=""): | ||
| self._real_motor = Reference(real_motor) | ||
| with self.add_children_as_readables(StandardReadableFormat.HINTED_SIGNAL): | ||
| self.offset_and_phase = derived_signal_rw( | ||
| self._get_motor_offset_and_phase, | ||
| self._set_motor_offset_and_phase, | ||
| motor_pos=real_motor, | ||
| ) | ||
| with self.add_children_as_readables(): | ||
| self.phase = derived_signal_rw( | ||
| self._get_phase, self._set_phase, offset_and_phase=self.offset_and_phase | ||
| ) | ||
| super().__init__(name=name) | ||
|
|
||
| def _get_motor_offset_and_phase(self, motor_pos: float) -> MotorOffsetAndPhase: | ||
| angle = AngleWithPhase.wrap(motor_pos) | ||
| return np.array([angle.offset, angle.phase]) | ||
|
|
||
| async def _set_motor_offset_and_phase(self, value: MotorOffsetAndPhase): | ||
| await self._real_motor().set( | ||
| AngleWithPhase.from_offset_and_phase(value).unwrap() | ||
| ) | ||
|
|
||
| def _get_phase(self, offset_and_phase: MotorOffsetAndPhase) -> float: | ||
| return offset_and_phase[1].item() | ||
|
|
||
| async def _set_phase(self, value: float): | ||
| """Set the motor phase to the specified phase value in degrees. | ||
| The motor will travel via the shortest distance path. | ||
| """ | ||
| offset_and_phase = await self.offset_and_phase.get_value() | ||
| current_position = AngleWithPhase.from_offset_and_phase(offset_and_phase) | ||
| target_value = current_position.nearest_to_phase(value).unwrap() | ||
| await self._real_motor().set(target_value) | ||
|
|
||
| def distance(self, theta1_deg: float, theta2_deg: float) -> float: | ||
| """Obtain the shortest distance between theta2 and theta1 in degrees. | ||
| This will be the shortest distance in mod360 space (i.e. always <= 180 degrees). | ||
| """ | ||
| return AngleWithPhase.wrap(theta1_deg).phase_distance( | ||
| AngleWithPhase.wrap(theta2_deg) | ||
| ) |
There was a problem hiding this comment.
This isn't common maths. This is a device and should live with where you define your custom stage.
There was a problem hiding this comment.
Thanks for separating it out from the maths. However, the wrapped_axis.,py still needs to live under dodal/devices/wrapped_axis.py as it is a device.
585d794 to
a95aa88
Compare
a95aa88 to
b610d52
Compare
| def values_for_rotation(request): | ||
| input_value, current_real_value, expected_real_value = request.param | ||
| yield input_value, current_real_value, expected_real_value | ||
|
|
There was a problem hiding this comment.
| def values_for_rotation(request): | |
| input_value, current_real_value, expected_real_value = request.param | |
| yield input_value, current_real_value, expected_real_value | |
| def values_for_rotation(request: pytest.FixtureRequest) -> tuple[float, float, float]: | |
| input_value, current_real_value, expected_real_value = request.param | |
| yield input_value, current_real_value, expected_real_value |
| @pytest.fixture() | ||
| async def stage_in_initial_state(values_for_rotation): | ||
| input_value, current_real_value, expected_real_value = values_for_rotation | ||
|
|
||
| stage = XYZOmegaStage("BL03I-MO-SGON-01:") | ||
| await stage.connect(mock=True) | ||
| set_mock_value(stage.omega.user_readback, current_real_value) | ||
| return stage |
There was a problem hiding this comment.
| @pytest.fixture() | |
| async def stage_in_initial_state(values_for_rotation): | |
| input_value, current_real_value, expected_real_value = values_for_rotation | |
| stage = XYZOmegaStage("BL03I-MO-SGON-01:") | |
| await stage.connect(mock=True) | |
| set_mock_value(stage.omega.user_readback, current_real_value) | |
| return stage | |
| @pytest.fixture() | |
| async def stage_in_initial_state(values_for_rotation: tuple[float, float, float]) -> XYZOmegaStage: | |
| input_value, current_real_value, expected_real_value = values_for_rotation | |
| with init_devices(mock=True): | |
| stage_in_initial_state = XYZOmegaStage("BL03I-MO-SGON-01:") | |
| set_mock_value(stage_in_initial_state.omega.user_readback, current_real_value) | |
| return stage_in_initial_state |
|
|
||
| async def test_mod_360_expected_direction_of_rotation_same_as_apparent_for_moves_apparently_less_than_180( | ||
| values_for_rotation, stage_in_initial_state | ||
| ): |
There was a problem hiding this comment.
| async def test_mod_360_expected_direction_of_rotation_same_as_apparent_for_moves_apparently_less_than_180( | |
| values_for_rotation, stage_in_initial_state | |
| ): | |
| async def test_mod_360_expected_direction_of_rotation_same_as_apparent_for_moves_apparently_less_than_180( | |
| values_for_rotation: tuple[float, float, float], stage_in_initial_state: XYZOmegaStage | |
| ) -> None: |
|
|
||
| def test_smargon_deferred_moves_move_omega_phase_not_absolute(): | ||
| pass |
Change calculate_motion_profile to start at the neareset omega
e2e258a to
80649ec
Compare
oliwenmandiamond
left a comment
There was a problem hiding this comment.
This is looking better, thanks. Still needs a bit more work though. Please see my comments but in summary, XYZWrappedOmegaStage appears to be an unnecessary layer. Instead just wrap the axis directly on the device you want it on such as Smargon. Type checking on the tests also need to be improved
| class AngleWithPhase: | ||
| """Represents a point in an absolute rotational space which is defined by a phase where 0<=phase<360 | ||
| and an offset from an origin where the absolute coordinate is the sum of the phase and the offset. | ||
|
|
||
| Attributes: | ||
| offset: The offset of 0 phase from some other unwrapped rotational coordinate space | ||
| phase: The phase in degrees relative to this offset. | ||
| """ | ||
|
|
||
| @overload | ||
| def __init__(self, offset_and_phase: Iterable[float], /): | ||
| pass # pragma: no cover | ||
|
|
||
| @overload | ||
| def __init__(self, offset: float, phase: float, /): | ||
| pass # pragma: no cover | ||
|
|
||
| def __init__(self, offset: float | Iterable[float], phase: float = 0): | ||
| """Construct a normalised representation of the offset and phase, such that | ||
| 0 <= phase < 360. | ||
|
|
||
| Args: | ||
| offset (float | Iterable[float]): the offset in degrees, or the | ||
| offset and phase as a list or other iterable | ||
| phase (float): the phase in degrees | ||
| """ | ||
| if isinstance(offset, Iterable): | ||
| offset, phase = offset | ||
| correction = 360 * (phase // 360) | ||
| self.offset: float = offset + correction | ||
| self.phase: float = phase - correction |
There was a problem hiding this comment.
I think this is a better fit for a dataclass
| class AngleWithPhase: | |
| """Represents a point in an absolute rotational space which is defined by a phase where 0<=phase<360 | |
| and an offset from an origin where the absolute coordinate is the sum of the phase and the offset. | |
| Attributes: | |
| offset: The offset of 0 phase from some other unwrapped rotational coordinate space | |
| phase: The phase in degrees relative to this offset. | |
| """ | |
| @overload | |
| def __init__(self, offset_and_phase: Iterable[float], /): | |
| pass # pragma: no cover | |
| @overload | |
| def __init__(self, offset: float, phase: float, /): | |
| pass # pragma: no cover | |
| def __init__(self, offset: float | Iterable[float], phase: float = 0): | |
| """Construct a normalised representation of the offset and phase, such that | |
| 0 <= phase < 360. | |
| Args: | |
| offset (float | Iterable[float]): the offset in degrees, or the | |
| offset and phase as a list or other iterable | |
| phase (float): the phase in degrees | |
| """ | |
| if isinstance(offset, Iterable): | |
| offset, phase = offset | |
| correction = 360 * (phase // 360) | |
| self.offset: float = offset + correction | |
| self.phase: float = phase - correction | |
| @dataclass | |
| class AngleWithPhase: | |
| """Represents a point in an absolute rotational space which is defined by a phase where 0<=phase<360 | |
| and an offset from an origin where the absolute coordinate is the sum of the phase and the offset. | |
| Attributes: | |
| offset: The offset of 0 phase from some other unwrapped rotational coordinate space | |
| phase: The phase in degrees relative to this offset. | |
| """ | |
| offset: float | |
| phase: float = 0.0 | |
| def __post_init__(self) -> None: | |
| correction = 360 * (self.phase // 360) | |
| self.offset += correction | |
| self.phase -= correction | |
| @classmethod | |
| def from_iterable(cls, values: Iterable[float]) -> Self: | |
| """Construct a normalised representation of the offset and phase, such that | |
| 0 <= phase < 360. | |
| Args: | |
| offset Iterable[float]: the offset and phase as a list or other iterable | |
| """ | |
| offset, phase = values | |
| return cls(offset, phase) |
| [-10000 * 360 - 0.001, 359.999], | ||
| ], | ||
| ) | ||
| async def test_mod_360_read(real_value: float, expected_phase): |
There was a problem hiding this comment.
| async def test_mod_360_read(real_value: float, expected_phase): | |
| async def test_mod_360_read(xyz_wrapped_omege_stage: XYZWrappedOmegaStage, real_value: float, expected_phase: float): |
Why don't we use the already created stage?
| with self.add_children_as_readables(): | ||
| self.phase = derived_signal_rw( | ||
| self._get_phase, self._set_phase, offset_and_phase=self.offset_and_phase | ||
| ) |
There was a problem hiding this comment.
We have a signal that has offset_and_phase, why do we need phase as well which gives the same information?
There was a problem hiding this comment.
phase is very convenient to directly read and write from plans, most plans are not interested in absolute position, but the extra information is useful when you are.
There was a problem hiding this comment.
Would it not make more sense to just have one signal for offset and one for phase?
|
|
||
|
|
||
| class Smargon(XYZOmegaStage, Movable): | ||
| class Smargon(XYZWrappedOmegaStage, Movable): |
There was a problem hiding this comment.
This is the only device that uses the wrapped omega axis. Rather than creating XYZWrappedOmegaStage, you should change it so Smargon still uses XYZOmegaStage and then just wrap the omega axis directly here.
class Smargon(XYZWrappedOmegaStage, Movable[float]):
def __init__(self, prefix: str, name: str = ""):
super().__init__(prefix=prefix, name=name)
with self.add_children_as_readables():
self.wrapped_omega = WrappedAxis(self.omega)
# Rest of smargon logic
...There was a problem hiding this comment.
Goniometer also inherits from XYZWrappedOmegaStage.
Goniometer is used in the same pin tip centring plan that is used with the Smargon, and which following DiamondLightSource/mx-bluesky#1640 does use the wrapped axis, so we need a common abstraction.
There was a problem hiding this comment.
Hmmm. My preference would be to still use XYZOmegaStage, and then have your plan include a parameter called wrapped_axis or wrapped_omega. So it looks like this:
@pydantic.dataclasses.dataclass(config={"arbitrary_types_allowed": True})
class PinTipCentringComposite:
"""All devices which are directly or indirectly required by this plan"""
oav: OAV
gonio: XYZOmegaStage
gonio_wrapped_axis: WrappedAxis
pin_tip_detection: PinTipDetectionThe reason being is this makes it way more generic. Any axis can now be wrapped. This means we don't need to create a new subclass for every possible stage combination which needs an axis wrapped.
However, looking at it more I'm not sure this would be supported by BlueAPI as it is a child device and from what I can tell, composites are extracted from being a unique type (correct me if wrong?).
|
|
||
|
|
||
| class Goniometer(XYZOmegaStage): | ||
| class Goniometer(XYZWrappedOmegaStage): |
There was a problem hiding this comment.
| class Goniometer(XYZWrappedOmegaStage): | |
| class Goniometer(XYZOmegaStage): | |
| def( | |
| self, | |
| prefix: str, | |
| name: str = "" | |
| ... | |
| ): | |
| super().__init__(prefix=prefix, name=name, ...) | |
| with self.add_children_as_readables(): | |
| self.wrapped_omega = WrappedAxis(self.omega) | |
| ... |
There was a problem hiding this comment.
See my comment below re Smargon
Co-authored-by: oliwenmandiamond <136330507+oliwenmandiamond@users.noreply.github.com>
oliwenmandiamond
left a comment
There was a problem hiding this comment.
This looks much better now, thanks! I just have a couple questions
| with self.add_children_as_readables(): | ||
| self.phase = derived_signal_rw( | ||
| self._get_phase, self._set_phase, offset_and_phase=self.offset_and_phase | ||
| ) |
There was a problem hiding this comment.
Would it not make more sense to just have one signal for offset and one for phase?
|
|
||
|
|
||
| class Smargon(XYZOmegaStage, Movable): | ||
| class Smargon(XYZWrappedOmegaStage, Movable): |
There was a problem hiding this comment.
Hmmm. My preference would be to still use XYZOmegaStage, and then have your plan include a parameter called wrapped_axis or wrapped_omega. So it looks like this:
@pydantic.dataclasses.dataclass(config={"arbitrary_types_allowed": True})
class PinTipCentringComposite:
"""All devices which are directly or indirectly required by this plan"""
oav: OAV
gonio: XYZOmegaStage
gonio_wrapped_axis: WrappedAxis
pin_tip_detection: PinTipDetectionThe reason being is this makes it way more generic. Any axis can now be wrapped. This means we don't need to create a new subclass for every possible stage combination which needs an axis wrapped.
However, looking at it more I'm not sure this would be supported by BlueAPI as it is a child device and from what I can tell, composites are extracted from being a unique type (correct me if wrong?).
Required for
Introduces a
wrapped_omegaReadabletoXYZOmegaStagewhich delegates motion to the underlying omega motor.wrapped_omegahas two signals:phasewhich is read-write and represents the phase angle with respect to an offset, which is a multiple of 360 degrees.offset_and_phasewhich returns a numpy array containing the offset and phase values together. This can be retrieved in plans and events and used to instantiate an instance ofAngleWithPhasewhich can be used as a convenient value-object helper for conversion and manipulation of phase angles, while retaining information about the mapping to the underlying unwrapped real coordinate.Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}