Initial VMXm FGS plan #942

Tom-Willemsen · 2023-10-23T15:45:26Z

First pass at a FGS plan for VMXm - to test and refine on the beamline.

Based largely on existing FGS plan with references to the following devices removed:

ap_sg
flux
s4_slits
undulator
xbpm

Attenuator replaced with VmxmAttenuator

Nothing has been done about either shutter - understand that the fast shutter is being controlled by motion program, and that slow shutter can be controlled by GDA prior to calling into hyperion? This assumption might be wrong, check it on the beamline.

Needs DiamondLightSource/dodal#211

codecov · 2023-10-24T10:46:12Z

Codecov Report

Attention: 91 lines in your changes are missing coverage. Please review.

Comparison is base (92b23da) 93.32% compared to head (a27463b) 90.38%.
Report is 86 commits behind head on main.

❗ Current head a27463b differs from pull request most recent head 35ee736. Consider uploading reports for the commit 35ee736 to get more accurate results

Files	Patch %	Lines
.../experiment_plans/vmxm_flyscan_xray_centre_plan.py	39.28%	68 Missing ⚠️
...nteraction/callbacks/xray_centre/nexus_callback.py	36.00%	16 Missing ⚠️
...ction/callbacks/xray_centre/callback_collection.py	66.66%	4 Missing ⚠️
...hyperion/external_interaction/nexus/nexus_utils.py	50.00%	2 Missing ⚠️
...hyperion/external_interaction/nexus/write_nexus.py	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     DiamondLightSource/hyperion#942      +/-   ##
==========================================
- Coverage   93.32%   90.38%   -2.94%     
==========================================
  Files          54       55       +1     
  Lines        2591     2747     +156     
==========================================
+ Hits         2418     2483      +65     
- Misses        173      264      +91

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Tom-Willemsen · 2023-11-03T14:49:55Z

run_hyperion.sh

@@ -92,7 +92,7 @@ if [[ $START == 1 ]]; then
            exit 1
        fi

-        ISPYB_CONFIG_PATH="/dls_sw/dasc/mariadb/credentials/ispyb-artemis-${BEAMLINE}.cfg"
+        ISPYB_CONFIG_PATH="/dls_sw/dasc/mariadb/credentials/ispyb-artemis-i03.cfg"


Need input from ispyb team to fix?

src/hyperion/experiment_plans/experiment_registry.py

src/hyperion/experiment_plans/vmxm_flyscan_xray_centre_plan.py

src/hyperion/external_interaction/callbacks/xray_centre/callback_collection.py

src/hyperion/external_interaction/callbacks/xray_centre/nexus_callback.py

src/hyperion/external_interaction/nexus/nexus_utils.py

src/hyperion/unit_tests/test_lookup_table.txt

test_parameters.json

Tom-Willemsen · 2023-11-24T10:49:32Z

src/hyperion/external_interaction/nexus/nexus_utils.py

    eiger_params = EigerDetector(
-        "Eiger 16M", (detector_pixels.height, detector_pixels.width), "Si", 46051, 0
+        "Eiger 9M", (detector_pixels.height, detector_pixels.width), "CdTe", 2418770, 0


Definitely needs fixing before merge - this would break i03 if merged as-is

Tom-Willemsen · 2023-11-24T10:49:55Z

src/hyperion/experiment_plans/experiment_registry.py

+    # FIXME - HACK - plans commented out here because VMXm complains about no
+    # aperture_scatterguard which is needed for some plans.
+    # Need a way to either mark plans as being relevant only on some beamlines, or
+    # generally allow hyperion to *start* even if some plans reference nonexistent
+    # devices (and then fail at runtime if that plan is actually used).


As per code comment - definitely needs fixing before merge.

Tom-Willemsen · 2023-11-24T10:52:29Z

deploy/deploy_hyperion.py

@@ -32,7 +32,7 @@ def deploy(self, url):
        deploy_repo.git.checkout(self.latest_version_str)

        print("Setting permissions")
-        groups_to_give_permission = ["i03_staff", "gda2", "dls_dasc"]
+        groups_to_give_permission = ["i02-1_staff", "gda2", "dls_dasc"]


Fix this before merge. But don't just add vmxm/i03 to the list as that would give i03 permissions on vmxm and vice-versa.

Probably f"{beamline}_staff" would work, check that group exists for all supported beamlines.

Tom-Willemsen · 2023-11-24T10:53:16Z

deploy/deploy_hyperion.py

+    elif args.beamline == "i03":
+        return f"/dls_sw/{args.beamline}/software/bluesky"
+    elif args.beamline == "i02-1":
+        return f"/dls_sw/{args.beamline}/software/bluesky"


Remove this, it doesn't do anything, it's obsolete.

Tom-Willemsen · 2023-11-24T10:55:19Z

src/hyperion/experiment_plans/vmxm_flyscan_xray_centre_plan.py

@@ -0,0 +1,225 @@
+from __future__ import annotations


For this whole file: there's obviously a lot of duplication, but also a lot of instrument-specific functionality.

Probably spin out an issue to come up with the right set of abstractions.

Also probably shouldn't be called _xray_centre_plan on VMXm, as it doesn't actually do xray centring at all - it does real data collections! So just vmxm_fast_grid_scan maybe. Will also need updating in GDA if changed.

Tom-Willemsen · 2023-11-24T10:57:16Z

src/hyperion/external_interaction/callbacks/ispyb_callback_base.py

+            self.params.hyperion_params.ispyb_params.undulator_gap = doc["data"].get(
+                "undulator_gap", 0.0
+            )
+            self.params.hyperion_params.ispyb_params.synchrotron_mode = doc["data"].get(
+                "synchrotron_machine_status_synchrotron_mode", None
+            )
+            self.params.hyperion_params.ispyb_params.slit_gap_size_x = doc["data"].get(
+                "s4_slit_gaps_xgap", 0.0
+            )
+            self.params.hyperion_params.ispyb_params.slit_gap_size_y = doc["data"].get(
+                "s4_slit_gaps_ygap", 0.0
+            )


I think write an issue in hyperion to think harder about this.

Conceptually, I don't think the ispyb_callback class should "know" about instrument-specific things like s4_slits, which either don't exist or don't make sense for other beamlines. I think it should be getting this list of parameters from some beamline-specific configuration or parameter.

But in the short term, I think this will work for both i03 and VMXm - even if it will end up putting nonsense values into VMXm's ispyb entries.

Tom-Willemsen · 2023-11-24T10:58:47Z

src/hyperion/external_interaction/callbacks/rotation/callback_collection.py

-    Cast to a list to pass it to Bluesky.preprocessors.subs_decorator()."""
+    Cast to a list to pass it to Bluesky.preprocessors.subs_decorator().
+
+    Note: specific to i03."""


It would be nice to have this kind of comment (or an equivalent naming standard) throughout hyperion so that we can easily tell what is beamline specific, and what is being kept generic.

Maybe create an issue to come up with this standard and then retroactively apply it where we know there is instrument-specific code.

Tom-Willemsen · 2023-11-24T11:00:29Z

src/hyperion/external_interaction/callbacks/xray_centre/callback_collection.py


    nexus_handler: GridscanNexusFileCallback
    ispyb_handler: GridscanISPyBCallback
    zocalo_handler: XrayCentreZocaloCallback

    @classmethod
    def from_params(cls, parameters: InternalParameters):
-        nexus_handler = GridscanNexusFileCallback()
+        nexus_handler = GridscanNexusFileCallback(create_i03_goniometer_axes)


Still feels like we should be passing this from "higher up" the call chain. But probably fine for now. Possibly create an issue to think about where this comes from.

Tom-Willemsen · 2023-11-24T11:01:29Z

src/hyperion/external_interaction/callbacks/xray_centre/nexus_callback.py

+
+
+class Gridscan2DNexusFileCallback(CallbackBase):
+    """Similar to above, but for a 2D gridscan"""


I think the duplication with the class above shouldn't block merging in the short term, but let's write an issue to improve this in the longer term.

Tom-Willemsen · 2023-11-24T11:03:34Z

src/hyperion/external_interaction/nexus/nexus_utils.py

+    chi: float = 0,
+    phi: float = 0,
+) -> Goniometer:
+    """Returns a Nexgen 'Goniometer' object with the dependency chain of I03's Smargon


Suggested change

"""Returns a Nexgen 'Goniometer' object with the dependency chain of I03's Smargon

"""Returns a Nexgen 'Goniometer' object with the dependency chain of VMXm's axes

Comment is obviously wrong!

olliesilvester · 2023-12-04T13:21:26Z

src/hyperion/external_interaction/callbacks/ispyb_callback_base.py

-            LOGGER.info("Creating ispyb entry.")
-            self.ispyb_ids = self.ispyb.begin_deposition()
-            LOGGER.info(f"Recieved ISPYB IDs: {self.ispyb_ids}")
+        LOGGER.info("Creating ispyb entry.")


How come this part is no longer within the if statement?

If I recall correctly, VMXm only uses the READ_HARDWARE plan so they need to create the ispyb entry on that event - otherwise they won't get an entry at all. For i03, we need to check whether this is right.

It feels like a bit of a hack anyway to have this ispyb stuff listening on a specific type of message - but I think David or Dom have been doing some work to pull the ispyb interactions into an ophyd device? If that's the case then this may go away.

olliesilvester · 2023-12-04T13:31:43Z

src/hyperion/external_interaction/callbacks/xray_centre/callback_collection.py

+
+
+@dataclass(frozen=True, order=True)
+class VmxmFastGridScanCallbackCollection(AbstractPlanCallbackCollection):


Do you think this would be nicer if it was called something like FastGridScanNoZocaloCallBackCollection, and then has the goniometer axes creation as a parameter? Then it isn't specific to VMXm

See my comment of the equivalent bit of code for i03:

Still feels like we should be passing this from "higher up" the call chain. But probably fine for now. Possibly create an issue to think about where this comes from.

So yes I agree with you :)

Don't put too much effort into this though, because I think David (or Dom?) is looking at pulling all or most of these callbacks into ophyd devices which would then make this obsolete anyway.

olliesilvester · 2023-12-04T13:38:06Z

Things we need to change before we merge: (You wrote all this stuff in the comments anyway, this is mainly for me)

In the deploy script, make sure the permissions go to the correct beamline (eg vmxm don't get permissions on i03 and vice versa) - Deploy script should give correct permissions to correct staff mx-bluesky#307
Contact ispyb to get separate credentials for Vmxm and i04 (Get an ISPBYB_CONFIG_PATH for i04 and VMXm mx-bluesky#321), then we can change ISPYB_CONFIG_PATH accordingly - Get an ISPBYB_CONFIG_PATH for i04 and VMXm mx-bluesky#321
Get a way for Hyperion to ignore certain plans on certain beamlines / not require all plans to sucessfully connect to all devices. (https://github.com/DiamondLightSource/hyperion/pull/942/files#r1404216293) I prefer the first of these two solutions: Hyperion should be able to ignore certain plans for certain beamlines #1020
Make sure the eiger_params in create_detector_parameters are generic enough for other beamlines: Make 'create_detector_parameters' more generic mx-bluesky#306
There are also a bunch of merge conflicts which we will eventually need to sort out. It probably makes sense to fix the above things before doing this though.

Other things which aren't as urgent but we should still do at some point:

Find the right set of abstractions between vmxm_flyscan_xray_centre_plan and the regular flyscan_xray_centre_plan. Also rename this for VMXm since it isn't actually doing any xray centring - instead the grid scan is the actual data collection. An issue for this should be made once this code is merged.
The ispyb callback class should use a beamline-specific config, since otherwise we have to put default values into ispyb entries for other beamlines - values for devices which don't exist for that beamline.
Make it clear throughout hyperion as to which parts are generic and which parts are beamline specific
Think of a nicer way to create the nexus file handler with beamline specific goniometer axes. Right now the callback collection classes are beamline specific, but only because the goniometer axes are beamline specific
Create abstractions for the GridscanNexusFilecallback, since there's currently a lot of duplication.
Since callbacks are being reworked right now anyway, I won't make an issue for some of these yet

Initial VMXm FGS plan

b3f08e2

Tom-Willemsen mentioned this pull request Nov 3, 2023

VMXm: fast grid scans DiamondLightSource/mx-bluesky#323

Open

Tom Willemsen added 2 commits October 27, 2023 16:23

VMXM - changes on beamline

325d810

VMXm: fast grid scan beamline changes

627e3c7

Tom-Willemsen commented Nov 3, 2023

View reviewed changes

Tom Willemsen added 2 commits November 7, 2023 16:43

VMXm fast grid scans: hyperion tidy-up

2b750e0

Merge remote-tracking branch 'origin/main' into VMXm_fast_grid_scans

2812e00

Tom-Willemsen force-pushed the VMXm_fast_grid_scans branch from 992927a to 9072827 Compare November 10, 2023 11:15

Merge tidy-ups

a27463b

Tom-Willemsen force-pushed the VMXm_fast_grid_scans branch from 9072827 to a27463b Compare November 10, 2023 15:19

Tom Willemsen added 2 commits November 17, 2023 09:31

Set omega as part of plan

f3d0bd8

Changes from VMXm beamtime

35ee736

Tom-Willemsen commented Nov 24, 2023

View reviewed changes

olliesilvester reviewed Dec 4, 2023

View reviewed changes

DominicOram mentioned this pull request Aug 27, 2024

Move open Hyperion PRs to mx_bluesky DiamondLightSource/mx-bluesky#184

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial VMXm FGS plan #942

Initial VMXm FGS plan #942

Tom-Willemsen commented Oct 23, 2023 •

edited

Loading

codecov bot commented Oct 24, 2023 •

edited

Loading

Tom-Willemsen Nov 3, 2023

Tom-Willemsen Nov 24, 2023

Tom-Willemsen Nov 24, 2023

Tom-Willemsen Nov 24, 2023

Tom-Willemsen Nov 24, 2023

Tom-Willemsen Nov 24, 2023

Tom-Willemsen Nov 24, 2023

Tom-Willemsen Nov 24, 2023

Tom-Willemsen Nov 24, 2023 •

edited

Loading

Tom-Willemsen Nov 24, 2023

Tom-Willemsen Nov 24, 2023

Tom-Willemsen Nov 24, 2023

olliesilvester Dec 4, 2023

Tom-Willemsen Dec 4, 2023 •

edited

Loading

olliesilvester Dec 4, 2023

Tom-Willemsen Dec 4, 2023

Tom-Willemsen Dec 4, 2023

olliesilvester commented Dec 4, 2023 •

edited

Loading



		class Gridscan2DNexusFileCallback(CallbackBase):
		"""Similar to above, but for a 2D gridscan"""

	"""Returns a Nexgen 'Goniometer' object with the dependency chain of I03's Smargon
	"""Returns a Nexgen 'Goniometer' object with the dependency chain of VMXm's axes



		@dataclass(frozen=True, order=True)
		class VmxmFastGridScanCallbackCollection(AbstractPlanCallbackCollection):

Initial VMXm FGS plan #942

Are you sure you want to change the base?

Initial VMXm FGS plan #942

Conversation

Tom-Willemsen commented Oct 23, 2023 • edited Loading

codecov bot commented Oct 24, 2023 • edited Loading

Codecov Report

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

Tom-Willemsen Nov 24, 2023 • 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

Tom-Willemsen Dec 4, 2023 • 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

olliesilvester commented Dec 4, 2023 • edited Loading

Tom-Willemsen commented Oct 23, 2023 •

edited

Loading

codecov bot commented Oct 24, 2023 •

edited

Loading

Tom-Willemsen Nov 24, 2023 •

edited

Loading

Tom-Willemsen Dec 4, 2023 •

edited

Loading

olliesilvester commented Dec 4, 2023 •

edited

Loading