Add ROI logic to i04 beam centre device by olliesilvester · Pull Request #1799 · DiamondLightSource/dodal

olliesilvester · 2025-12-19T14:55:28Z

Adds ROI logic to beam centre device. Should fix issues we were seeing at zoom level 1.0

This was done by @srishtysajeev - I'm making a draft PR so MX daq remember to finish it off

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)

…ic_new

codecov · 2026-01-20T15:11:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.05%. Comparing base (61abb37) to head (47e8e49).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1799   +/-   ##
=======================================
  Coverage   99.05%   99.05%           
=======================================
  Files         289      289           
  Lines       11000    11020   +20     
=======================================
+ Hits        10896    10916   +20     
  Misses        104      104

☔ 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

Great thanks! I can't actually approve this as it's my own PR but otherwise I would. Just a few comments.

Discussed in person: We might want to add an option in the plan to specify which zoom levels we want to use ROI for, but it's probably fine to always use it. Also, it would be good to properly close off this bit of work for now

olliesilvester · 2026-01-20T15:08:20Z

+
+    # Clip coordinates to stay within bounds
+    x_min = max(round_half_up(centre_x - x_dist), 0)
+    x_max = min(round_half_up(centre_x + x_dist), width) - 1


what's the -1 for? Do we need it given that we do a +1 again on line 91?

Yeah the -1 is just so x_min and x_max are semantically the same, i.e the min/max pixel included in the ROI array. I agree it's a bit weird given we have the +1 later on, but the alternative is returning
(x_max - 1, y_max - 1) for the bottom right corner coord of the box, which I think is more confusing.

Makes sense

olliesilvester · 2026-01-20T15:13:54Z

+            int, f"{prefix}OVER:{overlay_channel}:CenterY"
+        )
+
+        self.roi_box_size = soft_signal_rw(int, 300)


Does this need to be a soft signal, maybe it's better just as an attribute?

Not sure of the pros/cons of either tbh, happy to change it?

Ah it's fine as-is

olliesilvester · 2026-01-20T15:20:38Z

CI test failure looks unrelated, but linting is related. I will fix linting

jacob720

Approved as Ollie's not allowed to

srishtysajeev and others added 30 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

initial commit

04e4fbe

add logic for converting centre back to original image

a416832

get rid of comment

3843fb4

add logic to make sure that you stay within the bounds of the image

863f37e

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

change lints

114c110

Merge branch '1726_beam_centre_device_post_testing' into 1722_ROI_log…

6a612ae

…ic_new

fix: roi logic and add tests

edd1727

change docstring

5203096

Add comment on how you could use a different PV for the previous centre

ae0b81a

olliesilvester changed the title ~~At ROI logic~~ Add ROI logic to i04 beam centre device (wip) Dec 19, 2025

Base automatically changed from 1726_beam_centre_device_post_testing to main January 8, 2026 15:04

olliesilvester and others added 8 commits January 15, 2026 16:45

Update branch

76b59d5

Commit missed file

ee114e8

Fixes from merge

98c49c3

Merge branch 'main' into 1722_ROI_logic_new

c27d305

Small changes and docstring

db3af90

Parameterise tests

7f84c60

Fixes

ed839cf

Add test

dd07062

jacob720 marked this pull request as ready for review January 20, 2026 10:46

jacob720 requested a review from a team as a code owner January 20, 2026 10:46

Merge branch 'main' into 1722_ROI_logic_new

13da451

olliesilvester self-assigned this Jan 20, 2026

olliesilvester commented Jan 20, 2026

View reviewed changes

Comment thread src/dodal/devices/oav/oav_detector.py Outdated

Fix logger

ee076ec

olliesilvester commented Jan 20, 2026

View reviewed changes

linting

09fb08a

jacob720 approved these changes Jan 20, 2026

View reviewed changes

Merge branch 'main' into 1722_ROI_logic_new

47e8e49

jacob720 enabled auto-merge (squash) January 20, 2026 16:42

jacob720 changed the title ~~Add ROI logic to i04 beam centre device (wip)~~ Add ROI logic to i04 beam centre device Jan 20, 2026

jacob720 merged commit 65c5fae into main Jan 20, 2026
14 of 18 checks passed

jacob720 deleted the 1722_ROI_logic_new branch January 20, 2026 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ROI logic to i04 beam centre device#1799

Add ROI logic to i04 beam centre device#1799
jacob720 merged 43 commits intomainfrom
1722_ROI_logic_new

olliesilvester commented Dec 19, 2025

Uh oh!

Uh oh!

codecov Bot commented Jan 20, 2026 •

edited

Loading

Uh oh!

olliesilvester left a comment

Uh oh!

olliesilvester Jan 20, 2026

Uh oh!

jacob720 Jan 20, 2026 •

edited

Loading

Uh oh!

olliesilvester Jan 20, 2026

Uh oh!

olliesilvester Jan 20, 2026

Uh oh!

jacob720 Jan 20, 2026

Uh oh!

olliesilvester Jan 20, 2026

Uh oh!

olliesilvester commented Jan 20, 2026

Uh oh!

jacob720 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

olliesilvester commented Dec 19, 2025

Checks for reviewer

Uh oh!

Uh oh!

codecov Bot commented Jan 20, 2026 • 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!

olliesilvester Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

jacob720 Jan 20, 2026 • 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!

olliesilvester Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

olliesilvester Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

jacob720 Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

olliesilvester Jan 20, 2026

Choose a reason for hiding this comment

Uh oh!

olliesilvester commented Jan 20, 2026

Uh oh!

jacob720 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Jan 20, 2026 •

edited

Loading

jacob720 Jan 20, 2026 •

edited

Loading