New external parameter model #1284

d-perl · 2024-03-26T12:51:15Z

Fixes #698

Introduces a new set of pydantic classes to replace the old external params. Eventually these will replace the internal params too.

Associated GDA changes at https://gerrit.diamond.ac.uk/c/gda/gda-mx/+/41445

To test:

Run tests

…right

DominicOram

Thanks, I think this is getting towards the right direction. Some comments in code. We can have a chat about the geometry thing, not sure what a better representation is off the top of my head.

src/hyperion/parameters/components.py

src/hyperion/parameters/constants.py

src/hyperion/parameters/gridscan.py

src/hyperion/parameters/rotation.py

olliesilvester

I really like this, this new parameter model is much more intuitive. I think a section on the wiki summarising how it works and what various keywords mean would be very useful for onboarding too.

Also, it would be good to have some tests for the gridscan.py properties, like scan_points and scan_indicies

src/hyperion/parameters/components.py

olliesilvester · 2024-03-27T12:58:21Z

src/hyperion/experiment_plans/experiment_registry.py

        "experiment_param_type": PinCentreThenXrayCentreParams,
        "callbacks_factory": create_gridscan_callbacks,
    },
    "wait_for_robot_load_then_centre": {
        "setup": wait_for_robot_load_then_centre_plan.create_devices,
-        "internal_param_type": WaitForRobotLoadThenCentreInternalParameters,
+        "internal_param_type": RobotLoadThenCentre,


When I started in Hyperion, I found it confusing that we sometimes call it 'centring' and sometimes 'xray-centing' - I thought they were different things

They sort of are. In this case centre means pin tip centre centre then xray centre. I think it's ok to basically assume "centre" means do the full centring. But I appreciate it's not obvious

src/hyperion/parameters/gridscan.py

DominicOram

Some more comments, sorry. I think we are getting there though...

src/hyperion/parameters/components.py

DominicOram · 2024-04-08T18:22:59Z

src/hyperion/parameters/constants.py

+class I03Constants:
+    BEAMLINE = "BL03S" if TEST_MODE else "BL03I"
+    INSERTION_PREFIX = "SR03S" if TEST_MODE else "SR03I"
+    BASE_DATA_DIR = "/tmp/dls/i03/data/" if TEST_MODE else "/dls/i03/data/"
+    DETECTOR = "EIGER2_X_16M"
+    SHUTTER_TIME_S = 0.06


This point still stands I think, we can just use the object now rather than a string with a lookup? Appreciate we probably need them for now for the conversion to old params but can we write an issue to move to just the object then?

DominicOram · 2024-04-08T18:37:03Z

src/hyperion/parameters/gridscan.py

+    @property
+    def normal_axis(self) -> XyzAxis:
+        """The axis not used in the gridscan, e.g. Z for a scan in Y and X"""
+        return ({XyzAxis.X, XyzAxis.Y, XyzAxis.Z} ^ {self.axis_1, self.axis_2}).pop()
+
+    @property
+    def axis_1_start_um(self) -> float:
+        return self.axis_1.for_axis(self.x_start_um, self.y_start_um, self.z_start_um)
+
+    @property
+    def axis_2_start_um(self) -> float:
+        return self.axis_2.for_axis(self.x_start_um, self.y_start_um, self.z_start_um)
+
+    @property
+    def normal_axis_start(self) -> float:
+        return self.normal_axis.for_axis(
+            self.x_start_um, self.y_start_um, self.z_start_um
+        )
+
+    @property
+    def axis_1_end_um(self) -> float:
+        return self.axis_1_start_um + self.axis_1_step_size_um * self.axis_1_steps
+
+    @property
+    def axis_2_end_um(self) -> float:
+        return self.axis_2_start_um + self.axis_2_step_size_um * self.axis_2_steps


Should: normal_axis isn't used, can we remove? I then also find the self.axis_2.for_axis thing a bit awkward to understand. Could we instead have the function on the XyzStarts?

class XyzStarts(BaseModel): x_start_um: float y_start_um: float z_start_um: float def _start_for_axis(self, axis: XyzAxis) -> float: match axis: case XyzAxis.X: return self.x_start_um case XyzAxis.Y: return self.y_start_um case XyzAxis.Z: return self.z_start_um

Then this becomes:

Suggested change

@property

def normal_axis(self) -> XyzAxis:

"""The axis not used in the gridscan, e.g. Z for a scan in Y and X"""

return ({XyzAxis.X, XyzAxis.Y, XyzAxis.Z} ^ {self.axis_1, self.axis_2}).pop()

@property

def axis_1_start_um(self) -> float:

return self.axis_1.for_axis(self.x_start_um, self.y_start_um, self.z_start_um)

@property

def axis_2_start_um(self) -> float:

return self.axis_2.for_axis(self.x_start_um, self.y_start_um, self.z_start_um)

@property

def normal_axis_start(self) -> float:

return self.normal_axis.for_axis(

self.x_start_um, self.y_start_um, self.z_start_um

)

@property

def axis_1_end_um(self) -> float:

return self.axis_1_start_um + self.axis_1_step_size_um * self.axis_1_steps

@property

def axis_2_end_um(self) -> float:

return self.axis_2_start_um + self.axis_2_step_size_um * self.axis_2_steps

@property

def axis_1_start_um(self) -> float:

return self._start_for_axis(self.axis_1)

@property

def axis_2_start_um(self) -> float:

return self._start_for_axis(self.axis_2)

@property

def axis_1_end_um(self) -> float:

return self.axis_1_start_um + self.axis_1_step_size_um * self.axis_1_steps

@property

def axis_2_end_um(self) -> float:

return self.axis_2_start_um + self.axis_2_step_size_um * self.axis_2_steps

You could even argue axis_1_start_um is then a bit of a redundant function and instead just use self._start_for_axis(self.axis_1) everywhere

On discussion removed 2D grid scan params entirely, we will know what it should look like when we need to add it back

DominicOram · 2024-04-08T18:40:04Z

src/hyperion/parameters/gridscan.py

+    omega_start_deg: float = Field(default=CONST.PARAM.GRIDSCAN.OMEGA_1)  # type: ignore
+    omega2_start_deg: float = Field(default=CONST.PARAM.GRIDSCAN.OMEGA_2)


Should: The start here kind of implies that omega is moving throughout the grid, which it isnt.

DominicOram · 2024-04-08T18:46:52Z

src/hyperion/parameters/gridscan.py

+        return ScanPath(self.scan_spec.calculate()).consume().midpoints
+
+
+class ThreeDGridScan(SpecifiedGridScan, SplitScan):


Should: Can we add a docstring on what a 3D gridscan is, as it's not obvious. E.g. do a 2d grid, rotate in omega then do another grid

DominicOram · 2024-04-08T18:50:04Z

src/hyperion/parameters/gridscan.py

+    def scan_1(self):
+        x_end = self.x_start_um + self.x_step_size_um * self.x_steps
+        y1_end = self.y_start_um + self.y_step_size_um * self.y_steps
+
+        scan_1_x = Line("sam_x", self.x_start_um, x_end, self.x_steps)
+        scan_1_omega = Line(
+            "omega", self.omega_start_deg, self.omega_start_deg, self.x_steps
+        )
+        scan_1_z = Line("sam_z", self.z_start_um, self.z_start_um, self.x_steps)
+        scan_1_y = Line("sam_y", self.y_start_um, y1_end, self.y_steps)
+        return scan_1_x.zip(scan_1_z).zip(scan_1_omega) * ~scan_1_y
+
+    @property
+    def scan_2(self):
+        x_end = self.x_start_um + self.x_step_size_um * self.x_steps
+        z2_end = self.z2_start_um + self.z_step_size_um * self.z_steps
+
+        scan_2_x = Line("sam_x", self.x_start_um, x_end, self.x_steps)
+        scan_2_omega = Line(
+            "omega", self.omega2_start_deg, self.omega2_start_deg, self.x_steps
+        )
+        scan_2_y = Line("sam_y", self.y2_start_um, self.y2_start_um, self.x_steps)
+        scan_2_z = Line("sam_z", self.z2_start_um, z2_end, self.z_steps)
+        return scan_2_x.zip(scan_2_y).zip(scan_2_omega) * ~scan_2_z


Should: Can we change to grid_1/grid_2 rather than scan_1/scan_2?

Should: For the fixed things, rather than e.g. Line("sam_z", self.z_start_um, self.z_start_um, self.x_steps) I think we can just do Static("sam_z", self.z_start_um)? See https://dls-controls.github.io/scanspec/main/reference/api.html#scanspec.specs.Static. After that we should probably just run it past @coretl or @callumforrester to confirm it's the simplest it can be.

DominicOram · 2024-04-08T19:07:54Z

src/hyperion/parameters/rotation.py

+
+    @property
+    def directory(self):
+        directory = str(self.visit_directory / "auto" / str(self.sample_id))


Must: This certainly isn't right. From looking round on the file system it looks like some use sample name some use something else. We may have to provide the prefix as in http://agamemnon.diamond.ac.uk/example/collect for now then discuss with SIMS/Analysis how that is actually built.

We could just redefine it for UDC and say it is always the sample_id but we should shout a bit louder before we do that

Will take in the storage_directory as a param for now

DominicOram · 2024-04-08T19:11:07Z

src/hyperion/parameters/rotation.py

+    omega_start_deg: float = Field(default=0)  # type: ignore
+    rotation_axis: RotationAxis = Field(default=RotationAxis.OMEGA)
+    shutter_opening_time_s: float = Field(default=CONST.I03.SHUTTER_TIME_S)
+    rotation_angle_deg: float


Should: rotation_angle_deg wasn't immediately obvious to me when I looked at it. Could you add a docstring to say what it is?

I renamed it to scan_width_deg

DominicOram · 2024-04-08T19:12:01Z

src/hyperion/parameters/rotation.py

+        scan_spec = Line(
+            axis="omega",
+            start=self.omega_start_deg,
+            stop=(self.rotation_angle_deg + self.omega_start_deg),


Nit: The addition being the other way round makes more sense to me but whatever

DominicOram · 2024-04-08T20:01:23Z

Additionally, can we update https://github.com/DiamondLightSource/hyperion/wiki/Parameter-model a bit?

DominicOram

Thanks! Please update the docs at https://github.com/DiamondLightSource/hyperion/wiki/Parameter-model to something close to where we want to be with this and then happy to approve.

d-perl · 2024-04-22T09:57:26Z

@DominicOram will this do for now? https://github.com/DiamondLightSource/hyperion/wiki/Parameter-model

DominicOram

Good docs, ta!

…Source/698_new_external_param_model New external parameter model

d-perl added 14 commits March 22, 2024 15:26

(#698) introduce new parameter model skeleton side by side

adc81d1

(#698) start writing some tests

d4a75c6

(#698) start writing some tests

1140f99

(#698) fix some typing

c31720e

(#698) fix some typing

f56d9e0

(#698) add ispyb extras

fdc6746

(#698) start testing difference between old and new params

a82bf40

(#698) make old and new detector and ispyb params match

ead8167

(#698) disallow extra params

5ecb0fe

(#698) add new params

d1785cf

(#698) ignore pyright errors

b41b1d4

(#698) fix pyright errors

dbd1c1e

(#698) remove all uses of @cache because it doesn't play nice with py…

ac62c6d

…right

(#698) make old and new rotation params match

9079d6d

d-perl marked this pull request as draft March 26, 2024 13:19

d-perl added 3 commits March 26, 2024 13:23

(#698) replace param types in registry

81d0972

(#698) delete schema and validation

d03254f

(#698) fix most tests

042bca1

DominicOram suggested changes Mar 26, 2024

View reviewed changes

d-perl added 9 commits March 27, 2024 09:10

(#698) implement 3d scan spec

4602a90

(#698) add omega to scanspec

a421f7b

(#698) fix some typing

04841d4

(#698) add scan indices to 3d gridscan

274bf28

(#698) fix tests

5be82c7

(#698) allow using new params in FGS plan

f12c703

(#698) fix typing

e261ded

(#698) fix typing

59916e7

(#698) add test that all plans have correct param input type

1448c88

olliesilvester reviewed Mar 27, 2024

View reviewed changes

(#698) start adding new params to more plan inputs

992dac6

d-perl added 6 commits April 3, 2024 15:30

(#698) fix stuff in response to review comments

df3d538

(#698) fix stuff in response to review comments

00dd6a0

Merge branch 'main' into 698_new_external_param_model

ba3baeb

Merge branch 'main' into 698_new_external_param_model

d8af6e1

(#698) fix errors from merge and tests

4deaf03

(#698) add test mode lut path

f71170e

d-perl marked this pull request as ready for review April 4, 2024 08:30

d-perl added 4 commits April 4, 2024 09:34

(#698) fix typo

9b01f68

(#698) add comment to test

24f32b6

(#698) add comments things which can be removed in 1277

c72fdce

(#698) add test for new vs old rotation scan params

f8435f3

DominicOram suggested changes Apr 8, 2024

View reviewed changes

d-perl mentioned this pull request Apr 11, 2024

Get rid of detector type string #1307

Closed

d-perl added 4 commits April 11, 2024 10:20

remove 2d grid scan params

8b70b08

(#698) address review comments

f87e6ea

merge main

b3e7cfe

(#698) fix up from merge

3588167

d-perl force-pushed the 698_new_external_param_model branch from 1029fb8 to 3588167 Compare April 11, 2024 10:34

d-perl added 2 commits April 12, 2024 09:50

(#698) make directories if necessary

6d8a8c1

Merge branch 'main' into 698_new_external_param_model

5b68ead

d-perl requested a review from DominicOram April 12, 2024 08:57

Merge branch 'main' into 698_new_external_param_model

0b1298c

DominicOram suggested changes Apr 22, 2024

View reviewed changes

add some comments

746f9d9

DominicOram approved these changes Apr 22, 2024

View reviewed changes

d-perl merged commit ab757d4 into main Apr 22, 2024
6 checks passed

d-perl deleted the 698_new_external_param_model branch April 22, 2024 14:52

olliesilvester pushed a commit to olliesilvester/mx-bluesky that referenced this pull request Aug 23, 2024

Merge pull request DiamondLightSource/hyperion#1284 from DiamondLight…

3872ab7

…Source/698_new_external_param_model New external parameter model

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New external parameter model #1284

New external parameter model #1284

d-perl commented Mar 26, 2024 •

edited

Loading

DominicOram left a comment

olliesilvester left a comment

olliesilvester Mar 27, 2024

DominicOram Apr 8, 2024

DominicOram left a comment

DominicOram Apr 8, 2024

DominicOram Apr 8, 2024

d-perl Apr 11, 2024

DominicOram Apr 8, 2024

DominicOram Apr 8, 2024

DominicOram Apr 8, 2024

DominicOram Apr 8, 2024

DominicOram Apr 8, 2024

DominicOram Apr 8, 2024

d-perl Apr 11, 2024

DominicOram Apr 8, 2024

d-perl Apr 11, 2024

DominicOram Apr 8, 2024

DominicOram commented Apr 8, 2024

DominicOram left a comment

d-perl commented Apr 22, 2024

DominicOram left a comment

		omega_start_deg: float = Field(default=CONST.PARAM.GRIDSCAN.OMEGA_1) # type: ignore
		omega2_start_deg: float = Field(default=CONST.PARAM.GRIDSCAN.OMEGA_2)

		return ScanPath(self.scan_spec.calculate()).consume().midpoints


		class ThreeDGridScan(SpecifiedGridScan, SplitScan):

New external parameter model #1284

New external parameter model #1284

Conversation

d-perl commented Mar 26, 2024 • edited Loading

To test:

DominicOram left a comment

Choose a reason for hiding this comment

olliesilvester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram commented Apr 8, 2024

DominicOram left a comment

Choose a reason for hiding this comment

d-perl commented Apr 22, 2024

DominicOram left a comment

Choose a reason for hiding this comment

d-perl commented Mar 26, 2024 •

edited

Loading