-
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #857 +/- ##
==========================================
- Coverage 95.59% 95.53% -0.06%
==========================================
Files 125 127 +2
Lines 5422 5378 -44
==========================================
- Hits 5183 5138 -45
- Misses 239 240 +1 ☔ 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)
) -> 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
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}