370 implement rotation nexus by d-perl · Pull Request #736 · DiamondLightSource/hyperion

d-perl · 2023-06-14T11:48:27Z

The history got too messy on the other branch so this is a new one
Fixes #370

Implements writing of nexus file for rotation scans
simplifies the nexus_writer further as in comments from 695 nexus writer get documents from plan #696

Requires Dodal PR: https://github.com/DiamondLightSource/dodal/tree/artemis_370_implement_rotation_nexus
Requires Nexgen PR DiamondLightSource/nexgen#120
Requires GDA changes: current_energy parameter renamed to current_energy_ev

To test:
Run tests

codecov · 2023-06-14T12:49:30Z

Codecov Report

Merging #736 (554e603) into main (042c950) will increase coverage by 0.23%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##             main     DiamondLightSource/hyperion#736      +/-   ##
==========================================
+ Coverage   93.94%   94.18%   +0.23%     
==========================================
  Files          36       36              
  Lines        1718     1736      +18     
==========================================
+ Hits         1614     1635      +21     
+ Misses        104      101       -3

Impacted Files	Coverage Δ
src/artemis/parameters/internal_parameters.py	`98.68% <ø> (ø)`
...plan_specific/grid_scan_with_edge_detect_params.py	`95.12% <50.00%> (-4.88%)`	⬇️
...is/parameters/plan_specific/fgs_internal_params.py	`98.33% <96.96%> (-1.67%)`	⬇️
src/artemis/experiment_plans/rotation_scan_plan.py	`100.00% <100.00%> (ø)`
...ternal_interaction/callbacks/fgs/nexus_callback.py	`100.00% <100.00%> (ø)`
...l_interaction/callbacks/rotation/nexus_callback.py	`100.00% <100.00%> (ø)`
...callbacks/rotation/rotation_callback_collection.py	`100.00% <100.00%> (ø)`
.../artemis/external_interaction/nexus/nexus_utils.py	`100.00% <100.00%> (+8.69%)`	⬆️
.../artemis/external_interaction/nexus/write_nexus.py	`100.00% <100.00%> (+3.19%)`	⬆️
...ers/plan_specific/rotation_scan_internal_params.py	`100.00% <100.00%> (+1.88%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

DominicOram

Great, mostly the refactor we discussed and some minor comments. Also the __init__ file under the external interactions unit tests is spelt wrong

DominicOram · 2023-06-15T14:58:45Z

@@ -0,0 +1,19 @@
+{
+	"folders": [


Great addition, thanks!

DominicOram · 2023-06-15T15:05:16Z


-    @subs_decorator(list(subscriptions))
+    @bpp.subs_decorator(list(subscriptions))
+    @bpp.set_run_key_decorator("run_gridscan_move_and_tidy")


Should: This isn't the gridscan

Do we even need the run key here?

yes, setting the run key is what makes a section generate a start doc

DominicOram · 2023-06-15T15:32:54Z

            with h5py.File(temp_filename, "r+") as nxsfile:
                nxsfile["entry"].create_dataset(
-                    "end_time", data=np.string_(get_current_time())
+                    "end_time", data=np.string_(get_current_time() + "Z")


Should: get_current_time looks like it already ends with a Z, do we need two? If we do this should be put in get_current_time, not here

DominicOram · 2023-06-15T15:35:08Z

-        Goniometer: A Goniometer description for nexgen
-    """
-    # Axis: name, depends, type, vector, start
+def create_goniometer_axes(


Could: I think a docstring would still be helpful

DominicOram · 2023-06-15T15:59:06Z

 class FGSNexusWriter(NexusWriter):
-    grid_scan: dict
-
    def __init__(self, parameters: FGSInternalParameters, grid_scan: dict) -> None:
+        self.scan_points = grid_scan
        super().__init__(parameters)
-        self.goniometer = create_gridscan_goniometer_axes(
-            parameters.artemis_params.detector_params,
-            parameters.experiment_params,
-            grid_scan,
-        )
-        self.grid_scan = grid_scan

-    def _get_data_shape_for_vds(self) -> tuple[int | float, ...]:
-        ax = list(self.grid_scan.keys())[0]
-        num_frames_in_vds = len(self.grid_scan[ax])
+    def _get_data_shape_for_vds(self) -> tuple[int, ...]:
+        ax = list(self.scan_points.keys())[0]
+        num_frames_in_vds = len(self.scan_points[ax])
        nexus_detector_params: EigerDetector = self.detector.detector_params
        return (num_frames_in_vds, *nexus_detector_params.image_size)


 class RotationNexusWriter(NexusWriter):
-    def __init__(
-        self, parameters: RotationInternalParameters, rotation_scan: dict
-    ) -> None:
-        super().__init__(parameters)
-        self.goniometer = create_rotation_goniometer_axes(
-            parameters.artemis_params.detector_params,
-            parameters.experiment_params,
-            rotation_scan,
+    def __init__(self, parameters: RotationInternalParameters) -> None:
+        scan_spec = Line(
+            axis="omega",
+            start=parameters.experiment_params.omega_start,
+            stop=(
+                parameters.experiment_params.rotation_angle
+                + parameters.experiment_params.omega_start
+            ),
+            num=parameters.experiment_params.get_num_images(),
        )
+        scan_path = ScanPath(scan_spec.calculate())
+        self.scan_points = scan_path.consume().midpoints
+        super().__init__(parameters)

-    def _get_data_shape_for_vds(self) -> tuple[int | float, ...]:
-        return (1,)
-        # ax = list(self.grid_scan.keys())[0]
-        # num_frames_in_vds = len(self.grid_scan[ax])
-        # return (num_frames_in_vds, *self.detector.detector_params.image_size)
+    def _get_data_shape_for_vds(self) -> tuple[int, ...]:
+        nexus_detector_params: EigerDetector = self.detector.detector_params
+        return (self.full_num_of_images, *nexus_detector_params.image_size)


Should: As discussed, if the scan is held in the InternalParameters, which it probably should be anyway then the two different __init__s are redundant. Then, if we get the data shape from the scan itself the separate _get_data_shape functions are redundant, making these classes redundant

DominicOram · 2023-06-15T16:00:04Z

+    return plan()
+
+
+def test_rotation_scan_nexus_output_compared_to_existing_file(


Nice test, thanks!

DominicOram · 2023-06-15T16:02:31Z

+)
+
+
+def test_generic_create_gonio_creates_same_gonio_as_defined_with_individual_params(


Should: This test is great to confirm what we've done in this issue but seems a bit pointless going forward given the old way will no longer exist, suggest we remove it.

DominicOram · 2023-06-15T16:07:17Z

    omega_start: float = 0.0
    phi_start: float = 0.0
-    chi_start: Optional[float] = None
-    kappa_start: Optional[float] = None
+    chi_start: float | None = None
+    kappa_start: float | None = None


Could: Probably a new ticket but sort of feels like all of these are optional (apart from maybe the one you are rotating) and if not provided you just use the current value

Agree in principle but that's an issue with the rotation scan plan, not nexus writing - that did make me realise though that we're not even using these at all currently

issue for this: #743

DominicOram · 2023-06-15T16:23:21Z

+        if isinstance(rotation_direction, str):
+            return RotationDirection[rotation_direction]
+        else:
+            return RotationDirection(rotation_direction)


Nit: I would say we shouldn't allow the integer representation, only the string as it makes the API messier but not that fussed

DominicOram

Some comments in code. As discussed, I think this is cleaner than before but not 100%, I've made a few tickets to clean up more such as https://github.com/DiamondLightSource/python-artemis/issues/747

DominicOram · 2023-06-16T14:43:27Z

+    def get_scan_points(cls):
+        """Get points of the scan as calculated by scanspec."""
+        ...
+
+    @abstractmethod
+    def get_data_shape(cls):


Should: Can we have type hints on the return values of these?

DominicOram · 2023-06-16T14:50:43Z

        ...
+
+    @abstractmethod
+    def get_scan_points(cls):


Should: I think it will be slightly cleaner if we return the ScanPath here, then we can do the consume().midpoints in one place

currently we're using it to calculate the data shape, so we would still need to do it in multiple places, but less consistently. I think we should change this in #745 instead

DominicOram · 2023-06-16T14:55:08Z

+        ...
+
+    @abstractmethod
+    def get_data_shape(cls):


As discussed I don't think we need the data_shape if we have the scan, spun into https://github.com/DiamondLightSource/python-artemis/issues/745

DominicOram · 2023-06-16T15:04:33Z

+        if scan_number == 1:
+            x_axis = self.experiment_params.x_axis
+            y_axis = self.experiment_params.y_axis
+            spec = Line(
+                "sam_y",
+                y_axis.start,
+                y_axis.end,
+                y_axis.full_steps,
+            ) * ~Line(
+                "sam_x",
+                x_axis.start,
+                x_axis.end,
+                x_axis.full_steps,
+            )
+            scan_path = ScanPath(spec.calculate())
+            return scan_path.consume().midpoints
+        elif scan_number == 2:
+            x_axis = self.experiment_params.x_axis
+            z_axis = self.experiment_params.z_axis
+            spec = Line(
+                "sam_z",
+                z_axis.start,
+                z_axis.end,
+                z_axis.full_steps,
+            ) * ~Line(
+                "sam_x",
+                x_axis.start,
+                x_axis.end,
+                x_axis.full_steps,
+            )
+            scan_path = ScanPath(spec.calculate())
+            return scan_path.consume().midpoints


Nit: I think you can clean this up a bit using something like:

```suggestion def create_line(axis_name, axis_data): return Line( axis_name, axis_data.start, axis_data.end, axis_data.full_steps, ) x_line = create_line("sam_x", self.experiment_params.x_axis) y_line = create_line("sam_y", self.experiment_params.y_axis) z_line = create_line("sam_z", self.experiment_params.z_axis) if scan_number == 1: spec = y_line *~ x_line scan_path = ScanPath(spec.calculate()) return scan_path.consume().midpoints elif scan_number == 2: spec = z_line *~ x_line scan_path = ScanPath(spec.calculate()) return scan_path.consume().midpoints

I did something similar, but I wasn't keen on calculating unused lines so I still kept those separate

DominicOram · 2023-06-16T15:07:50Z

+        num_frames_in_vds = len(scan_points[ax])
+        return (num_frames_in_vds, size.width, size.height)
+
+    def get_omega_start(self, scan_number: int) -> float:


I think we can improve this, spun into https://github.com/DiamondLightSource/python-artemis/issues/746

Co-authored-by: Dominic Oram <dominic.oram@diamond.ac.uk>

DominicOram

Great, thank you!

d-perl added 21 commits June 13, 2023 09:01

flesh out callback, add to plan, add test

c8f653b

add file writing test

d92d0d8

add stuff to test generated nexus file

e4130cf

improve comparison test

6a21ee1

add more comparisons to test

6ab13cc

add dependency chain to test

0cfd867

implement vds function

86b0067

add data shape to test

d8864e6

fix uid stash in callback & test timestamp

5e3f713

add a couple of system tests

d8e4b10

improve comment

5b3b13b

re-add dodal req

0085917

update commit on dodal

9979676

old gonio creation -> test; make generic util

5d783b6

add test for new gonio function

ec58f4e

add test for rotation gonio

fa6c5b6

fix grid scan in new function

ad79f81

make gonio creation generic

729ee13

Merge branch 'main' into 370_implement_rotation_nexus_again

bed35e7

fix tests from merge

238ac8d

update dodal req

c9f7912

d-perl mentioned this pull request Jun 14, 2023

370 implement writing nexus files for rotation scans #697

Closed

add negen commit to requirements

a60ee05

d-perl added 6 commits June 14, 2023 14:28

Merge branch 'main' into 370_implement_rotation_nexus_again

ae174e5

update name in params from merge

729a663

fix zebra disarm plan

c14a358

fix launch.json

94f3b5e

fix zebra test

ba62b8b

bump parameter version

018eb05

DominicOram suggested changes Jun 15, 2023

View reviewed changes

d-perl added 6 commits June 16, 2023 10:39

get rit of nexus writer subclasses

35bc34b

fix some things

a5a44a3

fix some tests

3ba651c

add start index to initialisation

a637176

clean up test from debugging

680693f

fix typo

38e0425

d-perl requested a review from DominicOram June 16, 2023 13:08

This was referenced Sep 2, 2024

Use the scanspec to calculate the data shape DiamondLightSource/mx-bluesky#362

Open

Always use scanspec omega in nexus writing DiamondLightSource/mx-bluesky#361

Open

Refactor FGS Internal Params DiamondLightSource/mx-bluesky#360

Open

DominicOram suggested changes Jun 16, 2023

View reviewed changes

Update src/artemis/parameters/internal_parameters.py

12f3f1b

Co-authored-by: Dominic Oram <dominic.oram@diamond.ac.uk>

DominicOram mentioned this pull request Jun 16, 2023

Refactor nexus writer to be a standard API #725

Closed

d-perl and others added 4 commits June 20, 2023 09:45

Merge branch 'main' into 370_implement_rotation_nexus_again

db32d6e

tidy nexus param generation a little

cdec97b

fix use of data shape function and tests

7106cb4

Merge branch 'main' into 370_implement_rotation_nexus_again

d785da0

d-perl requested a review from DominicOram June 20, 2023 09:23

fix linting

554e603

DominicOram approved these changes Jun 26, 2023

View reviewed changes

DominicOram merged commit 45279b2 into main Jun 26, 2023

DominicOram deleted the 370_implement_rotation_nexus_again branch June 26, 2023 12:01

		return plan()


		def test_rotation_scan_nexus_output_compared_to_existing_file(

		)


		def test_generic_create_gonio_creates_same_gonio_as_defined_with_individual_params(

Uh oh!

Conversation

d-perl commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-perl Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

d-perl commented Jun 14, 2023 •

edited

Loading

codecov Bot commented Jun 14, 2023 •

edited

Loading

d-perl Jun 16, 2023 •

edited

Loading