Add a device that will calculate a beam centre #1757
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* 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
commented
Dec 5, 2025
|
|
||
| # 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") | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Need to refactor this to make more concise
Contributor
There was a problem hiding this comment.
Yes, additionally they don't exist on i03 so we will need to make them optional
4 tasks
DominicOram
reviewed
Dec 10, 2025
Contributor
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Comments mostly reminders to myself
|
|
||
| # 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") | ||
|
|
Contributor
There was a problem hiding this comment.
Yes, additionally they don't exist on i03 so we will need to make them optional
Comment on lines
39
to
41
| contour_array = np.array( | ||
| [[[10, 10]], [[10, 50]], [[50, 50]], [[50, 10]]], dtype=np.uint8 | ||
| ) |
Contributor
There was a problem hiding this comment.
Should: This isn't used, remove
| from ophyd_async.core import init_devices | ||
|
|
||
| from dodal.devices.i04.beam_centre import CentreEllipseMethod, binary_img | ||
|
|
Contributor
There was a problem hiding this comment.
Good tests, I would probably add one that is the whole thing from realistic data through to an actual centre
DominicOram
changed the title
…mondLightSource/dodal into 1726_beam_centre_device_post_testing
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1757 +/- ##
=======================================
Coverage 99.12% 99.13%
=======================================
Files 286 287 +1
Lines 10794 10861 +67
=======================================
+ Hits 10700 10767 +67
Misses 94 94 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
olliesilvester
approved these changes
Dec 18, 2025
Collaborator
olliesilvester
left a comment
olliesilvester
left a comment
There was a problem hiding this comment.
Nice PR - only some minor comments
|
|
||
| # 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) |
Collaborator
There was a problem hiding this comment.
Could: Move this into a constant
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1726
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}