Create beamsize devices #1704

jacob720 · 2025-11-12T14:19:52Z

Fixes DiamondLightSource/mx-bluesky#1424

Required by DiamondLightSource/mx-bluesky#1445

Adds beamsize devices for i03 and i04 so that the beamsize be read correctly in both cases, using the transfocator for i04 and the aperture for i03.

Instructions to reviewer on how to test:

Do thing x
Confirm thing y happens

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
If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

codecov · 2025-11-12T14:29:00Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.08%. Comparing base (c719bfc) to head (a58c05d).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #1704    +/-   ##
========================================
  Coverage   99.07%   99.08%            
========================================
  Files         271      278     +7     
  Lines        9971    10140   +169     
========================================
+ Hits         9879    10047   +168     
- Misses         92       93     +1

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jacob720 · 2025-11-13T10:10:09Z

src/dodal/devices/i03/beamsize.py

+
+
+class Beamsize(BeamsizeBase):
+    def __init__(self, aperture_scatterguard: ApertureScatterguard, name="beamsize"):


Is it better to add a name when you instantiate the device and leave this as name=""?

Yes, I think leave it as "" default

DominicOram

Great, thanks. This works (mostly) but I wonder if we could make something more generic where the Beamsize takes a list of signals/constants and just finds the minimum e.g. something like:

class Beamsize(BeamsizeBase):
    def __init__(
        self,
        x_signals:list[SignalR|float],
        y_signals:list[SignalR|float],
    ):
        super().__init__(name=name)

        self.x_um = derived_signal_r(
            self._get_beamsize_x,
            x_signals=x_signals,
            derived_units="µm",
        )
        self.y_um = derived_signal_r(
            self._get_beamsize_y,
            y_signals=y_signals,
            derived_units="µm",
        )

    def _get_beamsize_x(
        self,
        x_signals: list[float],
    ) -> float:
        return min(*x_signals)

    def _get_beamsize_y(
        self,
        y_signals: list[float],
    ) -> float:
        return min(*y_signals)

This won't work exactly like that because derived_signal won't play nice with lists of signals, especially where one of those things might not be a signal but a constant. We could maybe get round the constant thing by making a dummy soft signal that just has the constant value though. I think it's worth playing with this though and if it isn't obvious maybe asking in #ophy-async how it would be done

DominicOram · 2025-11-13T09:43:59Z

src/dodal/beamlines/i03.py

+
+@device_factory()
+def beamsize() -> Beamsize:
+    """Get the i03 beamstop device, instantiate it if it hasn't already been.


Should:

Suggested change

"""Get the i03 beamstop device, instantiate it if it hasn't already been.

"""Get the i03 beamsize device, instantiate it if it hasn't already been.

These comments are kind of rubbish anyway but let's make them correct rubbish

DominicOram · 2025-11-13T09:44:28Z

src/dodal/beamlines/i04.py

+
+@device_factory()
+def beamsize() -> Beamsize:
+    """Get the i03 beamstop device, instantiate it if it hasn't already been.


Should: As above

Suggested change

"""Get the i03 beamstop device, instantiate it if it hasn't already been.

"""Get the i03 beamsize device, instantiate it if it hasn't already been.

DominicOram · 2025-11-13T11:08:21Z

src/dodal/beamlines/i04.py

+
+@device_factory()
+def beamsize() -> Beamsize:
+    """Get the i04 beamstop device, instantiate it if it hasn't already been.


Should: As above

Suggested change

"""Get the i04 beamstop device, instantiate it if it hasn't already been.

"""Get the i04 beamsize device, instantiate it if it hasn't already been.

DominicOram · 2025-11-13T11:10:14Z

src/dodal/devices/i03/beamsize.py

+
+
+class Beamsize(BeamsizeBase):
+    def __init__(self, aperture_scatterguard: ApertureScatterguard, name="beamsize"):


Yes, I think leave it as "" default

DominicOram · 2025-11-13T12:32:29Z

src/dodal/devices/beamsize/beamsize.py

+    def __init__(self, name="beamsize"):
+        super().__init__(name=name)


Could: I think we can just remove the constructor all together and it will pass the value through.

DominicOram · 2025-11-13T12:35:47Z

src/dodal/devices/i03/beamsize.py

+
+        self.x_um = derived_signal_r(
+            self._get_beamsize_x,
+            aperture_radius=self._aperture_scatterguard_ref().radius,


It's only in reading this that I've realised that this isn't the radius at all, it's the diameter. Would you be able to write an issue to fix it in the ApertureScattergaurd and downstream?

Yeah that completely went over my head too lol

DominicOram · 2025-11-13T12:44:12Z

tests/devices/i03/test_beamsize.py

+        (ApertureValue.PARKED, (0, 0, 0), (0.0, 0.0)),
+    ],
+)
+async def test_beamsize_gives_min_of_aperture_and_beam_width_and_height(


Should: I think this is testing more of the internal logic of the aperture than you need. You should be able to just do set_mock_value(aperture.radius, 10) then test that value is used. Tests that selecting specific apertures changes the radius should already exist in other places. If you can't do this because you can't do set_mock_value on a derived signal then we should make an ophyd_async issue and instead do ap_sg.aperture._get_current_radius = MagicMock(return_value=10)

Yeah I tried that initially but it doesn't work with derived signals. I'll raise an issue and mock _get_current_radius for now.

bluesky/ophyd-async#1132

DominicOram · 2025-11-13T12:44:46Z

tests/devices/i04/test_beamsize.py

+        (ApertureValue.PARKED, (0, 0, 0), (120.0, 120.0), (0.0, 0.0)),
+    ],
+)
+async def test_beamsize_gives_min_of_aperture_and_transfocator_width_and_height(


Should: As on the other test re internal device logic

DominicOram · 2025-11-13T12:49:19Z

tests/devices/i03/test_beamsize.py

+        (ApertureValue.OUT_OF_BEAM, (0, 0, 0), (0.0, 0.0)),
+        (ApertureValue.PARKED, (0, 0, 0), (0.0, 0.0)),


Must: I think the logic here is actually wrong. When we're out of the beam there is no aperture so the beamsize should be the default value. I think a good solution to this is to change the aperture device so that it's radius at these positions is infinite

DominicOram · 2025-11-13T12:54:14Z

src/dodal/devices/beamsize/beamsize.py

+    x_um: SignalR[float]
+    y_um: SignalR[float]


Should: These should be hinted as Format.HINTED_SIGNAL. This will mean that they will get read when you read the whole device.

DominicOram · 2025-11-13T13:30:54Z

This won't work exactly like that because derived_signal won't play nice with lists of signals

https://blueskyproject.io/ophyd-async/main/how-to/derive-one-signal-from-others.html#multi-derived-signal might let you do it. Needs some investigation

jacob720 added 5 commits November 12, 2025 09:34

Add beamsize devices

e23a0a0

Remove redundant code

25181b5

Fixes

aeb7256

Add beamsize devices to device factories

d8cb85e

Add names

46096ff

jacob720 requested a review from a team as a code owner November 12, 2025 14:19

jacob720 mentioned this pull request Nov 12, 2025

Use beamsize device to get beamsize DiamondLightSource/mx-bluesky#1445

Open

1 task

jacob720 force-pushed the mx_bluesky_1424_put_correct_beamsize branch from a645d65 to 76f53b0 Compare November 12, 2025 14:24

jacob720 commented Nov 13, 2025

View reviewed changes

jacob720 force-pushed the mx_bluesky_1424_put_correct_beamsize branch 2 times, most recently from 9b06c0e to 28f718c Compare November 13, 2025 10:47

Fixes and tests

2a57c7b

jacob720 force-pushed the mx_bluesky_1424_put_correct_beamsize branch from 28f718c to 2a57c7b Compare November 13, 2025 10:51

DominicOram requested changes Nov 13, 2025

View reviewed changes

PR comments

a58c05d

jacob720 force-pushed the mx_bluesky_1424_put_correct_beamsize branch from 0146f69 to a58c05d Compare November 13, 2025 17:40

jacob720 mentioned this pull request Nov 14, 2025

Allow set_mock_value on derived signals bluesky/ophyd-async#1132

Open



		class Beamsize(BeamsizeBase):
		def __init__(self, aperture_scatterguard: ApertureScatterguard, name="beamsize"):

	"""Get the i03 beamstop device, instantiate it if it hasn't already been.
	"""Get the i03 beamsize device, instantiate it if it hasn't already been.

	"""Get the i04 beamstop device, instantiate it if it hasn't already been.
	"""Get the i04 beamsize device, instantiate it if it hasn't already been.

		def __init__(self, name="beamsize"):
		super().__init__(name=name)

		(ApertureValue.OUT_OF_BEAM, (0, 0, 0), (0.0, 0.0)),
		(ApertureValue.PARKED, (0, 0, 0), (0.0, 0.0)),

Create beamsize devices #1704

Are you sure you want to change the base?

Create beamsize devices #1704

Uh oh!

Conversation

jacob720 commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instructions to reviewer on how to test:

Checks for reviewer

Uh oh!

codecov bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob720 Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DominicOram commented Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jacob720 commented Nov 12, 2025 •

edited

Loading

codecov bot commented Nov 12, 2025 •

edited

Loading

jacob720 Nov 13, 2025 •

edited

Loading