470 Full experiment plan to replace execute request #538

rtuck99 · 2024-09-30T10:41:27Z

This fix addresses #470

It also requires GDA jython script change (gerrit change request https://gerrit.diamond.ac.uk/c/gda/gda-mx/+/42628)

To test:

Confirm unit tests pass, and additional system test test_load_centre_collect_full_plan work
This code change should support single and multiple rotations, and will be enabled if the property
gda.mx.hyperion.load_centre_collect.enabled is set to true in domain.properties

DominicOram

This is great, thank you! I think the only major issue is the WarningException thing otherwise just cleaning up. I think the failing test and some of the linting can be fixed by https://github.com/DiamondLightSource/mx-bluesky/pull/506/files. See also https://gerrit.diamond.ac.uk/c/gda/gda-mx/+/42628/comments/8c916387_22898ce5

DominicOram · 2024-09-30T15:58:59Z

src/mx_bluesky/hyperion/experiment_plans/flyscan_xray_centre_plan.py

@@ -77,6 +77,12 @@ class SmargonSpeedException(Exception):
    pass


+class CrystalNotFoundException(Exception):


Must: This should inherit from WarningException otherwise GDA will pick the exception up and stop UDC for it.

DominicOram · 2024-09-30T16:03:59Z

src/mx_bluesky/hyperion/experiment_plans/flyscan_xray_centre_plan.py

+            yield from bps.mv(
+                fgs_composite.sample_motors.stub_offsets, StubPosition.CURRENT_AS_CENTER
+            )
+    finally:


Should: I think in general it's preferable to use https://nsls-ii.github.io/bluesky/generated/bluesky.preprocessors.finalize_wrapper.html as it better handles the GeneratorExit case for when the RE wants to interrupt a plan to pause/stop. We don't do this currently but might as well think bear it in mind.

I will look at doing this for the multipin changes as I will need to move that anyway

DominicOram · 2024-09-30T16:07:01Z

src/mx_bluesky/hyperion/experiment_plans/load_centre_collect_full_plan.py

+    if not oav_params:
+        oav_params = OAVParameters(context="xrayCentring")


I think we need to refactor the OAVParams out as I don't really see why we need them for the rotation now. It's a different issue but leaving this as a note to myself.

DominicOram · 2024-09-30T16:11:41Z

tests/conftest.py

    set_mock_value(smargon.z.user_readback, 0.0)
    set_mock_value(smargon.x.high_limit_travel, 2)
    set_mock_value(smargon.x.low_limit_travel, -2)
+    set_mock_value(smargon.y.high_limit_travel, 2)
+    set_mock_value(smargon.y.low_limit_travel, -2)
+    set_mock_value(smargon.z.high_limit_travel, 2)
+    set_mock_value(smargon.z.low_limit_travel, -2)


Could: I feel like we should either put this in a loop:

for motor in [smargon.x, smargon.y, smargon.z]: set_mock_value(motor.user_readback, 0.0) set_mock_value(motor.low_limit_travel, -2) set_mock_value(motor.high_limit_travel, 2)

or add it into patch_async_motor

DominicOram · 2024-09-30T16:18:39Z

src/mx_bluesky/hyperion/parameters/components.py

+    zocalo_environment: str = Field(default=CONST.ZOCALO_ENV)
+    beamline: str = Field(default=CONST.I03.BEAMLINE, pattern=r"BL\d{2}[BIJS]")
+    det_dist_to_beam_converter_path: str = Field(
+        default=CONST.PARAM.DETECTOR.BEAM_XY_LUT_PATH
+    )
+    insertion_prefix: str = Field(
+        default=CONST.I03.INSERTION_PREFIX, pattern=r"SR\d{2}[BIJS]"
+    )
+    detector_distance_mm: float | None = Field(default=None, gt=0)


Another reminder to myself to write an issue cleaning this up

DominicOram · 2024-09-30T16:50:33Z

tests/unit_tests/hyperion/experiment_plans/test_flyscan_xray_centre_plan.py

-        class MoveException(Exception):
-            pass
-
        feature_controlled = _get_feature_controlled(
            fgs_composite_with_panda_pcap,
            test_fgs_params_panda_zebra,
        )
        mock_zocalo_trigger(fgs_composite_with_panda_pcap.zocalo, [])
-        move_xyz.side_effect = MoveException()

-        with pytest.raises(MoveException):
+        with pytest.raises(CrystalNotFoundException):


Could: I'm not a big fan of this change because we're relying on the fact the code is producing an error as is. Previously we were deliberately injecting an error in because we wanted to test with it, which seems more explicit but not that big a deal

DominicOram · 2024-09-30T16:59:12Z

tests/unit_tests/hyperion/experiment_plans/test_load_centre_collect_full_plan.py

+def find_a_pin(pin_tip_detection):
+    def set_good_position():
+        set_mock_value(pin_tip_detection.triggered_tip, (100, 110))
+        return NullStatus()
+
+    return set_good_position


Could: I think you can clean this up a bit using AsyncStatus and partials:

from functools import partial from ophyd_async.core import AsyncStatus @AsyncStatus.wrap async def find_a_pin(pin_tip_detection): set_mock_value(pin_tip_detection.triggered_tip, (100, 110)) ... composite.pin_tip_detection.trigger = MagicMock( side_effect=partial(find_a_pin, composite.pin_tip_detection) )

I think the use of AsyncStatus is a should as we're trying to move away from ophyd

dont_find_pin isn't used anyway as in the end I rewrote the test using the run engine simulator so I will remove it

I still think the use of partial and AsyncStatus is cleaner than the nested functions and NullStatus but not worth holding things up on

DominicOram · 2024-09-30T17:04:37Z

tests/unit_tests/hyperion/experiment_plans/test_load_centre_collect_full_plan.py

+    return set_good_position
+
+
+def dont_find_pin(pin_tip_detection):


Should: I don't think this is used? Can we remove it?

DominicOram · 2024-09-30T17:08:33Z

tests/system_tests/hyperion/external_interaction/test_load_centre_collect_full_plan.py

+def compare_actual_and_expected(
+    id, expected_values, fetch_datacollection_attribute, column_map: dict | None = None
+):
+    results = "\n"
+    for k, v in expected_values.items():
+        actual = fetch_datacollection_attribute(
+            id, column_map[k.lower()] if column_map else k
+        )
+        if isinstance(actual, Decimal):
+            actual = float(actual)
+        if isinstance(v, float):
+            actual_v = actual == pytest.approx(v)
+        else:
+            actual_v = actual == v
+        if not actual_v:
+            results += f"expected {k} {v} == {actual}\n"
+    assert results == "\n", results
+
+
+def compare_comment(
+    fetch_datacollection_attribute, data_collection_id, expected_comment
+):
+    actual_comment = fetch_datacollection_attribute(
+        data_collection_id, DATA_COLLECTION_COLUMN_MAP["comments"]
+    )
+    match = re.search(" Zocalo processing took", actual_comment)
+    truncated_comment = actual_comment[: match.start()] if match else actual_comment
+    assert truncated_comment == expected_comment


Should: Are these exactly the same as those in test_ispyb_dev_connection.py? Can we move them to somewhere common?

Yes, I have moved them

DominicOram · 2024-09-30T17:10:27Z

tests/system_tests/hyperion/external_interaction/test_load_centre_collect_full_plan.py

+
+
+@pytest.mark.s03
+def test_execute_load_centre_collect_full_plan(


Must: Running this I get an error:

FAILED tests/system_tests/hyperion/external_interaction/test_load_centre_collect_full_plan.py::test_execute_load_centre_collect_full_plan - AssertionError: expected beamSizeAtSampleX 0.1 == 0.02 assert '\nexpected beamSizeAtSampleX 0.1 == 0.02\n' == '\n' Strings contain only whitespace, escaping them using repr() - '\n' + '\nexpected beamSizeAtSampleX 0.1 == 0.02\n'

Otherwise, love this test though, thank you!

codecov · 2024-10-02T13:57:06Z

Codecov Report

Attention: Patch coverage is 95.89041% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.12%. Comparing base (7b9efa7) to head (a4c44ca).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #538      +/-   ##
==========================================
+ Coverage   78.01%   78.12%   +0.11%     
==========================================
  Files          91       93       +2     
  Lines        6754     6799      +45     
==========================================
+ Hits         5269     5312      +43     
- Misses       1485     1487       +2

Components	Coverage Δ
i24 SSX	`57.12% <ø> (ø)`
hyperion	`96.23% <95.89%> (-0.01%)`	⬇️
other	`94.79% <ø> (ø)`

DominicOram

Great, thank you!

Base automatically changed from 490_split_robot_load to main September 30, 2024 17:15

DominicOram requested changes Sep 30, 2024

View reviewed changes

DominicOram mentioned this pull request Sep 30, 2024

Remove initial_xyz #541

Open

rtuck99 added 4 commits October 2, 2024 11:36

Implementation of grand unified experiment plan

334114c

Add load_centre_collect to the experiment registry

ce1faa5

Additional system test for full experiment plan

273bf43

Make ruff happpy

35c78b6

rtuck99 force-pushed the 470_full_plan_to_replace_execute_request branch from 971ad3e to 35c78b6 Compare October 2, 2024 13:51

Changes following PR review comments

bd24559

Make pyright happy

a4c44ca

rtuck99 requested a review from DominicOram October 3, 2024 12:08

DominicOram approved these changes Oct 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

470 Full experiment plan to replace execute request #538

470 Full experiment plan to replace execute request #538

rtuck99 commented Sep 30, 2024 •

edited

Loading

DominicOram left a comment •

edited

Loading

DominicOram Sep 30, 2024

DominicOram Sep 30, 2024

rtuck99 Oct 2, 2024

DominicOram Sep 30, 2024

DominicOram Sep 30, 2024

DominicOram Sep 30, 2024

DominicOram Sep 30, 2024

DominicOram Sep 30, 2024

DominicOram Sep 30, 2024

DominicOram Sep 30, 2024

rtuck99 Oct 2, 2024

DominicOram Oct 4, 2024

DominicOram Sep 30, 2024

DominicOram Sep 30, 2024

rtuck99 Oct 2, 2024

DominicOram Sep 30, 2024

DominicOram Sep 30, 2024

codecov bot commented Oct 2, 2024 •

edited

Loading

DominicOram left a comment

		@@ -77,6 +77,12 @@ class SmargonSpeedException(Exception):
		pass


		class CrystalNotFoundException(Exception):

		if not oav_params:
		oav_params = OAVParameters(context="xrayCentring")

		return set_good_position


		def dont_find_pin(pin_tip_detection):



		@pytest.mark.s03
		def test_execute_load_centre_collect_full_plan(

470 Full experiment plan to replace execute request #538

Are you sure you want to change the base?

470 Full experiment plan to replace execute request #538

Conversation

rtuck99 commented Sep 30, 2024 • edited Loading

DominicOram left a comment • edited Loading

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

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

codecov bot commented Oct 2, 2024 • edited Loading

Codecov Report

DominicOram left a comment

Choose a reason for hiding this comment

rtuck99 commented Sep 30, 2024 •

edited

Loading

DominicOram left a comment •

edited

Loading

codecov bot commented Oct 2, 2024 •

edited

Loading