-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename multiplier to frames_per_event and move to first dim of shape #726
base: main
Are you sure you want to change the base?
Rename multiplier to frames_per_event and move to first dim of shape #726
Conversation
… dataset description" This reverts commit 488d7eb.
…inished moving the multiplier into the DataKey.shape (needs testing)
…ve-multiplier-to-first-dim
…ve-multiplier-to-first-dim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question about shapes when frames_per_event
is 1: do we want to always "squeeze" the shape?
I.e. there are a couple of options:
- For 2d arrays:
[1, h, w]
->[h, w]
whenframes_per_event = 1
[frames_per_event, h, w]
whenframes_per_event > 1
- For scalar values:
[1,]
->[]
whenframes_per_event = 1
[frames_per_event,]
whenframes_per_event > 1
Currently, it is set up such that if the result would be a single scalar value, the shape would be replaced with []
. Otherwise, the shape always contains the extra dim.
…ve-multiplier-to-first-dim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is close, just a few minor notes. We should try setting this up in the lab and running a test w/ collecting data from different devices w/ different frames_per_event
to make sure it behaves as expected (and also to maybe work out the needed changes to the consolidators).
@@ -21,7 +21,7 @@ | |||
Triggerable, | |||
WritesStreamAssets, | |||
) | |||
from event_model import DataKey | |||
from event_model import DataKey # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the # type: ignore
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does pyright
also complain? If it needs a py.typed
then please could you make an event-model PR to add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pyright does not seem to complain, event-model is missing a py.typed marker so I can add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR here: bluesky/event-model#346
I think I'd be in favor of avoiding such squeezing, because then we'd need a separate parameter to let us know if it had been squeezed or not. Say we have a frames_per_event of 1 w/ a dataset that's 10 x 10. If we squeeze we get [10, 10] as the shape, but there's no way of telling if this is actually a 1D dataset of size 10 w/ 10 frames per event. |
@jwlodek so the current squeezing behavior for the shape is
The final change I would make based on your comment would be to remove the squeezing on scalar values from [1,] -> []. |
Should be ready to review once more. The new shape behavior is such that the |
This PR does the following:
multiplier
->frames_per_event
frames_per_event
as the first dimension ofDataKey.shape
DetectorWriter.get_indices_written()
andDetectorWriter.observe_indices_written()
is divided byframes_per_event
so that it actually captures the correct amount of exposures in each index (except for PandA which explicitly says it only has 1 "frame" per event)Add unit tests showing that stream resources are actually batches of exposuresself._writer.open()
andself._writer.get_indices_written()
. The writer needs to be opened in order to get the indices written. Otherwise, it has no idea whatframes_per_event
to use when returning the index last written.I could not actually add tests using bluesky plans and inspecting the data afterword because
TriggerInfo
is hardcoded inStandardDetector
. I think it is a separate issue that should be raised since it would enhance the scope of this PR. I will open an issue for this soon and mention it below.Otherwise, I have a few open questions regarding my understanding of ophyd-async as well as the implementation which I will also leave as review comments. Please see below.
Closes #576