(#1327) ispyb omega_start, axis_start now read from hardware for gridscans #1445
Conversation
| def populate_axis_info_for_snapshot( | ||
| self, data_collection_info: DataCollectionInfo, omega_start: float | None | ||
| ): | ||
| if omega_start is not None: | ||
| data_collection_info.omega_start = omega_start | ||
| data_collection_info.axis_start = omega_start | ||
| data_collection_info.axis_end = omega_start | ||
| data_collection_info.axis_range = 0 |
There was a problem hiding this comment.
Should: These collection parameters aren't necessarily tied to snapshots, they're where the diffraction data was taken so I think the function name is misleading.
There was a problem hiding this comment.
It's got the name because it is an implementation of an abstract method which is called from the base class _handle_oav_snapshot_triggered method - so the method definitely is snapshot-specific.
| def populate_axis_info_for_snapshot( | ||
| self, data_collection_info: DataCollectionInfo, omega_start: float | None | ||
| ): | ||
| # TODO in subsequent PR which depends on rotation snapshots 349 | ||
| pass | ||
|
|
There was a problem hiding this comment.
Should: I don't think we should do this at all for the rotation we can probably always just use the start angle given in the parameters. The snapshots for XRC and rotation have subtlety different purposes for the user:
- For XRC the snapshots are used work out what grids to do, therefore they will always be taken at the same angles as the datacollection
- For rotation they are just diagnostics to prove to the user we have centered on the crystal, they could be taken at arbitrary rotations that don't match the rotation we would be doing during data collection. So I think it'll potentially cause us issues in the future to tie the two together.
There was a problem hiding this comment.
This was going to be implemented for rotations before we had our previous discussion, however the PR for that never got raised as we decided not to do it. I will remove the comment and leave it empty.
I was originally going to in the second PR for rotations, create a second snapshot event type that was rotation specific for this very reason, and then move the snapshot handlers into the callback subclasses and rename the event types, since they are plan-specific.
If you would like me to do that, I can move those changes across from the branch where I have saved them (minus the rotation snapshot event).
There was a problem hiding this comment.
As discussed, we should fix this by moving the snapshot code into the ispyb child classes e.g. #1454 then just putting the omega_start code into the grid snapshot one but not in the rotation snapshot one
| "axisstart": -90.0, | ||
| "axisend": -90.0, | ||
| "datacollectionnumber": 2, | ||
| "omegastart": 90.0, | ||
| "omegastart": -90.0, |
There was a problem hiding this comment.
Must: Sorry, I know we discussed this but upon reflection I think we should make sure that we get the sign right in this PR (maybe just invert it in the callback?) then we can remove in the follow up issue. Otherwise if we release this before getting round to the next issue we break analysis
rtuck99
force-pushed
the
1327_omega_start_should_be_read_from_hardware
branch
from
6ab0d45 to
5ece6bd
Compare
| @@ -86,17 +81,13 @@ def activity_gated_event(self, doc: Event) -> Event: | |||
| event_descriptor = self.descriptors.get(doc["descriptor"]) | |||
| if event_descriptor is None: | |||
| ISPYB_LOGGER.warning( | |||
| f"Ispyb handler {self} recieved event doc {format_doc_for_log(doc)} and " | |||
| f"Ispyb handler {self} received event doc {format_doc_for_log(doc)} and " | |||
There was a problem hiding this comment.
❤️ Every time I see this I think about correcting it, thank you for doing so
|
I think this does fully fix #1327 so have tagged this as such |
…read from hardware for gridscans (DiamondLightSource/hyperion#1445) * (DiamondLightSource/hyperion#1327) ispyb omega_start, axis_start now read from hardware for gridscans * (DiamondLightSource/hyperion#1327) Response to PR comments * (DiamondLightSource/hyperion#1327) refactor ispyb_callback classes as per PR discussion
Fixes #1327 and fixes #1454
Gridscans should now read populate the
omega_startandaxis_startvalues in ispyb by reading the smargon omega directly during grid detection when snapshots are taken.The corresponding changes for rotations will be made in a separate PR
as this depends on #1443Note that the values will be inverted from previous as they are taken from hardware; this change should be temporary until DiamondLightSource/mx-bluesky#247 is fixed
Link to dodal PR (if required): #N/A
(remember to update
setup.cfgwith the dodal commit tag if you need it for tests to pass!)To test:
omega_start/axis_start/axis_range/axis_endvalues