-
Notifications
You must be signed in to change notification settings - Fork 87
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
Enable collect for multiple detectors #1651
Conversation
e5eea62
to
acf1818
Compare
eff37e4
to
fa5cbda
Compare
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.
Looks great. The effort on these tests is hugely valuable. I am suggested additional docstrings and added one comment on DocHolder
for consideration, but I'm happy for this to go ahead.
) | ||
|
||
|
||
def read_Readable(self) -> Dict[str, Reading]: | ||
return dict(det2=dict(value=1.2, timestamp=0.0)) | ||
class DocHolder(dict): |
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.
This is clever.
I'll mention that there is a utility in the test utils with a similar functionality, but I like the simplicity of this one.
It might be worth considering using collections.defaultdict(list)
instead, which I think is a more "mainstream" / familiar way to achieve this, and refactoring assert_emitted
into a function. On the other hand, I'm not sure that buys much over implementation here. Just a thought.
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.
The choice between:
class DocHolder(dict):
def append(self, name, doc):
self.setdefault(name, []).append(doc)
def assert_emitted(self, **numbers: int):
assert list(self) == list(numbers)
assert {name: len(d) for name, d in self.items()} == numbers
def test_plan():
docs = DocHolder()
RE(plan(), docs.append)
docs.assert_emitted(start=1, descriptor=1, resource=1, datum=1, event=1, stop=1)
and
from collections import defaultdict
def assert_emitted(docs: Dict[str, list], **numbers: int):
assert list(docs) == list(numbers)
assert {name: len(d) for name, d in docs.items()} == numbers
def test_plan():
docs = defaultdict(list)
RE(plan(), lambda name, doc: docs[name].append(doc))
assert_emitted(docs, start=1, descriptor=1, resource=1, datum=1, event=1, stop=1)
I think I marginally prefer the second, but not enough to change all the tests. @abbiemery I leave the choice up to you...
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 am in complete agreement. Slight preference, but not sure it's worth the effort to change.
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.
Given the time pressure of these now, i'd rather make that a new issue to clean things up if it is preferred.
Fix Test Cases for "Enable collect for multiple detectors" (1651)
Description
Changes the collect method in bundler to handle the collection of multiple detectors. Contains the logic to collect stream resources using the maximum common number of frames written, determined by the minimum index.
Asset
has been altered to only address aresource
anddatum
and a newStreamAsset
has been created to addressstream_resource
andstream_datum
.Motivation and Context
This enables logic to be moved out of Ophyd-async, generalising the process
Fixes #1649.
How Has This Been Tested?
TBC.