Add Array spec #191
Add Array spec #191
Conversation
src/scanspec/specs.py
Outdated
| """ | ||
|
|
||
| axis: Axis = Field(description="An identifier for what to move") | ||
| _midpoints: npt.NDArray[np.float64] | None = Field( |
There was a problem hiding this comment.
Tempted to call these array_midpoints etc to solve the name clash rather than keep the leading underscore. What do you think?
There was a problem hiding this comment.
I don't have a strong opinion against the variable being two words, it does look a bit dissonant from the rest of the code, but it does look better than having _ at the start. I'll change the names and we can see how it feels.
* Changed Array class attribute names * Removed option for defining a Gap array * Removed unnecessary bits of code * Removed check for same size arrays in the Array spec
src/scanspec/specs.py
Outdated
| array_midpoints: npt.NDArray[np.float64] | None = Field( | ||
| description="Array describing the midpoint of each frame", default=None | ||
| ) | ||
| array_upper: npt.NDArray[np.float64] | None = Field( | ||
| description="Array describing the upper bounds of the array", default=None | ||
| ) | ||
| array_lower: npt.NDArray[np.float64] | None = Field( | ||
| description="Array describing the lower bounds of the array", default=None | ||
| ) | ||
| array_duration: npt.NDArray[np.float64] | None = Field( | ||
| description="Array describing the duration of each point in the array", | ||
| default=None, | ||
| ) |
There was a problem hiding this comment.
Hmm, now I've seen this I'm tempted by a much shallower interface, see what you think:
array: passed straight toDimensionasmidpoints=self.arraybounds: optional, of lengthlen(array) + 1, passed toDimensionaslower=bounds[:-1], upper=bounds[1:]durations: optional, passed toDimensionasduration=self.durations
Then if you pass bounds=True and self.bounds is None only then do the calculation of bounds = self.array[1:] - self.array[:-1]
This mean you can't do gaps, but if they want that they can Concat multiple Arrays together
There was a problem hiding this comment.
I quite like this idea! The Spec looks a lot neater and I think in the end will be easier to provide a single array with all the bounds.
I had a go at changing the implementation based on that and will test it against some real hardware later today.
There was a problem hiding this comment.
One thing that came up while I was testing the new implementation is the fact that if we lock the calculations of the bounds behind bounds == True in the calculate method this might cause problems as some of the trigger logic classes that were already implement just use the default value for bounds, which in most cases is false.
This could cause situations where the users creates the Array spec with only the bounds but is not capable of pass the use of them downstream as, for example, you can't pass bounds to a PmacTrajectoryTriggerLogic.
There was a problem hiding this comment.
My idea was that you would still wrap Array with Fly if you wanted bounds calculations. So you could do Array("x", [1, 2, 3]) to give you a step scan with bounds of the first point being [1, 1], or Fly(Array("x", [1, 2, 3])) with bounds of the first point being [0.5, 1.5]
There was a problem hiding this comment.
My bad! I didn't realised that the calculate method in Fly already set the bounds==True.
I think it all makes sense now
- Changed the implementation of the Array spec to make it more clean. - Only midpoints or bounds arrays are required and based on which arrays were provided the missing ones will be calculated. - Fixed typos from previous commits
Implemented Array spec based on #168. Tried keeping it similar with what was developed for Scan Point Generator.
Development was tested against real hardware in P99 and results seem to be what I expected.