Port `UndulatorDCM` to ophyd-async #461

callumforrester · 2024-04-24T07:46:56Z

Works towards #389 and #397

Changes:

Rename DCM.py to dcm.py to comply with pep8: https://peps.python.org/pep-0008/#package-and-module-names
Port UndulatorDCM and its two components Undulator and DCM to ophyd-async as StandardReadables.
Update tests

Instructions to reviewer on how to test:

Check unit tests pass

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

callumforrester · 2024-04-24T16:25:15Z

This work provoked a discussion about the best way to handle composite device logic like UndulatorDCM with ophyd-async. The basic options were:

Don't do it, use a plan stub
Don't use a soft signal like UndulatorDCM does, just directly implement set et al.
Keep the soft signal and write a custom SignalBackend

So far, this PR is doing the 3rd option, but is otherwise just a naive conversion to ophyd-async, changing as little logic/attributes as possible. I'm tempted to leave it that way and spin out the additional changes we discussed, as well as the required changes for #389 and #397 out into additional issues/PRs, this one is already getting big.

@DominicOram @dperl-dls @olliesilvester would that be okay with you?

Additional Changes Discussed

Expose the fields needed for I22 from Undulator and DCM (see Ophyd-async implementation of optical components for i22 #377)
Rename class to BeamEnergy so that BeamEnergy.set sets the beam energy
Implement the Locatable protocol so that the beam energy can have a setpoint and a readback
Make BeamEnergy.read return a reasonable value for the beam energy

The latter two are not required for Hyperion, however a small Hyperion PR is still needed to make this work

callumforrester · 2024-04-24T16:26:45Z

src/dodal/devices/dcm.py

+    def __init__(
+        self,
+        prefix: str,
+        daq_configuration_path: str,


@DominicOram and @rtuck99 how would you feel about moving the configuration file attributes for both the DCM and Undulator into UndulatorDCM? Since it's just a convenient place to store them for Hyperion and means that the devices are as simple as possible for the read-only use case

That's fine by me

Done, see what you think

callumforrester · 2024-04-25T09:42:44Z

src/dodal/devices/undulator_dcm.py

-                raise AccessError(
-                    "Undulator gap access is disabled. Contact Control Room"
-                )
+    def __init__(self, prefix: str, undulator: Undulator, dcm: DCM, name: str = ""):


prefix is unused and only there to satisfy device_instantiation, which assumes every device takes a prefix. I'm not overjoyed with this but unsure if this should be a change to device_instantiation or just not using to instantiate this device.

given we plan to redo device instantiation, is it worth worrying about? IMO best to just note that we don't want this to be the case anymore when we do that

DominicOram

@DominicOram @dperl-dls @olliesilvester would that be okay with you?

Yep, works for me, thank you!

Some comments in code. Additionally, can you make test_i03_system (with the i03 panda and pin detect commented out due to #261) and test_i04_system still work? That's how I caught the Enum issue

DominicOram · 2024-04-25T11:22:29Z

src/dodal/devices/dcm.py

+from dodal.beamlines.beamline_parameters import get_beamline_parameters
+
+
+class DCM(StandardReadable, HasHints):


Should: I appreciate this wasn't your doing but while where here... Can we roll -MO-DCM-01: into the prefix? Either by specifying it in the beamline file like we do in other devices or by just doing a prefix = prefix + "-MO-DCM-01" as the first thing in the init?

DominicOram · 2024-04-25T11:30:10Z

src/dodal/devices/dcm.py

+    def __init__(
+        self,
+        prefix: str,
+        daq_configuration_path: str,


That's fine by me

DominicOram · 2024-04-25T11:32:09Z

src/dodal/devices/undulator.py

+    ) -> None:
+        self.gap_motor = Motor(prefix + "BLGAPMTR")
+        self.current_gap = epics_signal_r(float, prefix + "CURRGAPD")
+        self.gap_access = epics_signal_r(UndulatorGapAccess, prefix + "IDBLENA")


Must: The Enum needs to inherit from str too. +1 to bluesky/ophyd-async#261

DominicOram · 2024-04-25T11:33:22Z

src/dodal/devices/undulator_dcm.py

+async def _get_energy_distance_table(lookup_table_path: str) -> ndarray:
+    # Slight cheat to make the file IO async, numpy doesn't do any real IO now, just
+    # decodes the text
+    async with aiofiles.open(lookup_table_path, "r") as stream:
+        raw_table = await stream.read()
+    return loadtxt(StringIO(raw_table), comments=["#", "Units"])


Should: Can you move this util function into lookup_tables.py?

DominicOram · 2024-04-25T11:34:24Z

src/dodal/devices/undulator_dcm.py

+        # Attributes are set after super call so they are not renamed to
+        # <name>-undulator, etc.


👍 Nice comment, thanks!

DominicOram · 2024-04-25T11:47:57Z

tests/devices/unit_tests/test_undulator.py

+        "undulator-gap_access",
+    ],
+)
+async def test_read_and_describe_includes(


Should: Same as above, why does this need to be staged?

DominicOram · 2024-04-25T11:49:14Z

tests/devices/unit_tests/test_undulator_dcm.py

+    # fake_undulator_dcm.undulator.gap_motor.move = MagicMock()
+    # fake_undulator_dcm.dcm.energy_in_kev.move = MagicMock()


Should: Can we remove these now?

DominicOram · 2024-04-25T11:49:42Z

tests/devices/unit_tests/test_undulator_dcm.py

+    # fake_undulator_dcm.undulator.gap_motor.move = MagicMock()
+    # fake_undulator_dcm.dcm.energy_in_kev.move = MagicMock()


Should: Can we remove these?

DominicOram · 2024-04-25T11:50:01Z

tests/devices/unit_tests/test_undulator_dcm.py

+    # fake_undulator_dcm.undulator.gap_motor.move = MagicMock()
+    # fake_undulator_dcm.dcm.energy_in_kev.move = MagicMock()


Should: As above, remove

DominicOram · 2024-04-25T11:51:19Z

tests/devices/unit_tests/test_undulator_dcm.py



-def test_energy_set_only_complete_when_all_statuses_are_finished(
+async def test_energy_set_only_complete_when_all_statuses_are_finished(


Thanks, this is a nice pattern for testing this that I hadn't yet worked out for ophyd_async!

callumforrester · 2024-04-25T13:35:49Z

Fix for black CI issues here: #470

In accordance with pep8: https://peps.python.org/pep-0008/#package-and-module-names

…d DCM to UndulatorDCM

DominicOram

Thanks, some more changes in code. This will also break Hyperion, I'll make a PR to fix

DominicOram · 2024-05-01T11:55:52Z

src/dodal/devices/dcm.py

+                "dcm_bragg_in_degrees",
+                "dcm_roll_in_mrad",
+                "dcm_offset_in_mm",
+                "dcm_perp_in_mm",
+                "dcm_energy_in_kev",
+                "dcm_pitch_in_mrad",
+                "dcm_wavelength",


Should: Can we return self.bragg_in_degrees.name etc. This adds the potential to catch people when if they rename the signal but miss this string.

DominicOram · 2024-05-01T15:04:49Z

I did have some comments about Motor signals being readable but I think those are a wider issue outside this PR, made #492 to discuss

DiamondJoseph · 2024-05-03T15:49:00Z

src/dodal/devices/dcm.py

+    def __init__(
+        self,
+        prefix: str,
+        name: str = "",
+    ) -> None:
+        self.bragg_in_degrees = Motor(prefix + "BRAGG")
+        self.roll_in_mrad = Motor(prefix + "ROLL")
+        self.offset_in_mm = Motor(prefix + "OFFSET")
+        self.perp_in_mm = Motor(prefix + "PERP")
+        self.energy_in_kev = Motor(prefix + "ENERGY")
+        self.pitch_in_mrad = Motor(prefix + "PITCH")
+        self.wavelength = Motor(prefix + "WAVELENGTH")
+
+        # temperatures
+        self.xtal1_temp = epics_signal_r(float, prefix + "TEMP1")
+        self.xtal2_temp = epics_signal_r(float, prefix + "TEMP2")
+        self.xtal1_heater_temp = epics_signal_r(float, prefix + "TEMP3")
+        self.xtal2_heater_temp = epics_signal_r(float, prefix + "TEMP4")
+        self.backplate_temp = epics_signal_r(float, prefix + "TEMP5")
+        self.perp_temp = epics_signal_r(float, prefix + "TEMP6")
+        self.perp_sub_assembly_temp = epics_signal_r(float, prefix + "TEMP7")
+
+        self.set_readable_signals(
+            read=[
+                self.bragg_in_degrees,  # type: ignore
+                self.roll_in_mrad,  # type: ignore
+                self.offset_in_mm,  # type: ignore
+                self.perp_in_mm,  # type: ignore
+                self.energy_in_kev,  # type: ignore
+                self.pitch_in_mrad,  # type: ignore
+                self.wavelength,  # type: ignore
+                self.xtal1_temp,
+                self.xtal2_temp,
+                self.xtal1_heater_temp,
+                self.xtal2_heater_temp,
+                self.backplate_temp,
+                self.perp_temp,
+                self.perp_sub_assembly_temp,
+            ]
+        )


Suggested change

def __init__(

self,

prefix: str,

name: str = "",

) -> None:

self.bragg_in_degrees = Motor(prefix + "BRAGG")

self.roll_in_mrad = Motor(prefix + "ROLL")

self.offset_in_mm = Motor(prefix + "OFFSET")

self.perp_in_mm = Motor(prefix + "PERP")

self.energy_in_kev = Motor(prefix + "ENERGY")

self.pitch_in_mrad = Motor(prefix + "PITCH")

self.wavelength = Motor(prefix + "WAVELENGTH")

# temperatures

self.xtal1_temp = epics_signal_r(float, prefix + "TEMP1")

self.xtal2_temp = epics_signal_r(float, prefix + "TEMP2")

self.xtal1_heater_temp = epics_signal_r(float, prefix + "TEMP3")

self.xtal2_heater_temp = epics_signal_r(float, prefix + "TEMP4")

self.backplate_temp = epics_signal_r(float, prefix + "TEMP5")

self.perp_temp = epics_signal_r(float, prefix + "TEMP6")

self.perp_sub_assembly_temp = epics_signal_r(float, prefix + "TEMP7")

self.set_readable_signals(

read=[

self.bragg_in_degrees, # type: ignore

self.roll_in_mrad, # type: ignore

self.offset_in_mm, # type: ignore

self.perp_in_mm, # type: ignore

self.energy_in_kev, # type: ignore

self.pitch_in_mrad, # type: ignore

self.wavelength, # type: ignore

self.xtal1_temp,

self.xtal2_temp,

self.xtal1_heater_temp,

self.xtal2_heater_temp,

self.backplate_temp,

self.perp_temp,

self.perp_sub_assembly_temp,

]

)

def __init__(

self,

prefix: str,

name: str = "",

) -> None:

with self.add_children_as_readables():

self.bragg_in_degrees = Motor(prefix + "BRAGG")

self.roll_in_mrad = Motor(prefix + "ROLL")

self.offset_in_mm = Motor(prefix + "OFFSET")

self.perp_in_mm = Motor(prefix + "PERP")

self.energy_in_kev = Motor(prefix + "ENERGY")

self.pitch_in_mrad = Motor(prefix + "PITCH")

self.wavelength = Motor(prefix + "WAVELENGTH")

# temperatures

self.xtal1_temp = epics_signal_r(float, prefix + "TEMP1")

self.xtal2_temp = epics_signal_r(float, prefix + "TEMP2")

self.xtal1_heater_temp = epics_signal_r(float, prefix + "TEMP3")

self.xtal2_heater_temp = epics_signal_r(float, prefix + "TEMP4")

self.backplate_temp = epics_signal_r(float, prefix + "TEMP5")

self.perp_temp = epics_signal_r(float, prefix + "TEMP6")

self.perp_sub_assembly_temp = epics_signal_r(float, prefix + "TEMP7")

DiamondJoseph · 2024-05-03T15:49:59Z

src/dodal/devices/dcm.py

+    @property
+    def hints(self) -> Hints:
+        return {
+            "fields": [
+                self.bragg_in_degrees.name,
+                self.roll_in_mrad.name,
+                self.offset_in_mm.name,
+                self.perp_in_mm.name,
+                self.energy_in_kev.name,
+                self.pitch_in_mrad.name,
+                self.wavelength.name,
+            ]
+        }


Suggested change

@property

def hints(self) -> Hints:

return {

"fields": [

self.bragg_in_degrees.name,

self.roll_in_mrad.name,

self.offset_in_mm.name,

self.perp_in_mm.name,

self.energy_in_kev.name,

self.pitch_in_mrad.name,

self.wavelength.name,

]

}

Not required with above change

I was literally half way through doing this when I got called to i03 ;)

We are of one mind, when my hair is long again we can link up Na'vi style

DominicOram

Great, thank you. Have updated the Hyperion side to match in DiamondLightSource/hyperion#1349 so happy to merge both at the same time when that is approved.

callumforrester force-pushed the 389-and-397-undulator-dcm-ophyd-async branch from 498792a to 6cce80e Compare April 24, 2024 14:11

callumforrester commented Apr 24, 2024

View reviewed changes

callumforrester commented Apr 25, 2024

View reviewed changes

callumforrester force-pushed the 389-and-397-undulator-dcm-ophyd-async branch from 40d8fde to e182834 Compare April 25, 2024 09:45

callumforrester marked this pull request as ready for review April 25, 2024 10:45

callumforrester requested a review from DominicOram April 25, 2024 10:46

DominicOram requested changes Apr 25, 2024

View reviewed changes

callumforrester mentioned this pull request Apr 25, 2024

Replace black with ruff #470

Merged

2 tasks

callumforrester added 18 commits May 1, 2024 10:50

Rename DCM.py to dcm.py

0ff71bc

In accordance with pep8: https://peps.python.org/pep-0008/#package-and-module-names

Naively port DCM to ophyd-async

f2b32ce

Begin moving Undulator and UndulatorDCM to ophyd-async

5d41025

Port UndulatorDCM to ophyd-async and remove energy signal

242da6b

Tidy and document UndulatorDCM class

de34ba9

Make UndulatorDCM file I/O asynchronous

de32163

Document undulator and dcm

7060696

Update Undulator system test

aca5199

Document and test undular and dcm classes

3961241

Fix UndulatorDCM instantiation on i03

88b7909

Remove MO-DCM-01: prefix from hardcoding

c63b2bf

Make undulator enum inherit from str

da44327

Move ID gap lookup table into util module

d305391

Remove commented out code

c1ae952

Simplify undulator system test

78d4bd3

Stop staging undulator and dcm during read tests

8e66fc5

Fix import errors and formatting

7c3fdaf

Move I03 energy change lookup table path attributes from Undulator an…

48091cd

…d DCM to UndulatorDCM

callumforrester force-pushed the 389-and-397-undulator-dcm-ophyd-async branch from d5dd286 to 48091cd Compare May 1, 2024 09:50

Update based on breaking changes in ophyd-async

d91bb3e

DominicOram requested changes May 1, 2024

View reviewed changes

Make more maintainable hint names

23bded3

callumforrester requested a review from DominicOram May 2, 2024 09:45

callumforrester mentioned this pull request May 3, 2024

Convert FocusingMirror to ophyd_async #467

Merged

2 tasks

DominicOram mentioned this pull request May 3, 2024

Update to use ophyd_async undulator_dcm DiamondLightSource/hyperion#1349

Merged

DominicOram added 2 commits May 3, 2024 16:30

(#397) Update adjuster to allow ophyd_async motors

ea806c5

Merge branch 'main' into 389-and-397-undulator-dcm-ophyd-async

d905a29

DiamondJoseph requested changes May 3, 2024

View reviewed changes

DominicOram added 2 commits May 5, 2024 18:45

(#397) Update to work for latest ophyd_async

360db19

(#397) Updated undulator to use add_children_as_readables

830adfe

DominicOram approved these changes May 5, 2024

View reviewed changes

DominicOram requested a review from DiamondJoseph May 5, 2024 18:17

DiamondJoseph approved these changes May 7, 2024

View reviewed changes

DominicOram merged commit 6864607 into main May 7, 2024
11 checks passed

DominicOram deleted the 389-and-397-undulator-dcm-ophyd-async branch May 7, 2024 08:59

DominicOram mentioned this pull request Aug 7, 2024

706 make undulator gap writeable i18 #721

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port `UndulatorDCM` to ophyd-async #461

Port `UndulatorDCM` to ophyd-async #461

callumforrester commented Apr 24, 2024 •

edited

Loading

callumforrester commented Apr 24, 2024

callumforrester Apr 24, 2024

DominicOram Apr 25, 2024

callumforrester Apr 25, 2024

callumforrester Apr 25, 2024

d-perl Apr 25, 2024

DominicOram left a comment

DominicOram Apr 25, 2024

DominicOram Apr 25, 2024

DominicOram Apr 25, 2024

DominicOram Apr 25, 2024

DominicOram Apr 25, 2024

DominicOram Apr 25, 2024

DominicOram Apr 25, 2024

DominicOram Apr 25, 2024

DominicOram Apr 25, 2024

DominicOram Apr 25, 2024

callumforrester commented Apr 25, 2024

DominicOram left a comment

DominicOram May 1, 2024

DominicOram commented May 1, 2024

DiamondJoseph May 3, 2024

DiamondJoseph May 3, 2024

DominicOram May 3, 2024

DiamondJoseph May 3, 2024

DominicOram left a comment

		from dodal.beamlines.beamline_parameters import get_beamline_parameters


		class DCM(StandardReadable, HasHints):

		# Attributes are set after super call so they are not renamed to
		# <name>-undulator, etc.

		# fake_undulator_dcm.undulator.gap_motor.move = MagicMock()
		# fake_undulator_dcm.dcm.energy_in_kev.move = MagicMock()



		def test_energy_set_only_complete_when_all_statuses_are_finished(
		async def test_energy_set_only_complete_when_all_statuses_are_finished(

Port UndulatorDCM to ophyd-async #461

Port UndulatorDCM to ophyd-async #461

Conversation

callumforrester commented Apr 24, 2024 • edited Loading

Instructions to reviewer on how to test:

Checks for reviewer

callumforrester commented Apr 24, 2024

Additional Changes Discussed

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

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

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

Choose a reason for hiding this comment

callumforrester commented Apr 25, 2024

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram commented May 1, 2024

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

DominicOram left a comment

Choose a reason for hiding this comment

Port `UndulatorDCM` to ophyd-async #461

Port `UndulatorDCM` to ophyd-async #461

callumforrester commented Apr 24, 2024 •

edited

Loading