(#1217)

* Remove upsert_dc_grid calls from hardware read ispyb updates
* Add units to GridScanInfo fields
* Move grid comment updates from hardware read to oav snapshot event

src/hyperion/experiment_plans/grid_detect_then_xray_centre_plan.py

@@ -199,6 +199,7 @@ def run_grid_detection_plan(
     )
 
     # Hack because the callback returns the list in inverted order
+    # TODO 1217 REMOVE THIS
     parameters.hyperion_params.ispyb_params.xtal_snapshots_omega_start = (
         oav_callback.snapshot_filenames[0][::-1]
     )

src/hyperion/experiment_plans/wait_for_robot_load_then_centre_plan.py

@@ -121,6 +121,8 @@ def wait_for_robot_load_then_centre_plan(
 
     yield from wait_for_smargon_not_disabled(composite.smargon)
 
+    # XXX 1278 this effectively casts between unrelated types which doesn't have all
+    # attributes needed for downstream e.g. grid_width_microns
     params_json = json.loads(parameters.json())
     pin_centre_params = PinCentreThenXrayCentreInternalParameters(**params_json)
 

src/hyperion/external_interaction/callbacks/common/ispyb_mapping.py

@@ -121,8 +121,8 @@ def get_xtal_snapshots(ispyb_params):
 
 @dataclass
 class GridScanInfo:
-    upper_left: Union[list[int], ndarray]  # TODO REMOVE THIS
-    # upper_left_x: float
-    # upper_left_y: float
+    upper_left_px: Union[list[int], ndarray]
+    x_steps: int
     y_steps: int
-    y_step_size: float
+    x_step_size_mm: float
+    y_step_size_mm: float

src/hyperion/external_interaction/callbacks/ispyb_callback_base.py

@@ -4,16 +4,24 @@
 from collections.abc import Sequence
 from typing import TYPE_CHECKING, Any, Callable, Dict, Optional, TypeVar
 
+import numpy
+
 from hyperion.external_interaction.callbacks.common.ispyb_mapping import (
+    GridScanInfo,
     populate_data_collection_group,
 )
 from hyperion.external_interaction.callbacks.plan_reactive_callback import (
     PlanReactiveCallback,
 )
+from hyperion.external_interaction.callbacks.xray_centre.ispyb_mapping import (
+    construct_comment_for_gridscan,
+)
 from hyperion.external_interaction.ispyb.data_model import (
+    DataCollectionGridInfo,
     DataCollectionInfo,
     ScanDataInfo,
 )
+from hyperion.external_interaction.ispyb.ispyb_dataclass import Orientation
 from hyperion.external_interaction.ispyb.ispyb_store import (
     IspybIds,
     StoreInIspyb,
@@ -46,7 +54,8 @@ def __init__(
         for self.ispyb_ids."""
         ISPYB_LOGGER.debug("Initialising ISPyB callback")
         super().__init__(log=ISPYB_LOGGER, emit=emit)
-        self._event_driven_data_collection_info: Optional[DataCollectionInfo] = None
+        self._oav_snapshot_event_idx: Optional[int] = None
+        self._hwscan_data_collection_info: Optional[DataCollectionInfo] = None
         self._sample_barcode: Optional[str] = None
         self.params: GridscanInternalParameters | RotationInternalParameters | None = (
             None
@@ -68,7 +77,8 @@ def __init__(
         self.log = ISPYB_LOGGER
 
     def activity_gated_start(self, doc: RunStart):
-        self._event_driven_data_collection_info = DataCollectionInfo()
+        self._oav_snapshot_event_idx = 0
+        self._hwscan_data_collection_info = DataCollectionInfo()
         self._sample_barcode = None
         return self._tag_doc(doc)
 
@@ -99,7 +109,7 @@ def activity_gated_event(self, doc: Event) -> Event:
                 self._handle_ispyb_transmission_flux_read(doc)
 
                 scan_data_infos = self.populate_info_for_update(
-                    self._event_driven_data_collection_info, self.params
+                    self._hwscan_data_collection_info, self.params
                 )
                 ISPYB_LOGGER.info("Updating ispyb entry.")
                 self.ispyb_ids = self.update_deposition(
@@ -111,44 +121,99 @@ def activity_gated_event(self, doc: Event) -> Event:
         return self._tag_doc(doc)
 
     def _handle_ispyb_hardware_read(self, doc):
-        assert self._event_driven_data_collection_info
+        assert self._hwscan_data_collection_info
         ISPYB_LOGGER.info("ISPyB handler received event from read hardware")
-        self._event_driven_data_collection_info.undulator_gap1 = doc["data"][
+        self._hwscan_data_collection_info.undulator_gap1 = doc["data"][
             "undulator_current_gap"
         ]
-        self._event_driven_data_collection_info.synchrotron_mode = doc["data"][
+        self._hwscan_data_collection_info.synchrotron_mode = doc["data"][
             "synchrotron_machine_status_synchrotron_mode"
         ]
-        self._event_driven_data_collection_info.slitgap_horizontal = doc["data"][
+        self._hwscan_data_collection_info.slitgap_horizontal = doc["data"][
             "s4_slit_gaps_xgap"
         ]
-        self._event_driven_data_collection_info.slitgap_vertical = doc["data"][
+        self._hwscan_data_collection_info.slitgap_vertical = doc["data"][
             "s4_slit_gaps_ygap"
         ]
         self._sample_barcode = doc["data"]["robot-barcode"]
 
     def _handle_oav_snapshot_triggered(self, doc):
+        assert self.ispyb_ids.data_collection_ids, "No current data collection"
+        data = doc["data"]
+        data_collection_id = None
+        data_collection_info = DataCollectionInfo()
+        data_collection_info.n_images = (
+            data["oav_snapshot_num_boxes_x"] * data["oav_snapshot_num_boxes_y"]
+        )
+        grid_scan_info = GridScanInfo(
+            upper_left_px=numpy.array(
+                [data["oav_snapshot_top_left_x"], data["oav_snapshot_top_left_y"]]
+            ),
+            x_steps=data["oav_snapshot_num_boxes_x"],
+            y_steps=data["oav_snapshot_num_boxes_y"],
+            # convert pixels back to mm
+            x_step_size_mm=data["oav_snapshot_box_width"]
+            * self.params.hyperion_params.ispyb_params.microns_per_pixel_x
+            / 1000,
+            y_step_size_mm=data["oav_snapshot_box_width"]
+            * self.params.hyperion_params.ispyb_params.microns_per_pixel_y
+            / 1000,
+        )
+        data_collection_info.comments = construct_comment_for_gridscan(
+            self.params.hyperion_params.ispyb_params, grid_scan_info
+        )
+        if len(self.ispyb_ids.data_collection_ids) > self._oav_snapshot_event_idx:
+            data_collection_id = self.ispyb_ids.data_collection_ids[
+                self._oav_snapshot_event_idx
+            ]
+
+        # TODO 1217 populate xtal_snapshot1/2/3
+        data_collection_grid_info = DataCollectionGridInfo(
+            dx_in_mm=data["oav_snapshot_box_width"]
+            * self.params.hyperion_params.ispyb_params.microns_per_pixel_x
+            / 1000,
+            dy_in_mm=data["oav_snapshot_box_width"]
+            * self.params.hyperion_params.ispyb_params.microns_per_pixel_y
+            / 1000,
+            steps_x=data["oav_snapshot_num_boxes_x"],
+            steps_y=data["oav_snapshot_num_boxes_y"],
+            microns_per_pixel_x=self.params.hyperion_params.ispyb_params.microns_per_pixel_x,
+            microns_per_pixel_y=self.params.hyperion_params.ispyb_params.microns_per_pixel_y,
+            snapshot_offset_x_pixel=data["oav_snapshot_top_left_x"],
+            snapshot_offset_y_pixel=data["oav_snapshot_top_left_y"],
+            orientation=Orientation.HORIZONTAL,
+            snaked=True,
+        )
+        scan_data_info = ScanDataInfo(
+            data_collection_info=data_collection_info,
+            data_collection_id=data_collection_id,
+            data_collection_grid_info=data_collection_grid_info,
+        )
+        self.ispyb_ids = self.ispyb.update_deposition(
+            self.ispyb_ids, None, [scan_data_info]
+        )
+        self._oav_snapshot_event_idx += 1
         pass
 
     def _handle_ispyb_transmission_flux_read(self, doc):
-        assert self._event_driven_data_collection_info
+        assert self._hwscan_data_collection_info
         if doc["data"]["attenuator_actual_transmission"]:
             # Ispyb wants the transmission in a percentage, we use fractions
-            self._event_driven_data_collection_info.transmission = (
+            self._hwscan_data_collection_info.transmission = (
                 doc["data"]["attenuator_actual_transmission"] * 100
             )
             # TODO 1173 Remove this once nexus_utils no longer needs it
             self.params.hyperion_params.ispyb_params.transmission_fraction = doc[
                 "data"
             ]["attenuator_actual_transmission"]
-        self._event_driven_data_collection_info.flux = doc["data"]["flux_flux_reading"]
+        self._hwscan_data_collection_info.flux = doc["data"]["flux_flux_reading"]
         # TODO 1173 Remove this once nexus_utils no longer needs it
         self.params.hyperion_params.ispyb_params.flux = (
-            self._event_driven_data_collection_info.flux
+            self._hwscan_data_collection_info.flux
         )
         if doc["data"]["dcm_energy_in_kev"]:
             energy_ev = doc["data"]["dcm_energy_in_kev"] * 1000
-            self._event_driven_data_collection_info.wavelength = convert_eV_to_angstrom(
+            self._hwscan_data_collection_info.wavelength = convert_eV_to_angstrom(
                 energy_ev
             )
             # TODO 1173 Remove this once nexus_utils no longer needs wavelength_angstroms

src/hyperion/external_interaction/callbacks/oav_snapshot_callback.py

@@ -18,6 +18,7 @@ def event(self, doc):
             ]
         )
 
+        # TODO 1217 REMOVE THIS
         self.out_upper_left.append(
             [data.get("oav_snapshot_top_left_x"), data.get("oav_snapshot_top_left_y")]
         )

src/hyperion/external_interaction/callbacks/rotation/ispyb_callback.py

@@ -129,6 +129,9 @@ def activity_gated_start(self, doc: RunStart):
     def populate_info_for_update(
         self, event_sourced_data_collection_info: DataCollectionInfo, params
     ) -> Sequence[ScanDataInfo]:
+        assert (
+            self.ispyb_ids.data_collection_ids
+        ), "Expect an existing DataCollection to update"
         params = cast(RotationInternalParameters, params)
         initial_collection_info = populate_data_collection_info_for_rotation(
             params.hyperion_params.ispyb_params,
@@ -155,6 +158,7 @@ def populate_info_for_update(
                 data_collection_position_info=populate_data_collection_position_info(
                     params.hyperion_params.ispyb_params
                 ),
+                data_collection_id=self.ispyb_ids.data_collection_ids[0],
             )
         ]
 

src/hyperion/external_interaction/callbacks/xray_centre/ispyb_callback.py

@@ -11,7 +11,6 @@
 from dodal.devices.zocalo.zocalo_results import ZOCALO_READING_PLAN_NAME
 
 from hyperion.external_interaction.callbacks.common.ispyb_mapping import (
-    GridScanInfo,
     populate_data_collection_group,
     populate_data_collection_position_info,
     populate_remaining_data_collection_info,
@@ -20,8 +19,6 @@
     BaseISPyBCallback,
 )
 from hyperion.external_interaction.callbacks.xray_centre.ispyb_mapping import (
-    construct_comment_for_gridscan,
-    populate_data_collection_grid_info,
     populate_xy_data_collection_info,
     populate_xz_data_collection_info,
 )
@@ -114,26 +111,12 @@ def activity_gated_start(self, doc: RunStart):
                 self.params.hyperion_params.detector_params,
                 self.params.hyperion_params.ispyb_params,
             )
-            grid_scan_info = GridScanInfo(
-                self.params.hyperion_params.ispyb_params.upper_left,
-                self.params.experiment_params.y_steps,
-                self.params.experiment_params.y_step_size,
-            )
-
-            def constructor():
-                return construct_comment_for_gridscan(
-                    self.params,
-                    self.params.hyperion_params.ispyb_params,
-                    grid_scan_info,
-                )
 
             scan_data_info = ScanDataInfo(
                 data_collection_info=populate_remaining_data_collection_info(
-                    constructor,
+                    lambda: None,
                     None,
                     populate_xy_data_collection_info(
-                        grid_scan_info,
-                        self.params,
                         self.params.hyperion_params.ispyb_params,
                         self.params.hyperion_params.detector_params,
                     ),
@@ -190,40 +173,14 @@ def populate_info_for_update(
         self, event_sourced_data_collection_info: DataCollectionInfo, params
     ) -> Sequence[ScanDataInfo]:
         params = cast(GridscanInternalParameters, params)
-        grid_scan_info = GridScanInfo(
-            [
-                int(params.hyperion_params.ispyb_params.upper_left[0]),
-                int(params.hyperion_params.ispyb_params.upper_left[1]),
-            ],
-            params.experiment_params.y_steps,
-            params.experiment_params.y_step_size,
-        )
         xy_scan_data_info = self.populate_xy_scan_data_info(
-            params, event_sourced_data_collection_info, grid_scan_info
-        )
-        xy_scan_data_info.data_collection_grid_info = (
-            populate_data_collection_grid_info(
-                params, grid_scan_info, params.hyperion_params.ispyb_params
-            )
+            params, event_sourced_data_collection_info
         )
         scan_data_infos = [xy_scan_data_info]
 
         if self.is_3d_gridscan():
-            xz_grid_scan_info = GridScanInfo(
-                [
-                    int(params.hyperion_params.ispyb_params.upper_left[0]),
-                    int(params.hyperion_params.ispyb_params.upper_left[2]),
-                ],
-                params.experiment_params.z_steps,
-                params.experiment_params.z_step_size,
-            )
             xz_scan_data_info = self.populate_xz_scan_data_info(
-                params, event_sourced_data_collection_info, xz_grid_scan_info
-            )
-            xz_scan_data_info.data_collection_grid_info = (
-                populate_data_collection_grid_info(
-                    params, xz_grid_scan_info, params.hyperion_params.ispyb_params
-                )
+                params, event_sourced_data_collection_info
             )
             scan_data_infos.append(xz_scan_data_info)
         return scan_data_infos
@@ -232,11 +189,11 @@ def populate_xy_scan_data_info(
         self,
         params,
         event_sourced_data_collection_info: DataCollectionInfo,
-        grid_scan_info: GridScanInfo,
     ):
+        assert (
+            self.ispyb_ids.data_collection_ids
+        ), "Expect at least one valid data collection to record scan data"
         xy_data_collection_info = populate_xy_data_collection_info(
-            grid_scan_info,
-            params,
             params.hyperion_params.ispyb_params,
             params.hyperion_params.detector_params,
         )
@@ -250,13 +207,8 @@ def populate_xy_scan_data_info(
             },
         )
 
-        def comment_constructor():
-            return construct_comment_for_gridscan(
-                params, params.hyperion_params.ispyb_params, grid_scan_info
-            )
-
         xy_data_collection_info = populate_remaining_data_collection_info(
-            comment_constructor,
+            lambda: None,
             self.ispyb_ids.data_collection_group_id,
             xy_data_collection_info,
             params.hyperion_params.detector_params,
@@ -268,16 +220,15 @@ def comment_constructor():
             data_collection_position_info=populate_data_collection_position_info(
                 params.hyperion_params.ispyb_params
             ),
+            data_collection_id=self.ispyb_ids.data_collection_ids[0],
         )
 
     def populate_xz_scan_data_info(
         self,
         params,
         event_sourced_data_collection_info: DataCollectionInfo,
-        xz_grid_scan_info: GridScanInfo,
     ):
         xz_data_collection_info = populate_xz_data_collection_info(
-            xz_grid_scan_info,
             params,
             params.hyperion_params.ispyb_params,
             params.hyperion_params.detector_params,
@@ -291,23 +242,24 @@ def populate_xz_scan_data_info(
             },
         )
 
-        def xz_comment_constructor():
-            return construct_comment_for_gridscan(
-                params, params.hyperion_params.ispyb_params, xz_grid_scan_info
-            )
-
         xz_data_collection_info = populate_remaining_data_collection_info(
-            xz_comment_constructor,
+            lambda: None,
             self.ispyb_ids.data_collection_group_id,
             xz_data_collection_info,
             params.hyperion_params.detector_params,
             params.hyperion_params.ispyb_params,
         )
+        data_collection_id = (
+            self.ispyb_ids.data_collection_ids[1]
+            if len(self.ispyb_ids.data_collection_ids) > 1
+            else None
+        )
         return ScanDataInfo(
             data_collection_info=xz_data_collection_info,
             data_collection_position_info=populate_data_collection_position_info(
                 params.hyperion_params.ispyb_params
             ),
+            data_collection_id=data_collection_id,
         )
 
     def activity_gated_stop(self, doc: RunStop) -> Optional[RunStop]: