From 4dad4fd63a3637acd58fda19721acb18bfddce6b Mon Sep 17 00:00:00 2001 From: Srishty Date: Wed, 24 Sep 2025 12:59:44 +0000 Subject: [PATCH 01/11] Add scint into beam + change Enum to be be more descriptive compared to just In and Out. --- src/dodal/devices/scintillator.py | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/src/dodal/devices/scintillator.py b/src/dodal/devices/scintillator.py index 0dc76c27026..6daf600245e 100644 --- a/src/dodal/devices/scintillator.py +++ b/src/dodal/devices/scintillator.py @@ -8,9 +8,10 @@ class InOut(StrictEnum): - """Currently Hyperion only needs to move the scintillator out for data collection.""" + """Moves scintillator in and out of the beam""" - OUT = "Out" + OUT = "Out_Beam" + IN = "In_Beam" UNKNOWN = "Unknown" @@ -45,6 +46,9 @@ def __init__( self._scintillator_out_yz_mm = [ float(beamline_parameters[f"scin_{axis}_SCIN_OUT"]) for axis in ("y", "z") ] + self._scintillator_in_yz_mm = [ + float(beamline_parameters[f"scin_{axis}_SCIN_IN"]) for axis in ("y", "z") + ] self._yz_tolerance_mm = [ float(beamline_parameters[f"scin_{axis}_tolerance"]) for axis in ("y", "z") ] @@ -63,6 +67,18 @@ def _get_selected_position(self, y: float, z: float) -> InOut: ) ): return InOut.OUT + + elif all( + isclose(axis_pos, axis_in_beam, abs_tol=axis_tolerance) + for axis_pos, axis_in_beam, axis_tolerance in zip( + current_pos, + self._scintillator_in_yz_mm, + self._yz_tolerance_mm, + strict=False, + ) + ): + return InOut.IN + else: return InOut.UNKNOWN @@ -78,5 +94,15 @@ async def _set_selected_position(self, position: InOut) -> None: ) await self.y_mm.set(self._scintillator_out_yz_mm[0]) await self.z_mm.set(self._scintillator_out_yz_mm[1]) + case InOut.IN: + if ( + self._aperture_scatterguard().selected_aperture.get_value() + != ApertureValue.PARKED + ): + raise ValueError( + "Cannot move scintillator out if aperture/scatterguard is not parked" + ) + await self.z_mm.set(self._scintillator_in_yz_mm[1]) + await self.y_mm.set(self._scintillator_in_yz_mm[0]) case _: raise ValueError(f"Cannot set scintillator to position {position}") From 107650d2f1048cbc781b77e6868aa187c6ef611b Mon Sep 17 00:00:00 2001 From: Srishty Date: Wed, 24 Sep 2025 13:14:45 +0000 Subject: [PATCH 02/11] Get rid of Enum change as this has to match what the PV is expecting --- src/dodal/devices/scintillator.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/dodal/devices/scintillator.py b/src/dodal/devices/scintillator.py index 6daf600245e..fa6728c1a7a 100644 --- a/src/dodal/devices/scintillator.py +++ b/src/dodal/devices/scintillator.py @@ -8,10 +8,10 @@ class InOut(StrictEnum): - """Moves scintillator in and out of the beam""" + """Moves scintillator in and out of the beam.""" - OUT = "Out_Beam" - IN = "In_Beam" + OUT = "Out" + IN = "In" UNKNOWN = "Unknown" From 10e8f8cbbb8afa9addb931dd0b942258fcf3a851 Mon Sep 17 00:00:00 2001 From: Srishty Date: Wed, 24 Sep 2025 13:17:46 +0000 Subject: [PATCH 03/11] Add comments for what In and Out mean for the scint --- src/dodal/devices/scintillator.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/dodal/devices/scintillator.py b/src/dodal/devices/scintillator.py index fa6728c1a7a..a3365fca06a 100644 --- a/src/dodal/devices/scintillator.py +++ b/src/dodal/devices/scintillator.py @@ -9,9 +9,9 @@ class InOut(StrictEnum): """Moves scintillator in and out of the beam.""" - - OUT = "Out" - IN = "In" + + OUT = "Out" #Out of beam + IN = "In" #In to beam UNKNOWN = "Unknown" From e743339e0c578c84643c88739d3bd67a4f9e1a76 Mon Sep 17 00:00:00 2001 From: Srishty Date: Fri, 26 Sep 2025 09:22:40 +0000 Subject: [PATCH 04/11] small change --- src/dodal/devices/scintillator.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/dodal/devices/scintillator.py b/src/dodal/devices/scintillator.py index a3365fca06a..c70f272b7ed 100644 --- a/src/dodal/devices/scintillator.py +++ b/src/dodal/devices/scintillator.py @@ -9,9 +9,9 @@ class InOut(StrictEnum): """Moves scintillator in and out of the beam.""" - - OUT = "Out" #Out of beam - IN = "In" #In to beam + + OUT = "Out" # Out of beam + IN = "In" # In to beam UNKNOWN = "Unknown" @@ -67,7 +67,7 @@ def _get_selected_position(self, y: float, z: float) -> InOut: ) ): return InOut.OUT - + elif all( isclose(axis_pos, axis_in_beam, abs_tol=axis_tolerance) for axis_pos, axis_in_beam, axis_tolerance in zip( @@ -78,7 +78,7 @@ def _get_selected_position(self, y: float, z: float) -> InOut: ) ): return InOut.IN - + else: return InOut.UNKNOWN @@ -100,7 +100,7 @@ async def _set_selected_position(self, position: InOut) -> None: != ApertureValue.PARKED ): raise ValueError( - "Cannot move scintillator out if aperture/scatterguard is not parked" + "Cannot move scintillator in if aperture/scatterguard is not parked" ) await self.z_mm.set(self._scintillator_in_yz_mm[1]) await self.y_mm.set(self._scintillator_in_yz_mm[0]) From 2a229420fc89e3699705c0fddf2cf1bf65b990e8 Mon Sep 17 00:00:00 2001 From: Srishty Date: Fri, 26 Sep 2025 10:42:51 +0000 Subject: [PATCH 05/11] add tests for scint in --- tests/devices/test_scintillator.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/devices/test_scintillator.py b/tests/devices/test_scintillator.py index d11c55e451a..b48cf8e0310 100644 --- a/tests/devices/test_scintillator.py +++ b/tests/devices/test_scintillator.py @@ -46,6 +46,7 @@ async def scintillator_and_ap_sg( @pytest.mark.parametrize( "y, z, expected_position", [ + (100.855, 101.5115, InOut.IN), (-0.02, 0.1, InOut.OUT), (0.1, 0.1, InOut.UNKNOWN), (10.2, 15.6, InOut.UNKNOWN), @@ -84,6 +85,11 @@ async def test_given_aperture_scatterguard_parked_when_set_to_out_position_then_ assert await scintillator.y_mm.user_setpoint.get_value() == -0.02 assert await scintillator.z_mm.user_setpoint.get_value() == 0.1 + await scintillator.selected_pos.set(InOut.IN) + + assert await scintillator.y_mm.user_setpoint.get_value() == -100.855 + assert await scintillator.z_mm.user_setpoint.get_value() == 101.5115 + async def test_given_aperture_scatterguard_not_parked_when_set_to_out_position_then_exception_raised( scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], @@ -92,6 +98,8 @@ async def test_given_aperture_scatterguard_not_parked_when_set_to_out_position_t if position != ApertureValue.PARKED: scintillator, ap_sg = scintillator_and_ap_sg ap_sg.return_value.selected_aperture.get_value.return_value = position # type: ignore - + # there is no assert so why does this work with pytest.raises(ValueError): await scintillator.selected_pos.set(InOut.OUT) + with pytest.raises(ValueError): + await scintillator.selected_pos.set(InOut.IN) From 166966ddd018834cf4f01db955bc1e7518f6b5a0 Mon Sep 17 00:00:00 2001 From: Srishty Date: Fri, 26 Sep 2025 13:15:58 +0000 Subject: [PATCH 06/11] get rid of comment! --- tests/devices/test_scintillator.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/devices/test_scintillator.py b/tests/devices/test_scintillator.py index b48cf8e0310..c0f3fe93418 100644 --- a/tests/devices/test_scintillator.py +++ b/tests/devices/test_scintillator.py @@ -87,7 +87,7 @@ async def test_given_aperture_scatterguard_parked_when_set_to_out_position_then_ await scintillator.selected_pos.set(InOut.IN) - assert await scintillator.y_mm.user_setpoint.get_value() == -100.855 + assert await scintillator.y_mm.user_setpoint.get_value() == 100.855 assert await scintillator.z_mm.user_setpoint.get_value() == 101.5115 @@ -98,7 +98,6 @@ async def test_given_aperture_scatterguard_not_parked_when_set_to_out_position_t if position != ApertureValue.PARKED: scintillator, ap_sg = scintillator_and_ap_sg ap_sg.return_value.selected_aperture.get_value.return_value = position # type: ignore - # there is no assert so why does this work with pytest.raises(ValueError): await scintillator.selected_pos.set(InOut.OUT) with pytest.raises(ValueError): From b6f901fae5b85449638a71178a8bc3825ebd7269 Mon Sep 17 00:00:00 2001 From: Srishty Date: Fri, 26 Sep 2025 15:26:56 +0000 Subject: [PATCH 07/11] make changes to scintillator.py from comments --- src/dodal/devices/scintillator.py | 49 +++++++++++++------------------ 1 file changed, 21 insertions(+), 28 deletions(-) diff --git a/src/dodal/devices/scintillator.py b/src/dodal/devices/scintillator.py index c70f272b7ed..561bb2d6ace 100644 --- a/src/dodal/devices/scintillator.py +++ b/src/dodal/devices/scintillator.py @@ -55,53 +55,46 @@ def __init__( super().__init__(name) - def _get_selected_position(self, y: float, z: float) -> InOut: - current_pos = [y, z] - if all( + def check_position( + self, current_pos: tuple[float, float], pos_to_check: tuple[float, float] + ): + return all( isclose(axis_pos, axis_in_beam, abs_tol=axis_tolerance) for axis_pos, axis_in_beam, axis_tolerance in zip( current_pos, - self._scintillator_out_yz_mm, + pos_to_check, self._yz_tolerance_mm, strict=False, ) - ): + ) + + def _get_selected_position(self, y: float, z: float) -> InOut: + current_pos = [y, z] + if self.check_position(current_pos, self._scintillator_out_yz_mm): return InOut.OUT - elif all( - isclose(axis_pos, axis_in_beam, abs_tol=axis_tolerance) - for axis_pos, axis_in_beam, axis_tolerance in zip( - current_pos, - self._scintillator_in_yz_mm, - self._yz_tolerance_mm, - strict=False, - ) - ): + elif self.check_position(current_pos, self._scintillator_in_yz_mm): return InOut.IN else: return InOut.UNKNOWN + def check_aperture_parked(self): + if ( + self._aperture_scatterguard().selected_aperture.get_value() + != ApertureValue.PARKED + ): + raise ValueError( + "Cannot move scintillator out if aperture/scatterguard is not parked" + ) + async def _set_selected_position(self, position: InOut) -> None: + self.check_aperture_parked() match position: case InOut.OUT: - if ( - self._aperture_scatterguard().selected_aperture.get_value() - != ApertureValue.PARKED - ): - raise ValueError( - "Cannot move scintillator out if aperture/scatterguard is not parked" - ) await self.y_mm.set(self._scintillator_out_yz_mm[0]) await self.z_mm.set(self._scintillator_out_yz_mm[1]) case InOut.IN: - if ( - self._aperture_scatterguard().selected_aperture.get_value() - != ApertureValue.PARKED - ): - raise ValueError( - "Cannot move scintillator in if aperture/scatterguard is not parked" - ) await self.z_mm.set(self._scintillator_in_yz_mm[1]) await self.y_mm.set(self._scintillator_in_yz_mm[0]) case _: From a7612b521665fad85bb3a38e8044b373951c8e02 Mon Sep 17 00:00:00 2001 From: Srishty Date: Fri, 26 Sep 2025 15:38:59 +0000 Subject: [PATCH 08/11] make changes from comments to test_scintillator.py --- tests/devices/test_scintillator.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/devices/test_scintillator.py b/tests/devices/test_scintillator.py index c0f3fe93418..658d28911fe 100644 --- a/tests/devices/test_scintillator.py +++ b/tests/devices/test_scintillator.py @@ -4,6 +4,7 @@ import pytest from ophyd_async.core import init_devices +from ophyd_async.testing import assert_value from dodal.common.beamlines.beamline_parameters import GDABeamlineParameters from dodal.devices.aperturescatterguard import ApertureScatterguard, ApertureValue @@ -82,13 +83,20 @@ async def test_given_aperture_scatterguard_parked_when_set_to_out_position_then_ await scintillator.selected_pos.set(InOut.OUT) - assert await scintillator.y_mm.user_setpoint.get_value() == -0.02 - assert await scintillator.z_mm.user_setpoint.get_value() == 0.1 + await assert_value(scintillator.y_mm.user_setpoint, -0.02) + await assert_value(scintillator.z_mm.user_setpoint, -0.1) + + +async def test_given_aperture_scatterguard_parked_when_set_to_in_position_then_returns_expected( + scintillator_and_ap_sg: tuple[Scintillator, ApertureScatterguard], +): + scintillator, ap_sg = scintillator_and_ap_sg + ap_sg.return_value.selected_aperture.get_value.return_value = ApertureValue.PARKED # type: ignore await scintillator.selected_pos.set(InOut.IN) - assert await scintillator.y_mm.user_setpoint.get_value() == 100.855 - assert await scintillator.z_mm.user_setpoint.get_value() == 101.5115 + await assert_value(scintillator.y_mm.user_setpoint, 100.855) + await assert_value(scintillator.z_mm.user_setpoint, 101.5115) async def test_given_aperture_scatterguard_not_parked_when_set_to_out_position_then_exception_raised( From 416de30ee1daa73a237261a8e44b7de7364a053b Mon Sep 17 00:00:00 2001 From: Srishty Date: Fri, 26 Sep 2025 15:49:32 +0000 Subject: [PATCH 09/11] fix typo --- tests/devices/test_scintillator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/devices/test_scintillator.py b/tests/devices/test_scintillator.py index 658d28911fe..f8fd9312814 100644 --- a/tests/devices/test_scintillator.py +++ b/tests/devices/test_scintillator.py @@ -84,7 +84,7 @@ async def test_given_aperture_scatterguard_parked_when_set_to_out_position_then_ await scintillator.selected_pos.set(InOut.OUT) await assert_value(scintillator.y_mm.user_setpoint, -0.02) - await assert_value(scintillator.z_mm.user_setpoint, -0.1) + await assert_value(scintillator.z_mm.user_setpoint, 0.1) async def test_given_aperture_scatterguard_parked_when_set_to_in_position_then_returns_expected( From 59479a16c0ec4dbc23cd048e71abb0b06190cdba Mon Sep 17 00:00:00 2001 From: Srishty Date: Tue, 30 Sep 2025 13:31:13 +0000 Subject: [PATCH 10/11] make all functions hidden --- src/dodal/devices/scintillator.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/dodal/devices/scintillator.py b/src/dodal/devices/scintillator.py index 561bb2d6ace..6e9bb98a409 100644 --- a/src/dodal/devices/scintillator.py +++ b/src/dodal/devices/scintillator.py @@ -55,7 +55,7 @@ def __init__( super().__init__(name) - def check_position( + def _check_position( self, current_pos: tuple[float, float], pos_to_check: tuple[float, float] ): return all( @@ -70,16 +70,16 @@ def check_position( def _get_selected_position(self, y: float, z: float) -> InOut: current_pos = [y, z] - if self.check_position(current_pos, self._scintillator_out_yz_mm): + if self._check_position(current_pos, self._scintillator_out_yz_mm): return InOut.OUT - elif self.check_position(current_pos, self._scintillator_in_yz_mm): + elif self._check_position(current_pos, self._scintillator_in_yz_mm): return InOut.IN else: return InOut.UNKNOWN - def check_aperture_parked(self): + def _check_aperture_parked(self): if ( self._aperture_scatterguard().selected_aperture.get_value() != ApertureValue.PARKED @@ -89,7 +89,7 @@ def check_aperture_parked(self): ) async def _set_selected_position(self, position: InOut) -> None: - self.check_aperture_parked() + self._check_aperture_parked() match position: case InOut.OUT: await self.y_mm.set(self._scintillator_out_yz_mm[0]) From 9aea822dc7fbb8433807352e5d2ea990d91c615c Mon Sep 17 00:00:00 2001 From: Srishty Date: Tue, 30 Sep 2025 13:43:40 +0000 Subject: [PATCH 11/11] Change current_pos to a tuple because it was causing lint failing in github CI --- src/dodal/devices/scintillator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dodal/devices/scintillator.py b/src/dodal/devices/scintillator.py index 6e9bb98a409..903c1100010 100644 --- a/src/dodal/devices/scintillator.py +++ b/src/dodal/devices/scintillator.py @@ -69,7 +69,7 @@ def _check_position( ) def _get_selected_position(self, y: float, z: float) -> InOut: - current_pos = [y, z] + current_pos = (y, z) if self._check_position(current_pos, self._scintillator_out_yz_mm): return InOut.OUT