Ophyd async slits #431

callumforrester · 2024-04-11T11:49:03Z

Fixes #384

Port S4SlitGaps to ophyd-async. Add tests to prove it reads correctly. Also create a group object for bunches of slits. Add slits config for I22 and P38.

Instructions to reviewer on how to test:

Run unit 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

callumforrester · 2024-04-11T11:56:08Z

src/dodal/devices/s4_slit_gaps.py

+        Constructor, formats prefix with indices for sub-devices. For example:
+        S4SlitGapsGroup("MY-SLITS-{0:02d}:", range(5)) will populate the group with 4
+        S4SlitGaps devices with prefixes MY-SLITS-00, MY-SLITS-01, MY-SLITS-02,
+        MY-SLITS-03. The index to access them will be the same as the PV number, so
+        MY-SLITS-02 is accessed via group.slits[2].


@DiamondJoseph and @DominicOram I'm not sure about this, just an idea I had to save boilerplate. See the i22 and p38 modules for example usage. Interested in your thoughts.

I think at the very least, indices should be a Sequence[int], in case Slits[n] are disabled temporarily.

Looks like this fits with the ophyd_async ADR https://github.com/bluesky/ophyd-async/blob/7be73e3ce0f79fca2e02f77f366792ee84586ed7/docs/developer/explanations/decisions/0006-procedural-device-definitions.rst#L58 for proceedurally created devices.

I do wish it was just slits[1] rather than group.slits[1]: does group.name = "" work with blueapi still being able to get group.slits[1]?

Agree with Sequence[int], for why the group adds another layer to the tree, see this comment bluesky/ophyd-async#181 (comment)

That said, I do agree it would be nicer, I wonder if there's a way of subclassing DeviceVector to incorporate the PV prefixing logic...

Tagging @coretl in case he has ideas

I'm not a big fan of this. I think from a use perspective doing i03.slits_1() is more obvious than i03.slits().slits[1] given that this is how all other devices work. In other devices we only group like this if the devices make sense for the user to move together, does it make sense to the user for all of the slits to be one composite? What is one of the slit 2 fails to connect but we only need slit 4 in the plan? @dperl-dls, what do you think?

Personally I'm not a fan of a composite device here, I'd prefer slits1, slits2, etc.

However, if you decide you really want it, it does occur to me that multiple inheritance of DeviceVector and StandardDetector (rather than your existing nesting of DeviceVector within StandardDetector might work to squash down the layers

Makes sense, I think we just remove the group entirely if it doesn't make semantic sense

Makes sense and I agree with the user perspective. @DominicOram how about i03.slits()[1]?

Better but I still think separate is the best for the user, as @Tom-Willemsen says. If we're only doing this to avoid boilerplate then all for thinking about ways to make the beamline files less boilerplate-y without changing the interface

Although this does create a lot of additional boilerplate in i22.py so should think more about it long-term

I'm not opposed to the boilerplate in this case. To be honest, long term we're probably going to need to split i22 into modules like i22.slits, i22.sample_environment, and so on, and the top-level i22 will just import * from each of those.

I think I fear the boilerplate just because of how much GDA's per-beamline config has ballooned over the years, it's ended up split into files and subdirectories etc. in a very similar way to your suggestion here. I suppose that's not the end of the world but it would be nice to have a more concise description of the beamline to work with.

callumforrester · 2024-04-11T11:56:58Z

src/dodal/devices/s4_slit_gaps.py

+        self.x_gap = Motor(prefix + "X:SIZE")
+        self.y_gap = Motor(prefix + "Y:SIZE")


@DominicOram Renamed from xgap and ygap on the original for more standard pythonicness, but can revert if it will cause pain for hyperion.

It will cause problems but we can deal with it. Can you wait for a linked Hyperion PR before merging this though?

Yep, no problem

DominicOram · 2024-04-11T13:42:49Z

@dan-fernandes how does this interact with the code in #315? Does this supersede some of it?

DominicOram

Some comments. Annoyingly we're more stuck than I thought we were on on getting the PV names changed for i03 too. Working on it...

DominicOram · 2024-04-11T13:45:51Z

src/dodal/devices/s4_slit_gaps.py



-class S4SlitGaps(Device):
-    """Note that the S4 slits have a different PV fromat to other beamline slits"""
+class S4SlitGaps(StandardReadable):


Should: Why are these named after S4? My understanding is that this in now a generic slit class?

Because I don't know what S4 is :)

Will rename to Slits

Incidentally, and at risk of betraying my woeful knowledge of xray optics, if either you or @Tom-Willemsen can come up with a better docstring than mine then I'm all ears. Mine is basically "erm, it's a set of some sort of slits...?"

This is a 4-blade slit set defined by a gap and a centre for each pair of blades.

The things I can immediately think of which we want to diambiguate from are:

Having different numbers of blades, e.g. a 2 blade vertical slit set might exist on some beamlines, this would only define the beam in a vertical oritentation.

Definining the slit set by absolute positions of each blade rather than gap and centre. Not sure, but I'd wager at least some beamlines do this - e.g. for scanning in each blade against the beam individually

Because I don't know what S4

Logically the fourth slit set along the x-ray beam path... However the numbering can be illogical sometimes. As an example VMXi used to have a slit set S2, but this was removed some time ago, so now their slits along the beam path are S1, S3, ...

Feel free to swing by my desk if you need...

Ah, so could actually call the class Slits4Blade or something similar? For absolute vs. relative it sounds like those things can coexist in the same device?

I'd love to swing by but I'm sick!

We can probably assume 4-blade is the standard case, so I think naming the class Slits is fine as long as the docstring clarifies it's a 4-blade set.

For absolute vs. relative it sounds like those things can coexist in the same device?

Yes, care is just needed because you then have multiple PVs which affect the same physical blade. (e.g. moving the north blade will change the vertical gap and vice versa).

Does a picture like this help to visualise what's going on?

Yes, quite a lot, thanks!

DominicOram · 2024-04-11T13:46:11Z

src/dodal/devices/s4_slit_gaps.py

+        super().__init__(name)
+
+
+class S4SlitGapsGroup(StandardReadable):


Should: As above, not sure why these have S4 in the name?

DominicOram · 2024-04-11T13:49:58Z

src/dodal/devices/s4_slit_gaps.py

+        self.x_gap = Motor(prefix + "X:SIZE")
+        self.y_gap = Motor(prefix + "Y:SIZE")


It will cause problems but we can deal with it. Can you wait for a linked Hyperion PR before merging this though?

DominicOram · 2024-04-11T13:56:18Z

src/dodal/devices/s4_slit_gaps.py

+        Constructor, formats prefix with indices for sub-devices. For example:
+        S4SlitGapsGroup("MY-SLITS-{0:02d}:", range(5)) will populate the group with 4
+        S4SlitGaps devices with prefixes MY-SLITS-00, MY-SLITS-01, MY-SLITS-02,
+        MY-SLITS-03. The index to access them will be the same as the PV number, so
+        MY-SLITS-02 is accessed via group.slits[2].


I'm not a big fan of this. I think from a use perspective doing i03.slits_1() is more obvious than i03.slits().slits[1] given that this is how all other devices work. In other devices we only group like this if the devices make sense for the user to move together, does it make sense to the user for all of the slits to be one composite? What is one of the slit 2 fails to connect but we only need slit 4 in the plan? @dperl-dls, what do you think?

DominicOram · 2024-04-11T16:03:09Z

src/dodal/beamlines/i22.py

+    return device_instantiation(
+        Slits,
+        "slits-01",
+        "-AL-SLITS-01:",
+        wait_for_connection,
+        fake_with_ophyd_sim,
+    )


Could: I would still want the slits_x() interface but no reason you can't hide it:

def _generic_slit(slit_num, wait_for_connection, fake): return device_instantiation( Slits, f"slits-0{slit_num}", f"-AL-SLITS-0{slit_num}:", wait_for_connection, fake_with_ophyd_sim, ) def slits_1(...): return _generic_slit(1, ...) ...

Good idea, is there currently anywhere sensible to put _generic_slit so that it can be shared between the i22 and p38 modules?

Hmm.. I guess devices.util.something?

I ended up putting it in a private module in beamlines because otherwise we were importing device_instantiation from beamline.beamline_utils into devices.util.something, then importing that back into beamlines.i22, which I could lead to circular import troubles at some point. But let me know what you think, happy to change it

dperl-dls · 2024-04-12T08:15:24Z

I think from a use perspective doing i03.slits_1() is more obvious than i03.slits().slits[1] given that this is how all other devices work. In other devices we only group like this if the devices make sense for the user to move together, does it make sense to the user for all of the slits to be one composite? What is one of the slit 2 fails to connect but we only need slit 4 in the plan?

I agree with this - I think we rarely if ever need to coordinate movements between different slits, except, like, setting the whole beamline to some standard state. It's a little academic because slits 1&2 should never actually be offline if we're running stuff, but there's no reason to actively make devices connect to them when we only need _4 99% of the time.

tests/devices/unit_tests/test_slits.py

Port S4SlitGaps to ophyd-async. Add tests to prove it reads correctly. Also create a group object for bunches of slits.

src/dodal/beamlines/_device_helpers.py

callumforrester · 2024-04-24T08:08:09Z

As discussed with @DominicOram, @dperl-dls and @olliesilvester, the work to standardise the slits PVs has encountered complications so we are going to keep both versions of the slits device for now. Once that work is done we can action #462 and rationalise the implementations.

DiamondJoseph · 2024-04-25T11:14:01Z

src/dodal/beamlines/_device_helpers.py

+from .beamline_utils import device_instantiation
+
+
+@skip_device()


Does this decorator do anything here?

DominicOram

Looks good to me, don't think it will have any affect on MX yet but looks like we will be able to switch to it easily when we get round to #462

callumforrester force-pushed the ophyd-async-slits branch from 5acee35 to 83a63db Compare April 11, 2024 11:49

callumforrester commented Apr 11, 2024

View reviewed changes

callumforrester requested review from DominicOram and DiamondJoseph April 11, 2024 12:06

callumforrester marked this pull request as ready for review April 11, 2024 12:08

DominicOram requested changes Apr 11, 2024

View reviewed changes

DominicOram reviewed Apr 11, 2024

View reviewed changes

callumforrester force-pushed the ophyd-async-slits branch from 800369c to 9caec0d Compare April 12, 2024 09:34

DiamondJoseph reviewed Apr 12, 2024

View reviewed changes

tests/devices/unit_tests/test_slits.py Show resolved Hide resolved

callumforrester force-pushed the ophyd-async-slits branch from 9caec0d to d75a999 Compare April 23, 2024 08:03

callumforrester added 10 commits April 24, 2024 08:56

Port S4SlitGaps to ophyd-async and add tests

8525643

Port S4SlitGaps to ophyd-async. Add tests to prove it reads correctly. Also create a group object for bunches of slits.

Add I22 and P38 config for slit gaps

b682332

Add docstrings to slit gaps

3f51cf8

Refactor tests

26bd2a7

Add test for extra slits

b512735

Remove slit group and rename S4SlitGaps to Slits

65ca40b

Rename s4_slit_gaps module to slits

7bb895f

Improve slits docstring

f70316a

Move common slit instantation logic into helper module

e9dc193

Revert MX beamlines to old slits

a01e63b

callumforrester force-pushed the ophyd-async-slits branch from 8fe6867 to a01e63b Compare April 24, 2024 07:58

DiamondJoseph requested changes Apr 24, 2024

View reviewed changes

src/dodal/beamlines/_device_helpers.py Outdated Show resolved Hide resolved

callumforrester mentioned this pull request Apr 24, 2024

Rationalise Slits Implementations #462

Open

Fix missing arg

08f4df8

DiamondJoseph approved these changes Apr 24, 2024

View reviewed changes

DiamondJoseph approved these changes Apr 25, 2024

View reviewed changes

DiamondJoseph reviewed Apr 25, 2024

View reviewed changes

DominicOram approved these changes Apr 25, 2024

View reviewed changes

callumforrester merged commit 081f82c into main Apr 25, 2024
11 checks passed

callumforrester deleted the ophyd-async-slits branch April 25, 2024 12:19

callumforrester mentioned this pull request Apr 26, 2024

Add fswitch device for i22 #469

Merged

2 tasks

joeshannon mentioned this pull request May 10, 2024

Fix i22 slits #520

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ophyd async slits #431

Ophyd async slits #431

callumforrester commented Apr 11, 2024

callumforrester Apr 11, 2024

DiamondJoseph Apr 11, 2024

callumforrester Apr 11, 2024

DominicOram Apr 11, 2024

coretl Apr 11, 2024

callumforrester Apr 11, 2024

DominicOram Apr 11, 2024

callumforrester Apr 11, 2024

Tom-Willemsen Apr 11, 2024

callumforrester Apr 11, 2024

callumforrester Apr 11, 2024

DominicOram Apr 11, 2024

callumforrester Apr 11, 2024

DominicOram commented Apr 11, 2024

DominicOram left a comment

DominicOram Apr 11, 2024

callumforrester Apr 11, 2024

callumforrester Apr 11, 2024

Tom-Willemsen Apr 11, 2024 •

edited

Loading

callumforrester Apr 11, 2024

Tom-Willemsen Apr 11, 2024

callumforrester Apr 11, 2024

DominicOram Apr 11, 2024

DominicOram Apr 11, 2024

DominicOram Apr 11, 2024

DominicOram Apr 11, 2024

callumforrester Apr 11, 2024

DominicOram Apr 11, 2024

callumforrester Apr 12, 2024

dperl-dls commented Apr 12, 2024

callumforrester commented Apr 24, 2024

DiamondJoseph Apr 25, 2024

DominicOram left a comment

		self.x_gap = Motor(prefix + "X:SIZE")
		self.y_gap = Motor(prefix + "Y:SIZE")

		super().__init__(name)


		class S4SlitGapsGroup(StandardReadable):

		from .beamline_utils import device_instantiation


		@skip_device()

Ophyd async slits #431

Ophyd async slits #431

Conversation

callumforrester commented Apr 11, 2024

Instructions to reviewer on how to test:

Checks for reviewer

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram commented Apr 11, 2024

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

Tom-Willemsen Apr 11, 2024 • edited Loading

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

dperl-dls commented Apr 12, 2024

callumforrester commented Apr 24, 2024

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

Tom-Willemsen Apr 11, 2024 •

edited

Loading