Refactor and create ophyd-async FGS devices #422
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
==========================================
+ Coverage 93.05% 93.24% +0.19%
==========================================
Files 90 89 -1
Lines 3325 3243 -82
==========================================
- Hits 3094 3024 -70
+ Misses 231 219 -12 ☔ View full report in Codecov by Sentry. |
olliesilvester
force-pushed
the
413_make_common_FGS
branch
from
b852925
to
9817eae
Compare
olliesilvester
force-pushed
the
413_make_common_FGS
branch
from
bc572dc
to
ec374d2
Compare
src/dodal/devices/fast_grid_scan.py
Outdated
| def set_fast_grid_scan_params(scan: FastGridScanCommon, params: GridScanParamsCommon): | ||
| assert set(params.get_param_positions().keys()) == set( | ||
| scan.movable_params.keys() | ||
| ), "Scan parameters don't match the scan device" |
There was a problem hiding this comment.
Could: We can get the type checker to check this.
ParamType = TypeVar("ParamType", bound=GridScanParamsCommon)
class FastGridScanCommon(Generic[ParamType], StandardReadable, ABC):
...
class ZebraFastGridScan(FastGridScanCommon[ZebraGridScanParams]):
....
class PandAFastGridScan(FastGridScanCommon[PandAGridScanParams]):
....
def set_fast_grid_scan_params(scan: FastGridScanCommon[ParamType], params: ParamType):
....Which will make the type checker highlight code like
assert isinstance(parameters.experiment_params, PandAGridScanParams)
yield from set_flyscan_params(
fgs_composite.zebra_fast_grid_scan, parameters.experiment_params
)I would be interested to see if @callumforrester or @dperl-dls have thoughts on using the type system like this.
src/dodal/devices/fast_grid_scan.py
Outdated
|
|
||
| def set_fast_grid_scan_params( | ||
| scan: FastGridScanCommon[ParamType], params: GridScanParamsCommon |
There was a problem hiding this comment.
Must: params should be of type ParamType. This means we can then remove the assert and just trust the type system to catch when the params don't match
src/dodal/devices/fast_grid_scan.py
Outdated
| self.parent = parent | ||
|
|
||
| async def get_value(self): | ||
| x = int(await self.parent.x_steps.get_value()) # type: ignore | ||
| y = int(await self.parent.y_steps.get_value()) # type: ignore | ||
| z = int(await self.parent.z_steps.get_value()) # type: ignore | ||
| first_grid = x * y | ||
| second_grid = x * z | ||
| return first_grid + second_grid |
There was a problem hiding this comment.
Should: I think if you specify the type of the parent you can fix these type issues:
| self.parent = parent | |
| async def get_value(self): | |
| x = int(await self.parent.x_steps.get_value()) # type: ignore | |
| y = int(await self.parent.y_steps.get_value()) # type: ignore | |
| z = int(await self.parent.z_steps.get_value()) # type: ignore | |
| first_grid = x * y | |
| second_grid = x * z | |
| return first_grid + second_grid | |
| self.parent: "FastGridScanCommon" = parent | |
| async def get_value(self): | |
| x = await self.parent.x_steps.get_value() | |
| y = await self.parent.y_steps.get_value() | |
| z = await self.parent.z_steps.get_value() | |
| first_grid = x * y | |
| second_grid = x * z | |
| return first_grid + second_grid |
| status.wait() | ||
| assert status.exception() is None | ||
| @pytest.fixture | ||
| async def panda_fast_grid_scan(request): |
There was a problem hiding this comment.
Should: Why do we need request here and in the other fixture? They don't seem to be used
I'll add a few now - actually, these tests won't change! |
| @pytest.mark.parametrize( | ||
| "use_pgs", | ||
| [(False), (True)], | ||
| ) | ||
| async def test_given_settings_valid_when_kickoff_then_run_started( | ||
| use_pgs, | ||
| zebra_fast_grid_scan: ZebraFastGridScan, | ||
| panda_fast_grid_scan: PandAFastGridScan, | ||
| ): | ||
| set_mock_value(zebra_fast_grid_scan.scan_invalid, False) | ||
| set_mock_value(zebra_fast_grid_scan.position_counter, 0) | ||
| set_mock_value(zebra_fast_grid_scan.status, 1) | ||
| grid_scan: ZebraFastGridScan | PandAFastGridScan = ( | ||
| panda_fast_grid_scan if use_pgs else zebra_fast_grid_scan | ||
| ) |
There was a problem hiding this comment.
Nit: So I think you could be clever here with fixtures (https://engineeringfordatascience.com/posts/pytest_fixtures_with_parameterize/#using-requestgetfixturevalue-) and do something like:
@pytest.mark.parametrize(
"grid_scan",
["zebra_fast_grid_scan", "panda_fast_grid_scan"],
)
async def test_given_settings_valid_when_kickoff_then_run_started(
grid_scan_type: str, request
):
grid_scan: ZebraFastGridScan | PandAFastGridScan = request.getfixturevalue(grid_scan_type)But I appreciate that this may cause more confusion/wackiness down the line so leave it up to you if you think it's nicer or not.
There was a problem hiding this comment.
I just had a play with this, I think there are some issues with using request.getfixturevalue with asyncio fixtures. There's probably ways around this, but I'll leave it for now
Fixes #413
See associated Hyperion change at: DiamondLightSource/hyperion#1294
Combines the FGS parameters and devices for panda and zebra scans, and converts them to ophyd-async.
Instructions to reviewer on how to test:
Checks for reviewer