Refactor wrapped scan plans so they more user friendly

[oliwenmandiamond]

This change aims to make the scan syntax much more user friendly. Some examples are show below:

Old syntax

from dodal.plans import wrapped as dpw
>>> RE(dpw.list_grid_scan([], [(x, [1, 5, 10, 50]), (y, [2, 5, 10, 15, 18])]))

>>> RE(dpw.step_grid_scan([], [(x, [1, 10, 1]), (y, [1, 5, 1])]))

# Blueapi client example
>>> bc.plans.step_grid_scan([], [(x, [1, 10, 1]), (y, [1, 5, 1])])

New syntax

>>> RE(dpw.list_grid_scan([], (x, [1, 5, 10, 50]), (y, [2, 5, 10, 15, 18])))

>>> RE(dpw.step_grid_scan([], (x, 1, 10, 1), (y, 1, 5, 1)))

# Blueapi client example
>>> bc.plans.step_grid_scan([], (x, 1, 10, 1), (y, 1, 5, 1))

This assumes BlueAPI supports *args

For demo using with BlueAPI client, please see DiamondLightSource/blueapi#1662

Instructions to reviewer on how to test:

1. Check implementation and logic is correct
2. Test coverage is sufficient.

Checks for reviewer

• Would the PR title make sense to a scientist on a set of release notes
• If a new device has been added does it follow the standards
• If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
• Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.28%. Comparing base (3c6eab0) to head (012433a).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2050   +/-   ##
=======================================
  Coverage   99.28%   99.28%           
=======================================
  Files         366      370    +4     
  Lines       14367    14397   +30     
=======================================
+ Hits        14264    14294   +30     
  Misses        103      103           

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[EmsArnold]

A few comments, but hopefully nothing too onerous. I would like to try it on test rig or similar if it hasn't been tested there yet.

[EmsArnold]

Though it's possible to use the different gird_scan plans for scanning one motor, is this something we want to be advertising/promoting? In my mind, grid_scans should only be used for scanning in 2 or more dimensions, but happy to be told differently if there's a use case I'm missing.

[oliwenmandiamond]

It is perfectly valid, though you get the most functionality if you use more than one motor. I personally don't think we should be blocking the use of one motor though if bluesky plans themselves are perfectly okay with handling it.

The doc string of bp.grid_scan is

(function) def grid_scan(
    detectors: Sequence[Readable[Unknown]],
    *args: Unknown,
    snake_axes: Iterable[Unknown] | bool | None = None,
    per_step: PerStep | None = None,
    md: CustomPlanMetadata | None = None
) -> MsgGenerator[str]
Scan over a mesh; each motor is on an independent trajectory.

Parameters
detectors : list or tuple
list of 'readable' objects

*args
patterned like (motor1, start1, stop1, num1, motor2, start2, stop2, num2, motor3, start3, stop3, num3, ... motorN, startN, stopN, numN)

The first motor is the "slowest", the outer loop. For all motors except the first motor, there is a "snake" argument: a boolean indicating whether to following snake-like, winding trajectory or a simple left-to-right trajectory.

snake_axes : boolean or iterable, optional
which axes should be snaked, either False (do not snake any axes), True (snake all axes) or a list of axes to snake. "Snaking" an axis is defined as following snake-like, winding trajectory instead of a simple left-to-right trajectory. The elements of the list are motors that are listed in args. The list must not contain the slowest (first) motor, since it can't be snaked.

per_step : callable, optional
hook for customizing action of inner loop (messages per step). See docstring of bluesky.plan_stubs.one_nd_step (the default) for details.

md : dict, optional
metadata

See Also
bluesky.plans.rel_grid_scan bluesky.plans.inner_product_scan bluesky.plans.scan_nd

[EmsArnold]

I agree there's no reason to block it, but I'm not sure we need to put a one-motor example within the docstring for scans intended for indepentent trajectories.
From the bluesky plan docstring(s) it looks like the pattern for example concurrent scans (scan, list_scan) is one axis and then n axes, while independent scans (grid_scan, etc.) is only for n axes. I would lean toward keeping this format here.

[EmsArnold]

In principle, I think the refactor is good. I think the move to supplying parameters as sets of trajectories makes the plans more user friendly. I think the grouping into trajectories is a good compromise between an un-typed list of all *args and minimisation of brackets which the original wrapping used. That being said, until this is handled in blueAPI, relying on *args will not work and this will either have to wait for that support from blueAPI or work around the lack of support on the short term.

I haven't made up my mind whether I prefer the pattern of trajectory, *extra_trajectories rather than a potential *all_trajectories for scans which share a type for the first trajectory and any subsequent trajectories (i.e. everything outside step_scan and step_rscan). I appreciate that keeping this pattern may make this nicer across different scan types (particularly for facility users who are using different scan types). In my experience, during a session users will tend to use one type of scan manually, and once they have a working script they won't make too many changes (and likely won't be swapping a step-type scan for a num-type scan). This may mean that users never even notice the difference between num_scan and step_scan. I'm not sure if this is enough though to justify simplifying the scans which lend themselves to being simplified though. I also have a relatively limited experience of scripts used across Diamond, so happy to be told that loads of users use a large number of scan types during their experiments.

It would be worth updating the description now (or in the near future if there are changes coming) to capture the current syntax to accomplish a 1D and nD scan.

[oliwenmandiamond]

In principle, I think the refactor is good. I think the move to supplying parameters as sets of trajectories makes the plans more user friendly. I think the grouping into trajectories is a good compromise between an un-typed list of all *args and minimisation of brackets which the original wrapping used. That being said, until this is handled in blueAPI, relying on *args will not work and this will either have to wait for that support from blueAPI or work around the lack of support on the short term.

Yes, I will try and pin down a timeline with core team on when *args will be fixed and way up worth waiting for that for this to be merged or force us not use *args as the wait is too long.

I haven't made up my mind whether I prefer the pattern of trajectory, *extra_trajectories rather than a potential *all_trajectories for scans which share a type for the first trajectory and any subsequent trajectories (i.e. everything outside step_scan and step_rscan).

I opted for trajectory, extra_trajectory because if you use just *args / list for everything, this means that all of the arguments are optional. You then have to add extra error handling if no *args are provided, If you split this so that the first trajectory is always mandatory and all others are extras, then the static typing and function itself will do our job for us and no additional error checking is required.

Specific scans such as step_scan also require different arguements for the primary trajectory and all extra ones. The primary is (movable, start, stop step) and all extra is (movable. start, step) as they use the primary calculated number of points for all extra trajectories. Separating them naturally fixes this problem too.

I appreciate that keeping this pattern may make this nicer across different scan types (particularly for facility users who are using different scan types). In my experience, during a session users will tend to use one type of scan manually, and once they have a working script they won't make too many changes (and likely won't be swapping a step-type scan for a num-type scan). This may mean that users never even notice the difference between num_scan and step_scan. I'm not sure if this is enough though to justify simplifying the scans which lend themselves to being simplified though. I also have a relatively limited experience of scripts used across Diamond, so happy to be told that loads of users use a large number of scan types during their experiments.

I'm not sure what you mean here, isn't this the point of clearly static type checking the plan with documentation and examples so they will notice and understand the difference between the two different types of scan? One takes num as argument and the other gives it the primary trajectory.

It would be worth updating the description now (or in the near future if there are changes coming) to capture the current syntax to accomplish a 1D and nD scan.

Yes I will update the description shortly!

[EmsArnold]

I'm not sure what you mean here, isn't this the point of clearly static type checking the plan with documentation and examples so they will notice and understand the difference between the two different types of scan? One takes num as argument and the other gives it the primary trajectory.

Sorry, this was poorly articulated on my part! For scans which take the same type for the initial trajectory and any subsequent trajectories, users shouldn't care about the differences between the first trajectory and any further trajectories (within that singular scan type). They should still care deeply about the differences between concurrent vs independent nD scans, and step vs list vs num scans.

My comment about users noticing the difference between scan types comes more from a science-driven perspective. For example, if you're interested in determining resolution you'll always need to know your step size - and thus will likely want to define your scan based on step size. Conversely if you always want a constant number of steps (for example if you're rotating around an arc or full circle), you may never think about step size. From my experience, users will be unlikely to use both step_scan and num_scan during one session. If we're talking purely about expected user flow over the course of hours/days, in my experience there's some playing around at the start of a session to get a good experimental set up (manual alignment, manual scans, script(s), etc.), and then users forget about manual controls & scans that they won't need for the rest of the experiment. In this way, users shouldn't care about what type something is - this isn't to say that it shouldn't be validated or enforced.

From an end-user point of view, I question why I should care about the difference between trajectory and extra_trajectories when I'm not doing a step_(r)scan. However, I do appreciate that keeping trajectory and extra_trajectories means that, regardless of the type of scan you're doing, extra_trajectories can always be optional.