Merge pull request #534 from DiamondLightSource/527_528_improve_and_t…

…est_aperture

527 528 improve and test aperture

src/artemis/devices/aperturescatterguard.py

@@ -9,9 +9,13 @@
 from artemis.devices.scatterguard import Scatterguard
 
 
+class InvalidApertureMove(Exception):
+    pass
+
+
 @dataclass
 class AperturePositions:
-    """Holds the tuple (miniap_x, miniap_y, miniap_z, scatterguard_x, scatterguard_y)
+    """Holds tuples (miniap_x, miniap_y, miniap_z, scatterguard_x, scatterguard_y)
     representing the motor positions needed to select a particular aperture size.
     """
 
@@ -53,6 +57,14 @@ def from_gda_beamline_params(cls, params):
             ),
         )
 
+    def position_valid(self, pos: tuple[float, float, float, float, float]):
+        """
+        Check if argument 'pos' is a valid position in this AperturePositions object.
+        """
+        if pos not in [self.LARGE, self.MEDIUM, self.SMALL, self.ROBOT_LOAD]:
+            return False
+        return True
+
 
 class ApertureScatterguard(InfoLoggingDevice):
     aperture: Aperture = Cpt(Aperture, "-MO-MAPT-01:")
@@ -62,26 +74,34 @@ class ApertureScatterguard(InfoLoggingDevice):
     def load_aperture_positions(self, positions: AperturePositions):
         self.aperture_positions = positions
 
-    def safe_move_within_datacollection_range(
+    def set(self, pos: tuple[float, float, float, float, float]) -> AndStatus:
+        try:
+            assert isinstance(self.aperture_positions, AperturePositions)
+            assert self.aperture_positions.position_valid(pos)
+        except AssertionError as e:
+            raise InvalidApertureMove(repr(e))
+        return self._safe_move_within_datacollection_range(*pos)
+
+    def _safe_move_within_datacollection_range(
         self,
         aperture_x: float,
         aperture_y: float,
         aperture_z: float,
         scatterguard_x: float,
         scatterguard_y: float,
-    ) -> None:
+    ) -> AndStatus:
         """
         Move the aperture and scatterguard combo safely to a new position
         """
-        assert isinstance(self.aperture_positions, AperturePositions)
-
+        # EpicsMotor does not have deadband/MRES field, so the way to check if we are
+        # in a datacollection position is to see if we are "ready" (DMOV) and the target
+        # position is correct
         ap_z_in_position = self.aperture.z.motor_done_move.get()
         if not ap_z_in_position:
             return
-
         current_ap_z = self.aperture.z.user_setpoint.get()
         if current_ap_z != aperture_z:
-            raise Exception(
+            raise InvalidApertureMove(
                 "ApertureScatterguard safe move is not yet defined for positions "
                 "outside of LARGE, MEDIUM, SMALL, ROBOT_LOAD."
             )
@@ -92,9 +112,13 @@ def safe_move_within_datacollection_range(
                 scatterguard_x
             ) & self.scatterguard.y.set(scatterguard_y)
             sg_status.wait()
-            self.aperture.x.set(aperture_x)
-            self.aperture.y.set(aperture_y)
-            self.aperture.z.set(aperture_z)
+            final_status = (
+                sg_status
+                & self.aperture.x.set(aperture_x)
+                & self.aperture.y.set(aperture_y)
+                & self.aperture.z.set(aperture_z)
+            )
+            return final_status
 
         else:
             ap_status: AndStatus = (
@@ -103,5 +127,9 @@ def safe_move_within_datacollection_range(
                 & self.aperture.z.set(aperture_z)
             )
             ap_status.wait()
-            self.scatterguard.x.set(scatterguard_x)
-            self.scatterguard.y.set(scatterguard_y)
+            final_status = (
+                ap_status
+                & self.scatterguard.x.set(scatterguard_x)
+                & self.scatterguard.y.set(scatterguard_y)
+            )
+            return final_status

src/artemis/devices/system_tests/test_aperturescatterguard_system.py

@@ -0,0 +1,149 @@
+import bluesky.plan_stubs as bps
+import pytest
+from bluesky.callbacks import CallbackBase
+from bluesky.run_engine import RunEngine
+
+from artemis.devices.aperturescatterguard import (
+    AperturePositions,
+    ApertureScatterguard,
+    InvalidApertureMove,
+)
+from artemis.parameters import I03_BEAMLINE_PARAMETER_PATH, GDABeamlineParameters
+
+
+@pytest.fixture
+def ap_sg():
+    ap_sg = ApertureScatterguard(prefix="BL03S", name="ap_sg")
+    ap_sg.load_aperture_positions(
+        AperturePositions.from_gda_beamline_params(
+            GDABeamlineParameters.from_file(I03_BEAMLINE_PARAMETER_PATH)
+        )
+    )
+    return ap_sg
+
+
+@pytest.fixture
+def move_to_large(ap_sg: ApertureScatterguard):
+    yield from bps.abs_set(ap_sg, ap_sg.aperture_positions.LARGE)
+
+
+@pytest.fixture
+def move_to_medium(ap_sg: ApertureScatterguard):
+    yield from bps.abs_set(ap_sg, ap_sg.aperture_positions.MEDIUM)
+
+
+@pytest.fixture
+def move_to_small(ap_sg: ApertureScatterguard):
+    yield from bps.abs_set(ap_sg, ap_sg.aperture_positions.SMALL)
+
+
+@pytest.fixture
+def move_to_robotload(ap_sg: ApertureScatterguard):
+    yield from bps.abs_set(ap_sg, ap_sg.aperture_positions.ROBOT_LOAD)
+
+
+@pytest.mark.s03
+def test_aperturescatterguard_setup(ap_sg: ApertureScatterguard):
+    ap_sg.wait_for_connection()
+    assert ap_sg.aperture_positions is not None
+
+
+@pytest.mark.s03
+def test_aperturescatterguard_move_in_plan(
+    ap_sg: ApertureScatterguard,
+    move_to_large,
+    move_to_medium,
+    move_to_small,
+    move_to_robotload,
+):
+    RE = RunEngine({})
+    ap_sg.wait_for_connection()
+
+    ap_sg.aperture.z.set(ap_sg.aperture_positions.LARGE[2], wait=True)
+
+    RE(move_to_large)
+    RE(move_to_medium)
+    RE(move_to_small)
+    RE(move_to_robotload)
+
+
+@pytest.mark.s03
+def test_move_fails_when_not_in_good_starting_pos(
+    ap_sg: ApertureScatterguard, move_to_large
+):
+    RE = RunEngine({})
+    ap_sg.wait_for_connection()
+
+    ap_sg.aperture.z.set(0, wait=True)
+
+    with pytest.raises(InvalidApertureMove):
+        RE(move_to_large)
+
+
+class MonitorCallback(CallbackBase):
+    # holds on to the most recent time a motor move completed for aperture and
+    # scatterguard y
+
+    t_ap_y: float = 0
+    t_sg_y: float = 0
+    event_docs: list[dict] = []
+
+    def event(self, doc):
+        self.event_docs.append(doc)
+        if doc["data"].get("ap_sg_aperture_y_motor_done_move") == 1:
+            self.t_ap_y = doc["timestamps"].get("ap_sg_aperture_y_motor_done_move")
+        if doc["data"].get("ap_sg_scatterguard_y_motor_done_move") == 1:
+            self.t_sg_y = doc["timestamps"].get("ap_sg_scatterguard_y_motor_done_move")
+
+
+@pytest.mark.s03
+@pytest.mark.parametrize(
+    "pos1,pos2,sg_first",
+    [
+        ("L", "M", True),
+        ("L", "S", True),
+        ("L", "R", False),
+        ("M", "L", False),
+        ("M", "S", True),
+        ("M", "R", False),
+        ("S", "L", False),
+        ("S", "M", False),
+        ("S", "R", False),
+        ("R", "L", True),
+        ("R", "M", True),
+        ("R", "S", True),
+    ],
+)
+def test_aperturescatterguard_moves_in_correct_order(
+    pos1, pos2, sg_first, ap_sg: ApertureScatterguard
+):
+    cb = MonitorCallback()
+    positions = {
+        "L": ap_sg.aperture_positions.LARGE,
+        "M": ap_sg.aperture_positions.MEDIUM,
+        "S": ap_sg.aperture_positions.SMALL,
+        "R": ap_sg.aperture_positions.ROBOT_LOAD,
+    }
+    pos1 = positions[pos1]
+    pos2 = positions[pos2]
+    RE = RunEngine({})
+    RE.subscribe(cb)
+
+    ap_sg.wait_for_connection()
+    ap_sg.aperture.y.set(0, wait=True)
+    ap_sg.scatterguard.y.set(0, wait=True)
+    ap_sg.aperture.z.set(pos1[2], wait=True)
+
+    def monitor_and_moves():
+        yield from bps.open_run()
+        yield from bps.monitor(ap_sg.aperture.y.motor_done_move, name="ap_y")
+        yield from bps.monitor(ap_sg.scatterguard.y.motor_done_move, name="sg_y")
+        yield from bps.mv(ap_sg, pos1)
+        yield from bps.sleep(0.05)
+        yield from bps.mv(ap_sg, pos2)
+        yield from bps.sleep(0.05)
+        yield from bps.close_run()
+
+    RE(monitor_and_moves())
+
+    assert (cb.t_sg_y < cb.t_ap_y) == sg_first

src/artemis/experiment_plans/fast_grid_scan_plan.py

@@ -81,7 +81,7 @@ def set_aperture_for_bbox_size(
     artemis.log.LOGGER.info(
         f"Setting aperture to {aperture_size_positions} based on bounding box size {bbox_size}."
     )
-    aperture_device.safe_move_within_datacollection_range(*aperture_size_positions)
+    yield from bps.abs_set(aperture_device, aperture_size_positions)
 
 
 def read_hardware_for_ispyb(
@@ -225,7 +225,9 @@ def gridscan_with_subscriptions(fgs_composite, detector, params):
 
     if bbox_size is not None:
         with TRACER.start_span("change_aperture"):
-            set_aperture_for_bbox_size(fgs_composite.aperture_scatterguard, bbox_size)
+            yield from set_aperture_for_bbox_size(
+                fgs_composite.aperture_scatterguard, bbox_size
+            )
 
     # once we have the results, go to the appropriate position
     artemis.log.LOGGER.info("Moving to centre of mass.")

src/artemis/external_interaction/__init__.py

@@ -7,11 +7,3 @@
 execution of the experimental plan. It's not recommended to use the interaction classes
 here directly in plans except through the use of such callbacks.
 """
-from artemis.external_interaction.ispyb.store_in_ispyb import (
-    StoreInIspyb2D,
-    StoreInIspyb3D,
-)
-from artemis.external_interaction.nexus.write_nexus import NexusWriter
-from artemis.external_interaction.zocalo.zocalo_interaction import ZocaloInteractor
-
-__all__ = ["ZocaloInteractor", "NexusWriter", "StoreInIspyb2D", "StoreInIspyb3D"]

src/artemis/parameters.py

@@ -1,3 +1,5 @@
+from __future__ import annotations
+
 import copy
 from dataclasses import dataclass, field
 from os import environ

src/artemis/unit_tests/test_fast_grid_scan_plan.py

@@ -158,7 +158,7 @@ def standalone_read_hardware_for_ispyb(und, syn, slits):
 
 
 @patch(
-    "artemis.devices.aperturescatterguard.ApertureScatterguard.safe_move_within_datacollection_range"
+    "artemis.devices.aperturescatterguard.ApertureScatterguard._safe_move_within_datacollection_range"
 )
 @patch("artemis.experiment_plans.fast_grid_scan_plan.run_gridscan")
 @patch("artemis.experiment_plans.fast_grid_scan_plan.move_xyz")
@@ -191,7 +191,7 @@ def test_results_adjusted_and_passed_to_move_xyz(
 
 
 @patch(
-    "artemis.devices.aperturescatterguard.ApertureScatterguard.safe_move_within_datacollection_range"
+    "artemis.devices.aperturescatterguard.ApertureScatterguard._safe_move_within_datacollection_range"
 )
 @patch("artemis.experiment_plans.fast_grid_scan_plan.run_gridscan")
 @patch("artemis.experiment_plans.fast_grid_scan_plan.move_xyz")
@@ -251,7 +251,7 @@ def test_results_passed_to_move_aperture(
     )
 
     move_aperture.assert_has_calls(
-        [call_large, call_medium, call_small], any_order=False
+        [call_large, call_medium, call_small], any_order=True
     )
 
 
@@ -274,7 +274,7 @@ def test_results_passed_to_move_motors(bps_mv: MagicMock, test_params: FullParam
 
 
 @patch(
-    "artemis.devices.aperturescatterguard.ApertureScatterguard.safe_move_within_datacollection_range"
+    "artemis.devices.aperturescatterguard.ApertureScatterguard._safe_move_within_datacollection_range"
 )
 @patch("artemis.experiment_plans.fast_grid_scan_plan.run_gridscan.do_fgs")
 @patch("artemis.experiment_plans.fast_grid_scan_plan.run_gridscan")
@@ -308,7 +308,7 @@ def test_individual_plans_triggered_once_and_only_once_in_composite_run(
 
 
 @patch(
-    "artemis.devices.aperturescatterguard.ApertureScatterguard.safe_move_within_datacollection_range"
+    "artemis.devices.aperturescatterguard.ApertureScatterguard._safe_move_within_datacollection_range"
 )
 @patch("artemis.experiment_plans.fast_grid_scan_plan.run_gridscan.do_fgs")
 @patch("artemis.experiment_plans.fast_grid_scan_plan.run_gridscan")