(#391) Create an ophyd-async ApertureScatterguard device #419

katesmith280 · 2024-04-04T15:40:29Z

Fixes #391

Instructions to reviewer on how to test:

Run tests

Checks for reviewer

Would the PR title make sense to a scientist on a set of release notes
If a new device has been added does it follow the standards

…avid!

codecov · 2024-04-05T08:05:13Z

Codecov Report

Attention: Patch coverage is 98.55072% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 92.82%. Comparing base (3745ab3) to head (418de3d).
Report is 61 commits behind head on main.

❗ Current head 418de3d differs from pull request most recent head b2ef997. Consider uploading reports for the commit b2ef997 to get more accurate results

Files	Patch %	Lines
src/dodal/devices/aperturescatterguard.py	97.61%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #419      +/-   ##
==========================================
- Coverage   93.28%   92.82%   -0.46%     
==========================================
  Files          92       90       -2     
  Lines        3441     3332     -109     
==========================================
- Hits         3210     3093     -117     
- Misses        231      239       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…_desired_transmission Move transmission to the experiment param base class

DominicOram · 2024-04-05T10:35:12Z

src/dodal/devices/util/motor_utils.py



 class ExtendedEpicsMotor(EpicsMotor):
    motor_resolution: Component[EpicsSignalRO] = Component(EpicsSignalRO, ".MRES")
    max_velocity: Component[EpicsSignalRO] = Component(EpicsSignalRO, ".VMAX")
+
+
+class ExtendedMotor(Motor):


I forgot that I had done some work to get these into ophy-async ages ago. I've revived it at bluesky/ophyd-async#174. Let's not wait for that to be merged though, we can merge this in here and remove later

Still trying to get this into ophyd-async

dperl-dls

Almost there, just one more thing really - I put some comments in the code to explain. We should make sure that the set method conforms to what bluesky expects by making the device Movable and not only Readable

This change should be really small but please add one more test which checks that we can use the device in a plan in the RunEngine. If you write this test first it should fail because the RunEngine expects set() to return a Status of some kind

It will look something roughly like:

import bluesky.plan_stubs as bps
def test_ap_sg_in_runengine(aperture_in_medium_pos: ApertureScatterguard, RE: RunEngine):
    test_position = ... # I'll leave this to you
    RE(bps.abs_set(aperture_in_medium_pos, test_position, wait=True)
    # assert that positions afterwards are equal to specified in the test position

dperl-dls · 2024-04-05T12:27:44Z

src/dodal/devices/aperturescatterguard.py

@@ -1,7 +1,6 @@
 import asyncio
 from collections import namedtuple
 from dataclasses import dataclass
-from typing import List, Optional



you will want to from bluesky.protocols import Movable

dperl-dls · 2024-04-05T12:29:36Z

src/dodal/devices/aperturescatterguard.py

@@ -85,7 +84,7 @@ class ApertureScatterguard(StandardReadable):
    def __init__(self, prefix: str = "", name: str = "") -> None:


ApertureScatterguard should inherit from both StandardReadable but also Moveable, which defines what the signature for set() should look like

dperl-dls · 2024-04-05T12:30:57Z

src/dodal/devices/aperturescatterguard.py

-        return reduce(
-            operator.and_, [motor.set(pos) for motor, pos in zip(motors, positions)]
+    async def _set_raw_unsafe(self, positions: ApertureFiveDimensionalLocation):
+        """Accept the risks and move in an unsafe way. Collisions possible."""


love this docstring

src/dodal/devices/aperturescatterguard.py

katesmith280 · 2024-04-11T16:10:01Z

@dperl-dls thanks for all your help - hopefully we're on the home stretch!

I've merged the main branch into this one now and your test works great :-)

dperl-dls

Looks great! @DominicOram might want to take a quick look since I have had so much involvement in writing this, but as far as I can see this is good to merge. Let's squash the commits into one before merging it since there are a lot of WIPs and merges in there; you can do that directly from the github interface (I'll leave that for you to do so you get the well-deserved credit for it)

DominicOram

Great work, thank you @katesmith280! Couple of comments in code. More importantly (and annoyingly) this has broken Hyperion. You can test this if you run the tests on Hyperion main against this branch. I think most of the failures seem to be around the renaming of user_setpoint/user_readback in ophyd-async. This should be fixed in bluesky/ophyd-async#174. Maybe we wait on this to be merged first :/

DominicOram · 2024-04-15T08:55:47Z

src/dodal/devices/aperturescatterguard.py

+        # extendedepicsmotor class has tolerance fields added
+        # ophyd-async epics motor may need to do the same thing - epics motor


Should: I think we now have these fields on the ophyd-async equivalent, so this comment is redundant?

Comment is now removed

DominicOram · 2024-04-15T09:04:41Z

src/dodal/devices/util/motor_utils.py



 class ExtendedEpicsMotor(EpicsMotor):
    motor_resolution: Component[EpicsSignalRO] = Component(EpicsSignalRO, ".MRES")
    max_velocity: Component[EpicsSignalRO] = Component(EpicsSignalRO, ".VMAX")
+
+
+class ExtendedMotor(Motor):


Still trying to get this into ophyd-async

tests/devices/unit_tests/test_aperture_scatterguard.py

DominicOram · 2024-04-15T09:14:50Z

src/dodal/devices/aperturescatterguard.py

-        return reduce(
-            operator.and_, [motor.set(pos) for motor, pos in zip(motors, positions)]
+    async def _set_raw_unsafe(self, positions: ApertureFiveDimensionalLocation):
+        """Accept the risks and move in an unsafe way. Collisions possible."""


katesmith280 · 2024-04-15T11:47:56Z

Thanks for the review @DominicOram! I agree waiting so it doesn't break Hyperion is a good idea :-)

DominicOram · 2024-04-15T16:45:38Z

Ok, new motor is merged into ophyd_async. Would you mind updating this PR so it works against that @katesmith280?

katesmith280 · 2024-04-17T14:51:36Z

@DominicOram tested now against hyperion and it passes - hooray!

Should be good to merge if you think it's all good :-)

Changes were addressed

dperl-dls

Sorry, I got the wrong idea about what happened here. We don't need the ExtendedMotor at all anymore thanks to the recent change to the ophyd-async motor, they can all just be replaced with Motor, and the motor_resolution is now called deadband

katesmith280 · 2024-04-18T09:40:09Z

@dperl-dls good catch - my bad!

The ExtendedMotor is now removed and replaced with Motor, motor_readback is now also set to deadband, tests updated and have been run against dodal and hyperion repos - all tests for aperture scatterguard pass :-)

Stale

dperl-dls

Sorry this has taken so long, thanks for all your hard work on this @katesmith280 !

katesmith280 and others added 12 commits March 20, 2024 15:41

ophyd-async aperture and scatterguard first attempt

e4e5122

WIP help..

adaaf77

...

7b376be

work in progress, scatterguard extended motor, async/await

241414b

Work in progress async

2309790

WIP ophyd-asyncio .connect(sim=True) for unit-tests

661ec59

WIP more tests passing now we are updating position values - thanks D…

7d0f304

…avid!

WIP working on tests

ed2b34b

(#391) mock _move instead of set in ap_sg

9650bea

(#391) remove stray await

41ca74f

_set_raw_unsafe to asyncio.gather

41436a3

tidy up unused imports and old commented code block

32be1b5

katesmith280 requested a review from dperl-dls April 4, 2024 15:40

dperl-dls added 2 commits April 5, 2024 08:38

Merge branch 'main' into 391_aperture_scatterguard_ophyd_async

1b94847

placate linter

19a03bd

dperl-dls force-pushed the 391_aperture_scatterguard_ophyd_async branch 2 times, most recently from 0ebe4ae to b7dc905 Compare April 5, 2024 08:28

dperl-dls and others added 2 commits April 5, 2024 10:40

Merge pull request #394 from DiamondLightSource/hyperion_1033_clarify…

ad5ba13

…_desired_transmission Move transmission to the experiment param base class

(#391) fix logging and LUT tests

2de7fa1

dperl-dls force-pushed the 391_aperture_scatterguard_ophyd_async branch from 7cbcadd to 2de7fa1 Compare April 5, 2024 09:40

DominicOram reviewed Apr 5, 2024

View reviewed changes

dperl-dls added 2 commits April 5, 2024 11:41

Merge branch 'main' of github.com:DiamondLightSource/dodal

b6f7373

Merge branch 'main' into 391_aperture_scatterguard_ophyd_async

418de3d

dperl-dls requested changes Apr 5, 2024

View reviewed changes

katesmith280 and others added 3 commits April 10, 2024 10:00

WIP AsyncStatus bluseky tests

49fa716

(#391) fix test

7325b3b

Merge branch 'main' into 391_aperture_scatterguard_ophyd_async

bf1de01

dperl-dls changed the title ~~391 aperture scatterguard ophyd async~~ (#391) Create an ophyd-async ApertureScatterguard device Apr 12, 2024

dperl-dls added 2 commits April 12, 2024 08:40

(#391) update i03 test

cedcfd2

(#391) tidy up tests a little

5c5c97e

dperl-dls approved these changes Apr 12, 2024

View reviewed changes

DominicOram previously requested changes Apr 15, 2024

View reviewed changes

Remove print from tests and obseleted comment

9652718

update for compatability with ophyd-async breaking change 0e7e45b

211703f

Merge branch 'main' into 391_aperture_scatterguard_ophyd_async

852adb6

dperl-dls previously requested changes Apr 17, 2024

View reviewed changes

Removed ExtendedMotor for Motor and changed motor_resolution to deadband

991aaf0

katesmith280 and others added 5 commits April 18, 2024 10:28

remove old comment about deadband not in epics motor

73c1361

remove unnecessary if statement (no more ExtendedMotor)

a991d2f

Merge branch 'main' into 391_aperture_scatterguard_ophyd_async

3848acb

Remove unused imports

e173bbe

Tidy up tests a bit

1142f8b

Merge branch 'main' into 391_aperture_scatterguard_ophyd_async

b2ef997

dperl-dls approved these changes May 7, 2024

View reviewed changes

dperl-dls merged commit f863e24 into main May 7, 2024
11 checks passed

dperl-dls deleted the 391_aperture_scatterguard_ophyd_async branch May 7, 2024 10:38

dperl-dls mentioned this pull request May 7, 2024

Update for new ApertureScatterguard DiamondLightSource/hyperion#1354

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(#391) Create an ophyd-async ApertureScatterguard device #419

(#391) Create an ophyd-async ApertureScatterguard device #419

katesmith280 commented Apr 4, 2024

codecov bot commented Apr 5, 2024 •

edited

Loading

DominicOram Apr 5, 2024

DominicOram Apr 15, 2024

dperl-dls left a comment

dperl-dls Apr 5, 2024

dperl-dls Apr 5, 2024

dperl-dls Apr 5, 2024

DominicOram Apr 15, 2024

katesmith280 commented Apr 11, 2024 •

edited

Loading

dperl-dls left a comment •

edited

Loading

DominicOram left a comment

DominicOram Apr 15, 2024

katesmith280 Apr 15, 2024

DominicOram Apr 15, 2024

DominicOram Apr 15, 2024

katesmith280 commented Apr 15, 2024

DominicOram commented Apr 15, 2024

katesmith280 commented Apr 17, 2024

dperl-dls left a comment •

edited

Loading

katesmith280 commented Apr 18, 2024

dperl-dls left a comment

		@@ -85,7 +84,7 @@ class ApertureScatterguard(StandardReadable):
		def __init__(self, prefix: str = "", name: str = "") -> None:

		# extendedepicsmotor class has tolerance fields added
		# ophyd-async epics motor may need to do the same thing - epics motor

(#391) Create an ophyd-async ApertureScatterguard device #419

(#391) Create an ophyd-async ApertureScatterguard device #419

Conversation

katesmith280 commented Apr 4, 2024

Instructions to reviewer on how to test:

Checks for reviewer

codecov bot commented Apr 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dperl-dls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katesmith280 commented Apr 11, 2024 • edited Loading

dperl-dls left a comment • edited Loading

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katesmith280 commented Apr 15, 2024

DominicOram commented Apr 15, 2024

katesmith280 commented Apr 17, 2024

dperl-dls left a comment • edited Loading

Choose a reason for hiding this comment

katesmith280 commented Apr 18, 2024

dperl-dls left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 5, 2024 •

edited

Loading

katesmith280 commented Apr 11, 2024 •

edited

Loading

dperl-dls left a comment •

edited

Loading

dperl-dls left a comment •

edited

Loading