Add a device that will calculate a beam centre #1757

srishtysajeev · 2025-12-04T13:09:38Z

Instructions to reviewer on how to test:

Confirm tests work
Review code

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}

* Proof of concept device manager * Use fixtures parameter instead of **kwargs * Expand dependencies * Pre-optional handling * 'Working' optional dependencies * Docs, comments and warnings * Check for positional only arguments * Handle connections and support new loader in cli * Convert adsim to new loading * Add auto_connect to device manager * Remove references to connecting from device manager * DeviceResult wrappers * Demo adsim * build_and_connect method * Multiple manager CLI * FIXTUEES * Use device decorator for timeouts * Type build_and_connect and rely on fixtures for path provider * mostly reformatting * Make fixture generators lazy * Apologise for build_order method * Return function from fixture decorator * Add timeout parameter to build_and_connect * Remove dead comment * Use set in expand_dependencies to prevent repetition * Check for duplicate factory names * Add Ophyd v1 support * Connect Ophyd v1 devices * Move device_manager to new module Instead of burying it amongst the beamlines. * Remove test code from device_manager module * Remove debugging and commented code * Merge connectionspec and connectionparameters * Add or_raise method to DeviceBuildResult * Add docstrings * Use or_raise to handle build errors * Only set device name if required * Add TODOs to remove v1 support * Make v1 device timeout configurable * Default to waiting for v1 device connection * Add repr to v1 device factory * Split DeviceBuildResult devices and connection specs Makes access to the devices easier and makes it easier to build parameters * Remove device_type property from factories * Include fixture overrides in built devices * Fix duplication in factory repr * Add initial device_manager tests * Revert adsim changes for now * Enough tests to get full coverage Maybe enough tests to cover basic use * Create DeviceManager in fixture * Add test for docstrings * Reformat tests * Linting et al * Support single device manager name in CLI * Ignore mock type warnings * Appease pre-commit lints * Add tests for device manager use in CLI * Make 'devices' default name for device manager in CLI * Clean up TODO comments * Set return_value when creating mocks * Fix typing in v1_init decorator * Use ParamSpec when passing through __call__ * Handle or ignore var args * Update tests * Rename ignore skip test * Simplify LazyFixture __getitem__ * Used UserDict as base class for LazyFixtures * Make pyright happy * Remove need for type ignore --------- Co-authored-by: Robert Tuck <robert.tuck@diamond.ac.uk>

* Eiger changes from i04 beamline * Add test for two eiger stops being called at the same time * Revert async arming changes * Fix tests --------- Co-authored-by: Dominic Oram <dominic.oram@diamond.ac.uk>

* first try * add trigger function which writes to soft signals * small changes * get rid of max pixel location * add tests - need modifying as made with copilot * make triggerable for blesky plan * start to add max-pixel device in i04 (WIP because on beamline) * Finish adding device to i04.py * Add changes from beamline testing - from github * add comment for kernal size * attemt own tests * add my tests * add assert called onece tests * make changes from Dom's comments * add greyscale test * remove print statements in test * Fix merge issue --------- Co-authored-by: Dominic Oram <dominic.oram@diamond.ac.uk>

…n as the max pixel device (to blur and convert to greyscale)

srishtysajeev · 2025-12-05T18:47:14Z

src/dodal/devices/oav/oav_detector.py


        # Level is the string description of the zoom level e.g. "1.0x" or "1.0"
        self.level = epics_signal_rw(str, f"{prefix}MP:SELECT")
+


Need to refactor this to make more concise

Yes, additionally they don't exist on i03 so we will need to make them optional

DominicOram

Comments mostly reminders to myself

src/dodal/devices/i04/beam_centre.py

src/dodal/devices/oav/oav_detector.py

DominicOram · 2025-12-10T18:09:01Z

src/dodal/devices/oav/oav_detector.py


        # Level is the string description of the zoom level e.g. "1.0x" or "1.0"
        self.level = epics_signal_rw(str, f"{prefix}MP:SELECT")
+


Yes, additionally they don't exist on i03 so we will need to make them optional

DominicOram · 2025-12-10T18:11:02Z

tests/devices/i04/test_beam_centre.py

+contour_array = np.array(
+    [[[10, 10]], [[10, 50]], [[50, 50]], [[50, 10]]], dtype=np.uint8
+)


Should: This isn't used, remove

DominicOram · 2025-12-10T18:12:36Z

tests/devices/i04/test_beam_centre.py

+from ophyd_async.core import init_devices
+
+from dodal.devices.i04.beam_centre import CentreEllipseMethod, binary_img
+


Good tests, I would probably add one that is the whole thing from realistic data through to an actual centre

…mondLightSource/dodal into 1726_beam_centre_device_post_testing

codecov · 2025-12-16T14:40:10Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.12%. Comparing base (f919ae5) to head (cfe6770).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1757   +/-   ##
=======================================
  Coverage   99.12%   99.12%           
=======================================
  Files         282      283    +1     
  Lines       10683    10750   +67     
=======================================
+ Hits        10589    10656   +67     
  Misses         94       94

☔ 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.

olliesilvester

Nice PR - only some minor comments

olliesilvester · 2025-12-18T11:33:28Z

src/dodal/devices/i04/beam_centre.py

+
+        largest_contour = max(contours, key=cv2.contourArea)
+        if len(largest_contour) < 5:
+            raise ValueError("Not enough points to fit an ellipse.")


nit: error message could include {largest_contour}

olliesilvester · 2025-12-18T11:34:54Z

src/dodal/devices/i04/beam_centre.py

+ADDITIONAL_BINARY_THRESH = 20
+
+
+def convert_image_to_binary(image: np.ndarray):


Could: Move this function to oav/utils.py

olliesilvester · 2025-12-18T11:36:22Z

src/dodal/devices/oav/utils.py

+
+    # kernel size describes how many of the neighbouring pixels are used for the blur,
+    # higher kernal size means more of a blur effect
+    kernel_size = (7, 7)


Could: Move this into a constant

olliesilvester · 2025-12-18T11:38:10Z

src/dodal/devices/i04/max_pixel.py

-        arr = await self._convert_to_gray_and_blur()
+        img_data = await self.array_data.get_value()
+        arr = convert_to_gray_and_blur(img_data)
        max_val = float(np.max(arr))  # np.int64


Not sure what the # np.int64 comment is saying?

olliesilvester · 2025-12-18T11:44:26Z

tests/devices/i04/test_beam_centre.py

+
+@patch("dodal.devices.i04.beam_centre.CentreEllipseMethod._fit_ellipse")
+@patch("dodal.devices.i04.beam_centre.convert_image_to_binary")
+async def test_trigger_converts_to_binary_then_finds_ellipse(


Nit: I think for these types of tests its better to use a parent magic mock so that we can also check the order

olliesilvester · 2025-12-18T11:45:57Z

tests/devices/i04/test_beam_centre.py

+
+@patch("dodal.devices.i04.beam_centre.cv2.threshold")
+@patch("dodal.devices.i04.beam_centre.convert_to_gray_and_blur")
+async def test_convert_image_to_binary_calls_threshold_twice(


nit: Can also check that mock_convert has been called once here

srishtysajeev and others added 22 commits November 25, 2025 18:42

Add centring with the ellipse method as a device

47bbbb7

Prevent Eiger from being disarmed multiple times at once (#1719)

a23ec51

* Eiger changes from i04 beamline * Add test for two eiger stops being called at the same time * Revert async arming changes * Fix tests --------- Co-authored-by: Dominic Oram <dominic.oram@diamond.ac.uk>

test planning

1693295

change file names

5b68f28

change so that the centring deviceuses the same preprocessing functio…

bda598e

…n as the max pixel device (to blur and convert to greyscale)

fix max pixel test which was failing after function change

9cebf19

add more tests

ceb2a4c

more tests

903cfef

tests working now

eec0c2e

merge main

228c151

fix: linting issues and add more to docstring

717748c

fix import

873c64f

fix: patch

b1573f1

add trigger test

dc2230a

add device to i04.py

7bc5f7e

Add changes from tests

edf985a

responding to Ollie comments

ccbb8f5

get rid of comments from beamline testing

84d6203

fix issue relating to using time.sleep and use asyncio.sleep instead

d98585d

get rid of uneeded awaits

13ae58a

srishtysajeev requested a review from a team as a code owner December 4, 2025 13:09

srishtysajeev requested a review from olliesilvester December 4, 2025 13:09

change lints

114c110

srishtysajeev commented Dec 5, 2025

View reviewed changes

olliesilvester mentioned this pull request Dec 9, 2025

Add i04 device which finds the centre of the beam from OAV camera by fitting ellipses #1749

Closed

4 tasks

DominicOram mentioned this pull request Dec 9, 2025

Add plan for automatically finding the beam centre on the OAV DiamondLightSource/mx-bluesky#1496

Open

DominicOram reviewed Dec 10, 2025

View reviewed changes

Tidy some things up from review

1ec8f45

DominicOram added 2 commits December 15, 2025 13:30

Separate zoom controller out

95016cd

Tidy up and add test

8c83d25

DominicOram changed the title ~~1726 beam centre device post testing~~ Add a device that will calculate a beam centre Dec 15, 2025

DominicOram added 5 commits December 15, 2025 15:27

Merge branch 'main' into 1726_beam_centre_device_post_testing

136dac1

Use a small move time for mock zoom controller

103780c

Merge branch '1726_beam_centre_device_post_testing' of github.com:Dia…

a7ccf95

…mondLightSource/dodal into 1726_beam_centre_device_post_testing

Fix import

fe095b3

Fix tests

0f23ada

DominicOram and others added 3 commits December 16, 2025 14:47

Fix linting

4b78eb2

Fix linting

9d2e62f

Merge branch 'main' into 1726_beam_centre_device_post_testing

cfe6770

olliesilvester approved these changes Dec 18, 2025

View reviewed changes


		# Level is the string description of the zoom level e.g. "1.0x" or "1.0"
		self.level = epics_signal_rw(str, f"{prefix}MP:SELECT")

		from ophyd_async.core import init_devices

		from dodal.devices.i04.beam_centre import CentreEllipseMethod, binary_img

		ADDITIONAL_BINARY_THRESH = 20


		def convert_image_to_binary(image: np.ndarray):

Add a device that will calculate a beam centre #1757

Are you sure you want to change the base?

Add a device that will calculate a beam centre #1757

Uh oh!

Conversation

srishtysajeev commented Dec 4, 2025

Instructions to reviewer on how to test:

Checks for reviewer

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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

Uh oh!

Codecov Report

Uh oh!

olliesilvester 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Dec 16, 2025 •

edited

Loading