-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Finish setting up an ophyd_async OAV #857
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #857 +/- ##
==========================================
+ Coverage 95.53% 95.60% +0.07%
==========================================
Files 124 125 +1
Lines 5304 5236 -68
==========================================
- Hits 5067 5006 -61
+ Misses 237 230 -7 ☔ View full report in Codecov by Sentry. |
I realised once I started updating the hyperion side that we do set some of the Also, another pair of eyes on the codecov patch would help, please. I think I covered all the old tests and I can't figure out what's missing that makes the coverage decrease... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks. Almost there but I added a few comments
self.color_mode = epics_signal_rw(ColorMode, "GC_BalRatioSelector") | ||
self.acquire_period = epics_signal_rw(float, "AcquirePeriod") | ||
self.acquire_time = epics_signal_rw(float, "AcquireTime") | ||
self.gain = epics_signal_rw(float, "Gain") | ||
|
||
self.array_size_x = epics_signal_r(int, "ArraySizeX_RBV") | ||
self.array_size_y = epics_signal_r(int, "ArraySizeY_RBV") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must: these aren't connecting in dodal connect i03
as they need the PV prefix
self.x_size = epics_signal_r(int, prefix + "ArraySize1_RBV") | ||
self.y_size = epics_signal_r(int, prefix + "ArraySize2_RBV") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must: these aren't connecting in dodal connect i03
as you're missing a colon
# TODO check type of these two | ||
self.input_rbpv = epics_signal_r(str, prefix + "NDArrayPort_RBV") | ||
self.input_plugin = epics_signal_rw(str, prefix + "NDArrayPort") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: These two PVs aren't used anyway, lets just remove them
await self.last_saved_path.set(path, wait=True) | ||
|
||
@AsyncStatus.wrap | ||
async def trigger(self): | ||
"""This takes a snapshot image from the MJPG stream and send it to the | ||
post_processing method, expected to be implemented by a child of this class. | ||
|
||
It is the responsibility of the child class to save any resulting images. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: by calling _save_image
if not response.ok: | ||
LOGGER.error( | ||
f"OAV responded with {response.status}: {response.reason}." | ||
) | ||
raise ClientConnectionError( | ||
f"OAV responded with {response.status}: {response.reason}." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: I think we can just do response.raise_for_status()
or actually ClientSession(raise_for_status=True)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried using this response.raise_for_status()
and couldn't make it work, all the tests kept failing because they couldn't get past it - even when mocking it. Haven't managed to figure out why yet but this at least did the job. Didn't know it could be passed as an argument to ClientSession
so will try that
src/dodal/devices/oav/utils.py
Outdated
) -> np.ndarray: | ||
beam_distance_px: Pixel = oav_params.calculate_beam_distance(*pixel) | ||
beam_distance_px: Pixel = calculate_beam_distance((beam_centre), *pixel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: These brackets seem unrequired
system_tests/test_oav_system.py
Outdated
oav_config = OAVConfig(ZOOM_LEVELS_XML, DISPLAY_CONFIGURATION) | ||
async with DeviceCollector(): | ||
oav = OAV("", config=oav_config, name="oav") | ||
await oav.connect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: DeviceCollector
already does the connect for you
autospec=True, | ||
) | ||
@patch("dodal.devices.areadetector.plugins.MJPG.Image") | ||
async def test_snapshot_trigger_fails_in_post_processing_withouth_raising_error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Typo on without
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: I also think we probably do want it to raise
# Set new directory and test that it's created | ||
set_mock_value(snapshot.directory, "new_dir") | ||
|
||
await snapshot.trigger() | ||
|
||
mock_mkdir.assert_called_once() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I would test this in a separate test that's just testing a new folder is created i.e. like test_given_directory_not_existing_when_snapshot_triggered_then_directory_created
mock_get.return_value.__aenter__.return_value = (mock_response := AsyncMock()) | ||
mock_response.ok = True | ||
mock_response.read.return_value = (test_data := b"TEST") | ||
|
||
mock_aio_open = mock_aiofiles.open | ||
mock_aio_open.return_value.__aenter__.return_value = (mock_file := AsyncMock()) | ||
|
||
mock_open = patch_image.open | ||
mock_open.return_value.__aenter__.return_value = test_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: You do this mocking a few times, I think pull it into a helper function
Closes #716
Closes #824
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}