Remove multiplier from resource docs, add it instead as an extra first dimension in descriptor shape

[jwlodek]

Per discussion today, it was decided that in order to keep the dimensionality of shape in the event descriptor consistent, that the multiplier should be added there as the first dimension. So for example in the case of two detectors w/ x-y dimensions of 10x10, with one collecting a single frame per acq, and one collecting 5, we would have

shape: (1, 10, 10)

and

shape: (5, 10, 10)

respectively in the descriptor. Since this tells you the multiplier, and you can fetch this information given a resource doc by looking back at the associated descriptor, the multiplier field can be removed from the resource doc creation.

[jwlodek]

@coretl @genematx @danielballan

[coretl]

@jwlodek was this something you were going to do, or should someone else pick it up?

[thomashopkins32]

Going to open a PR for this change soon.

From my understanding, the way this operates is that it holds off on producing a StreamResource until the index has been incremented by one. The index does not get incremented until multiplier number of "captures" or "exposures" have been processed.

So the only work to be completed for this issue is to do the following:

• Add the multiplier as the first dimension of DataKey.shape
• Ensure that the index provided by DetectorWriter.get_indices_written() and DetectorWriter.observe_indices_written() is divided by this multiplier so that it actually captures the correct amount of exposures
• Add unit tests showing that describe() works as intended
• Add unit tests showing that stream resources are actually batches of exposures

Also, I think batch_size is a better name for this...multiplier could mean anything and it took me a few passes to try and figure out what it actually meant.

[thomashopkins32]

Blocked on the merge of #606 since that reworks a lot of the overlapping code

[coretl]

* Ensure that the index provided by `DetectorWriter.get_indices_written()` and `DetectorWriter.observe_indices_written()` is divided by this `multiplier` so that it actually captures the correct amount of exposures

This should already be done in the code...

* Add unit tests showing that `describe()` works as intended

* Add unit tests showing that stream resources are actually batches of exposures

...although when you add these unit tests you may find it doesn't work as indended!

Also, I think batch_size is a better name for this...multiplier could mean anything and it took me a few passes to try and figure out what it actually meant.

Good idea to change the name, but we're trying to convey number of detector triggers/exposures/frames that make up a single index in a StreamDatum, so maybe frames_per_event would be better?

[thomashopkins32]

@coretl frames_per_event sounds good as well. I decided on batch_size since that lines up with what typical deep learning frameworks in Python use for the first dimension. Instinctively when I see a 4-d array that is supposed to be image data I assume that the first dimension is a batch size. I'm not sure how true this is for the would-be users of ophyd-async though. Let me know if you think frames_per_event would be better and I can change it to that.

This should already be done in the code...

I believe I found a few places where this was not done. Either way, I did not notice any unit tests that actually examine the behavior when the multiplier > 1.

[coretl]

Decided on frames_per_event. Also decided to rename StandardDetector -> FramingDetector.

[coretl]

Decided on frames_per_event. Also decided to rename StandardDetector -> FramingDetector.

Since then we have had second thoughts on the FramingDetector name, especially given that the unique thing about StandardDetector is that it is prepared with the number of triggers and type it takes, and the writer makes stream_resource and stream_datum. On balance it is better to leave it as StandardDetector and explain what that entails. If we have something later on that doesn't fit we will revisit that name then.

The frames_per_event name change should still take place.