349 rotation scan now collects OAV snapshots #1443

rtuck99 · 2024-06-12T13:09:20Z

Addresses the remaining parts of DiamondLightSource/mx-bluesky#388

Link to dodal PR (if required): #N/A
(remember to update setup.cfg with the dodal commit tag if you need it for tests to pass!)

To test:

When supplied with snapshot_omegas_deg as an array of up to 4 snapshot angles, and snapshot_directory in the request parameters to a rotation scan, OAV snapshots will be acquired prior to the rotation scan at the requested angles and deposited in ispyb
If snapshot_omegas_deg is empty or missing, no snapshots will be acquired and snapshot filenames will be registered in ispyb as specified by the ispyb_extras.xtal_snapshots_omega_start array

DominicOram

Great, thank you, some comments in code.

DominicOram · 2024-06-18T13:50:33Z

src/hyperion/device_setup_plans/manipulate_sample.py

-def setup_sample_environment(
-    aperture_scatterguard: ApertureScatterguard,
-    aperture_position_gda_name: AperturePositionGDANames | None,
+def begin_sample_environment_setup(


Should: I think a docstring on here would be good. I think at the moment it would be something like "Do all the sample environment changes that can be done in parallel to oav snapshots"

DominicOram · 2024-06-18T13:50:54Z

src/hyperion/device_setup_plans/manipulate_sample.py

    """Move the aperture into required position, move out the backlight, retract the detector shutter,
    and set the attenuator to transmission."""


Should: This docstring is now wrong

DominicOram · 2024-06-18T14:18:26Z

src/hyperion/experiment_plans/oav_snapshot_plan.py

+        return
+    yield from bps.wait(group=OAV_SNAPSHOT_SETUP_GROUP)
+    yield from _setup_oav(composite, parameters, oav_parameters)
+    for omega in parameters.snapshot_omegas_deg or []:


Could: Why not default snapshot_omegas_deg to [] in WithSnapshot then it will never be None?

I did what you suggested but then was reminded why I didn't - pyright complains that it can still be None, which you still need in the type hint in order to allow the parameter to be omitted rather than left empty in the json. So I figured might as well leave the check in there.

src/hyperion/experiment_plans/oav_snapshot_plan.py

DominicOram · 2024-06-18T14:24:13Z

src/hyperion/experiment_plans/oav_snapshot_plan.py

+    yield from bps.wait(group=OAV_SNAPSHOT_SETUP_GROUP)
+    yield from _setup_oav(composite, parameters, oav_parameters)
+    for omega in parameters.snapshot_omegas_deg or []:
+        yield from _take_oav_snapshot(composite, parameters, omega)


Should: Why does this function take the params? I don't think they're used.

I assume this was on _take_oav_snapshot? I will remove it

DominicOram · 2024-06-18T14:48:37Z

src/hyperion/experiment_plans/rotation_scan_plan.py

+        yield from bps.wait("move_gonio_to_start")
+        if params.take_snapshots:
+            yield from bps.wait("move_to_rotation_start")
+            yield from oav_snapshot_plan(composite, params, oav_params)
+
+            # Move to start again for rotation scan after snapshots
+            yield from bps.abs_set(
+                axis,
+                motion_values.start_motion_deg,
+                group="move_to_rotation_start",
+                wait=True,
+            )
+
+        yield from setup_sample_environment(
+            composite.aperture_scatterguard,
+            params.selected_aperture,
+            composite.backlight,
+        )


Should: Without measuring it I'm not 100% convinced on the optimal order of operations here, except that begin_sample_environment_setup can take ages so that should be first and for the snapshots to be correct we definitely want the x/y/z to be correct and velocity to be at maximum. So as a first pass I would err on reducing the code complexity a bit by moving this earlier. Like I think we could do:

begin_sample_environment_setup(...) move_x_y_z(...) move_phi_chi_omega(...) wait(`move_gonio_to_start`) set_max_velo setup_oav_snapshot_plan(...) oav_snapshot_plan(...) setup_sample_environment(...) _rotation_scan_plan()

I think it's good to keep the setup and taking of snapshots separate func calls though as we will probably want to shift them a bit when we get more into optimising.

I made as few changes to the order as I thought necessary. Not quite sure what you want, current order is this:

# These are already first begin_sample_environment_setup(...) setup_oav_snapshot_plan(...) # this is first bc AFAIK scatterguard and backlight have no dependencies and no waits but are physical movement therefore take time move_x_y_z(...) move_phi_chi_omega(...) set_max_velo wait('move_gonio_to_start') oav_snapshot_plan(...) setup_sample_environment(...)

So apart from moving set_max_velo and setup_oav_snapshot_plan, there is no difference, apart from other calls you haven't listed? There doesn't seem to be much point in moving setup_oav_snapshot_plan to just before oav_snapshot_plan; you would lose all the benefit of having two separate calls

The only other difference is that all of these are moved out of the _rotation_scan_plan, which I didn't do in order to ensure the events were not disturbed.

Yh, it's very similar but there are a few differences, like not having to move to start_motion_deg twice, which I don't think you need to do if you put this earlier? I also think moving it out of _rotation_scan_plan is good as it reduces the complexity of the inner plan. What do you mean by ensure the events were not disturbed?

As discussed, moving snapshot stuff to outer plan makes things less complicated and should be fine so doing that. We can get away with setting max_velo twice if we have to.

DominicOram

Thanks. Sorry, a few more comments.

DominicOram · 2024-06-27T14:35:56Z

src/hyperion/experiment_plans/oav_snapshot_plan.py

+    yield from bps.abs_set(
+        composite.smargon.omega, omega, group=OAV_SNAPSHOT_SETUP_SHOT
+    )
    time_now = datetime.now()
    filename = f"{time_now.strftime('%H%M%S')}_oav_snapshot_{omega:.0f}"
-    yield from bps.abs_set(composite.oav.snapshot.filename, filename)
-    yield from bps.trigger(composite.oav.snapshot, group=OAV_SNAPSHOT_GROUP)
-    yield from bps.wait(group=OAV_SNAPSHOT_GROUP)
+    yield from bps.abs_set(
+        composite.oav.snapshot.filename, filename, group=OAV_SNAPSHOT_SETUP_SHOT
+    )
+    yield from bps.wait(group=OAV_SNAPSHOT_SETUP_SHOT)


Could: mv takes multiple args and waits for them all so you could do this a little more cleanly, if you like:

time_now = datetime.now() filename = f"{time_now.strftime('%H%M%S')}_oav_snapshot_{omega:.0f}" yield from bps.mv( composite.smargon.omega, omega composite.oav.snapshot.filename, filename )

~~they won't be included in any readings for documents in that case though~~ ignore this, obviously sets won't be included in readings anyway

src/hyperion/experiment_plans/rotation_scan_plan.py

(#349) refactor rotation_scan_plan to move more things to outside inner plan as per PR comments

…ure unit tests don't accidentally read files from /dls,/dls_sw

DominicOram

Great! Thanks. I fixed a linting error but otherwise all good.

d-perl · 2024-07-17T15:59:38Z

tests/unit_tests/conftest.py

+
+
+@pytest.fixture(autouse=True)
+def patch_open_to_prevent_dls_reads_in_tests():


@rtuck99 this is amazing

…#1443) * (DiamondLightSource/hyperion#349) integrate snapshot plan with main rotation scan plan * (DiamondLightSource/hyperion#349) additional ispyb system test and fixes/refactor of existing * (DiamondLightSource/hyperion#349) Response to PR comments (DiamondLightSource/hyperion#349) refactor rotation_scan_plan to move more things to outside inner plan as per PR comments * Fix CI failure caused by import of /dls config files.Add check to ensure unit tests don't accidentally read files from /dls,/dls_sw --------- Co-authored-by: Dominic Oram <[email protected]>

rtuck99 force-pushed the 349_create_oav_snapshot_plan branch from 486d71e to eed8d74 Compare June 12, 2024 14:43

rtuck99 force-pushed the 349_oav_snapshot_plan_part_2 branch from c70a6b9 to a9906a1 Compare June 12, 2024 14:50

rtuck99 marked this pull request as ready for review June 13, 2024 07:42

rtuck99 requested review from DominicOram and d-perl June 13, 2024 07:42

d-perl approved these changes Jun 13, 2024

View reviewed changes

Base automatically changed from 349_create_oav_snapshot_plan to main June 13, 2024 11:59

rtuck99 mentioned this pull request Jun 14, 2024

(#1327) ispyb omega_start, axis_start now read from hardware for gridscans #1445

Merged

rtuck99 marked this pull request as draft June 14, 2024 12:46

rtuck99 marked this pull request as ready for review June 14, 2024 12:50

rtuck99 force-pushed the 349_oav_snapshot_plan_part_2 branch from a9906a1 to 488e6b0 Compare June 14, 2024 13:29

DominicOram suggested changes Jun 18, 2024

View reviewed changes

rtuck99 force-pushed the 349_oav_snapshot_plan_part_2 branch from 557b073 to f4402e0 Compare June 20, 2024 09:22

rtuck99 requested review from d-perl and DominicOram June 20, 2024 10:35

rtuck99 force-pushed the 349_oav_snapshot_plan_part_2 branch from 7ccfcb3 to 326115d Compare June 27, 2024 09:35

DominicOram suggested changes Jun 27, 2024

View reviewed changes

rtuck99 requested a review from DominicOram July 3, 2024 08:41

rtuck99 force-pushed the 349_oav_snapshot_plan_part_2 branch 2 times, most recently from 69fe5e5 to a649605 Compare July 3, 2024 09:08

rtuck99 added 7 commits July 12, 2024 10:52

(#349) integrate snapshot plan with main rotation scan plan

4efe65d

(#349) additional ispyb system test and fixes/refactor of existing

f1444bb

(#349) Response to PR comments

51f90a5

(#349) refactor rotation_scan_plan to move more things to outside inner plan as per PR comments

Fix CI failure caused by import of /dls config files.Add check to ens…

5438c6b

…ure unit tests don't accidentally read files from /dls,/dls_sw

(#349) response to PR feedback

7899928

Fix missing import

281da6b

Fix broken system tests

831618e

rtuck99 force-pushed the 349_oav_snapshot_plan_part_2 branch from 8146ce0 to 831618e Compare July 12, 2024 10:44

Fix linting error

9b66756

DominicOram approved these changes Jul 17, 2024

View reviewed changes

DominicOram merged commit 323a1f1 into main Jul 17, 2024
6 checks passed

DominicOram deleted the 349_oav_snapshot_plan_part_2 branch July 17, 2024 15:30

d-perl reviewed Jul 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

349 rotation scan now collects OAV snapshots #1443

349 rotation scan now collects OAV snapshots #1443

rtuck99 commented Jun 12, 2024

DominicOram left a comment

DominicOram Jun 18, 2024

DominicOram Jun 18, 2024

DominicOram Jun 18, 2024

rtuck99 Jun 19, 2024

DominicOram Jun 18, 2024

rtuck99 Jun 19, 2024

DominicOram Jun 18, 2024

rtuck99 Jun 19, 2024 •

edited

Loading

DominicOram Jun 19, 2024

DominicOram Jun 19, 2024

DominicOram left a comment

DominicOram Jun 27, 2024

d-perl Jun 28, 2024 •

edited

Loading

DominicOram left a comment

d-perl Jul 17, 2024

		"""Move the aperture into required position, move out the backlight, retract the detector shutter,
		and set the attenuator to transmission."""



		@pytest.fixture(autouse=True)
		def patch_open_to_prevent_dls_reads_in_tests():

349 rotation scan now collects OAV snapshots #1443

349 rotation scan now collects OAV snapshots #1443

Conversation

rtuck99 commented Jun 12, 2024

To test:

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

rtuck99 Jun 19, 2024 • edited Loading

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

d-perl Jun 28, 2024 • edited Loading

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

rtuck99 Jun 19, 2024 •

edited

Loading

d-perl Jun 28, 2024 •

edited

Loading