Allow doing gridscans with generic number of grids#1623
Allow doing gridscans with generic number of grids#1623olliesilvester wants to merge 18 commits intomainfrom
Conversation
| if not self.omega_starts_deg | ||
| else self.omega_starts_deg[ | ||
| 0 | ||
| ], # This value is probably a lie after this PR... Could it be stored somewhere else? Should be an experiment param, not detector |
There was a problem hiding this comment.
It's metadata that we set on the detector but AFAIK it's not used downstream for gridscans. Would probably be fine to always set omega_start and omega_increment to 0, 0 for this tbh
| x_steps: int = Field(gt=0) | ||
| y_steps: int = Field(gt=0) | ||
| omega_starts_deg: list[float] = Field( | ||
| default=[GridscanParamConstants.OMEGA_1, GridscanParamConstants.OMEGA_2] |
There was a problem hiding this comment.
Do we need defaults here? Why default to 2 grids at all?
There was a problem hiding this comment.
Only because this is the current "standard". But maybe it's better without a default
| default=[GridscanParamConstants.BOX_WIDTH_UM] * 2 | ||
| ) | ||
| x_steps: PositiveInt # Currently this must be the same for each grid for panda scan | ||
| y_steps: list[PositiveInt] |
There was a problem hiding this comment.
It might make sense to hold a list of objects like _SingleGrid here and push the logic for building the grids into each single grid then SpecifiedGrids just holds the logic for concatenating them
There was a problem hiding this comment.
I think this probably will look nicer. The external parameters should still give all the grid info together, eg
omega_starts_deg: list[float]
x_step_size_um: PositiveFloat
y_step_sizes_um: list[PositiveFloat]
x_steps: PositiveInt
y_steps: list[PositiveInt]
are all specified together. Then, internally we make SpecifiedGrids which contains: a list of SingleGrids (each of which has all the info needed to specify that one grid); as well as the concatenation bits: num_images, scan_spec, num_grids.
This will add another reasonable chunk onto the PR though. Better to accept this as technical debt and put into another issue or address now? I'd normally just do it, but I'm running out of time...
There was a problem hiding this comment.
I think write up as another issue and address another time unless @rtuck99 disagrees.
The external parameters should still give all the grid info together
I actually think the external params should reflect what is possible. i.e. it's not clear that all those lists need to be the same length. IMO a better interface would be:
class SingleGrid:
omega: float
y_step_size: PositiveFloat # actually maybe should be outside this as there is no way of specifying differently for each grid
y_steps: PositiveInt
x_step_size_um: PositiveFloat
x_steps: PositiveInt
grids: list[SingleGrid]
again, new issue though.
There was a problem hiding this comment.
OK, I agree that i's better to be clear about what has to be the same length rather than relying on runtime validation. I was concerned that it would make the json blob look really ugly having to specify each grid like that. I think ugly json is preferable to unclear parameter rules though.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1623 +/- ##
==========================================
- Coverage 92.81% 92.78% -0.04%
==========================================
Files 153 153
Lines 8642 8671 +29
==========================================
+ Hits 8021 8045 +24
- Misses 621 626 +5
🚀 New features to boost your workflow:
|
olliesilvester
changed the title
|
FYI @rtuck99 this is a breaking change to hyperion - the external parameters have changed a bit. I'm happy to help Hyperion adjust for this |
2 participants
Fixes #364 , #410
Changes nexus callbacks and the parameter model to allow for a generic number of grids done in the gridscan plan, rather than fixed fixed to doing 2 x 2D grids.
Note that this the ispyb integration with this will be done in a separate PR. Ispyb callback still expects 2 x 2D grids.
Also note that any plans using grid detection still only work for 2 X 2D grids
See issue for refactoring this here
Link to dodal PR (if required): #XXX
(remember to update
pyproject.tomlwith the dodal commit tag if you need it for tests to pass!)Instructions to reviewer on how to test:
Checks for reviewer