(#475) Xray-centre and collect on multiple centres #553

rtuck99 · 2024-10-04T15:04:38Z

Implements #475

Codecov Report

Attention: Patch coverage is 96.63462% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.58%. Comparing base (0ff840f) to head (d3da321).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #553      +/-   ##
==========================================
+ Coverage   78.21%   78.58%   +0.37%     
==========================================
  Files          92       94       +2     
  Lines        6797     6943     +146     
==========================================
+ Hits         5316     5456     +140     
- Misses       1481     1487       +6

Components	Coverage Δ
i24 SSX	`57.17% <ø> (ø)`
hyperion	`96.22% <96.63%> (-0.02%)`	⬇️
other	`100.00% <ø> (ø)`

DominicOram

Thank you! This is great, we will see how it goes on Monday. Some comments in code, think a few places where we should probably all talk it through together.

Additionally, for multipin tests we're also probably going to need something that gives us the top result in each segment e.g. for a 100 box long multipin with 5 samples in we would want the top result in 0-20, 20-40 etc. We can do this on the beamline for the tests next week and then maybe pull it into a better defined issue.

DominicOram · 2024-10-04T16:32:55Z

src/mx_bluesky/hyperion/experiment_plans/change_aperture_then_centre_plan.py

+        pass
+
+
+def change_aperture_then_centre(


Should: I think centre here is misleading as upon first reading it I thought it meant you were about to do a pin tip/xrc. Maybe change_aperture_and_move_to_crystal?

DominicOram · 2024-10-04T16:33:59Z

src/mx_bluesky/hyperion/experiment_plans/change_aperture_then_centre_plan.py

+from mx_bluesky.hyperion.tracing import TRACER
+
+
+class CentringComposite(Protocol):


Could: Feels like a composite might be overkill here, we could just have change_aperture_then_centre take a ApertureScatterguard and a Smargon

DominicOram · 2024-10-04T16:36:35Z

src/mx_bluesky/hyperion/experiment_plans/common/flyscan_result.py

+
+
+@dataclasses.dataclass
+class FlyscanResult:


Should: It's more a XRCResult than flyscan result. We could do a XRC as a step scan and produce these results.

DominicOram · 2024-10-04T16:50:01Z

src/mx_bluesky/hyperion/experiment_plans/flyscan_xray_centre_plan.py

@@ -112,26 +116,27 @@ def sample_motors(self) -> Smargon:
        return self.smargon


+class FlyscanEventHandler:


Should: For other events we use CallbackBase. I can't see why you can't here and would be good to keep consistency.

I think it's overkill for what should just be a simple event handler. What does it give us?

DominicOram · 2024-10-04T16:50:43Z

src/mx_bluesky/hyperion/experiment_plans/flyscan_xray_centre_plan.py

+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)


Should: It looks weird to me to take args/kwargs when they're never used

I think that's a leftover from when I first started off inheriting from DocumentRouter and decided it was too much faff

DominicOram · 2024-10-04T17:14:03Z

src/mx_bluesky/hyperion/experiment_plans/flyscan_xray_centre_plan.py

@@ -242,6 +262,38 @@ def run_gridscan_and_move(
        yield from bps.wait()


+def _xrc_result_to_flyscan_result(


Should: I think it is cleaner, and more idiomatic of bluesky, to move this calculation to the FlyscanEventHandler. It should be able to get everything it needs for the calculation by the events produced throughout the plan. This would reduce the complexity of the plans themselves and change _fire_flyscan_result_event to just be a bps.read(zocalo) Probably needs a discussion.

I started doing this and have run into complications with unit tests as now ThreeDGridscan needs to be injected in lots of places. Also I think I have run into additional complications due to #220 which previously wasn't an issue because the tests either didn't need to account for this bug or did it both ways which cancelled each other out.

DominicOram · 2024-10-04T17:21:01Z

src/mx_bluesky/hyperion/experiment_plans/load_centre_collect_full_plan.py

+    selection_params = params.select_centres
+    selection_func = getattr(flyscan_result, selection_params.name)  # type: ignore
+    assert callable(selection_func)


Should: I don't think letting the user provide a function name like this is very good. It's quite brittle to name changes/argument changes and I don't think we need this much flexibility right now anyway.

DominicOram · 2024-10-04T17:24:18Z

src/mx_bluesky/hyperion/experiment_plans/load_centre_collect_full_plan.py

+    for hit in hits:
+        LOGGER.info(f"Performing rotations for {hit}")
+        yield from change_aperture_then_centre(hit, composite)
+        yield from multi_rotation_scan(
+            composite, params.multi_rotation_scan, oav_params
+        )


Must: I think what we actually want to do is take all the hits and wrap them up in one call to multi_rotation_scan with different x/y/z starts. Otherwise we're going to get the detector re-arming between them all etc. For a first pass this probably means just stay at the same aperture for each of them, which is fine.

DominicOram · 2024-10-04T17:24:49Z

src/mx_bluesky/hyperion/experiment_plans/pin_centre_then_xray_centre_plan.py

@@ -105,15 +111,27 @@ def pin_tip_centre_then_xray_centre(
    oav_config_file: str = OAV_CONFIG_JSON,
 ) -> MsgGenerator:
    """Starts preparing for collection then performs the pin tip centre and xray centre"""
+    # TODO listen for flyscan results and then centre


Should: We're doing this, no need for the comment

DominicOram · 2024-10-04T17:38:15Z

src/mx_bluesky/hyperion/experiment_plans/pin_centre_then_xray_centre_plan.py

-        pin_centre_then_xray_centre_plan(composite, parameters, oav_config_file),
-        group=CONST.WAIT.GRID_READY_FOR_DC,
-    )
+    flyscan_event_handler = FlyscanEventHandler()


Should: This pattern is used quite a bit now, I think it's probably worth making a wrapper of something like:

def do_plan_and_move_to_xrc_result(plan): """Do the specified plan then move to whatever result XRC gives back""" yield from bpp.subs_wrapper(plan, flyscan_event_handler := FlyscanEventHandler()) flyscan_results = flyscan_event_handler.flyscan_results assert ( flyscan_results ), "Flyscan result event not received or no crystal found and exception not raised" yield from change_aperture_then_centre(flyscan_results[0], composite)

I didn't partly as we are ultimately removing most of the places where it's used anyway

Fix system tests, unit tests

rtuck99 requested review from DominicOram and olliesilvester October 4, 2024 15:41

DominicOram requested changes Oct 4, 2024

View reviewed changes

Base automatically changed from 470_full_plan_to_replace_execute_request to main October 7, 2024 10:11

rtuck99 and others added 14 commits October 15, 2024 16:20

(#475) Xray-centre and collect on multiple centres

e9b34df

Make pyright happy

e45b132

Make pyright even more happy

a6738d6

Pass parameters through to multi-rotation-scan for beamline testing

4e0d128

Hotfixes for multipin DNM

4b8380a

Tidy up parameter mangling for load_centre_collect

f461487

Fix unit, system tests

c18b774

Add tests for vds_start_img

1df54a2

Minor changes from PR review

b231941

Changes after PR review, refactoring ZocaloResults soft signals.

9b5ed73

Fix system tests, unit tests

Bump dodal commit hash

cd4f625

selection function no longer maps directly to function name

f5298ba

Rename FlyscanResult and associated handlers etc to XRayCentreResult...

64264a8

Bump dodal commit hash

646e46f

rtuck99 force-pushed the 475_multipin_plan branch from 928a917 to 646e46f Compare October 15, 2024 15:52

Make pyright happy

d3da321

rtuck99 marked this pull request as ready for review October 15, 2024 16:27

rtuck99 requested a review from DominicOram October 15, 2024 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(#475) Xray-centre and collect on multiple centres #553

(#475) Xray-centre and collect on multiple centres #553

rtuck99 commented Oct 4, 2024 •

edited

Loading

codecov bot commented Oct 4, 2024 •

edited

Loading

DominicOram left a comment

DominicOram Oct 4, 2024

DominicOram Oct 4, 2024

DominicOram Oct 4, 2024

DominicOram Oct 4, 2024

rtuck99 Oct 7, 2024

DominicOram Oct 4, 2024

rtuck99 Oct 7, 2024

DominicOram Oct 4, 2024

rtuck99 Oct 15, 2024

DominicOram Oct 4, 2024

DominicOram Oct 4, 2024

DominicOram Oct 4, 2024

DominicOram Oct 4, 2024

rtuck99 Oct 7, 2024

		from mx_bluesky.hyperion.tracing import TRACER


		class CentringComposite(Protocol):

		@@ -112,26 +116,27 @@ def sample_motors(self) -> Smargon:
		return self.smargon


		class FlyscanEventHandler:

		def __init__(self, args, *kwargs):
		super().__init__(args, *kwargs)

		@@ -242,6 +262,38 @@ def run_gridscan_and_move(
		yield from bps.wait()


		def _xrc_result_to_flyscan_result(

(#475) Xray-centre and collect on multiple centres #553

Are you sure you want to change the base?

(#475) Xray-centre and collect on multiple centres #553

Conversation

rtuck99 commented Oct 4, 2024 • edited Loading

codecov bot commented Oct 4, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

rtuck99 commented Oct 4, 2024 •

edited

Loading

codecov bot commented Oct 4, 2024 •

edited

Loading