Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Dataset Attribute type for Pytrees #5732
Add Dataset Attribute type for Pytrees #5732
Changes from 46 commits
253515f
7180296
1a7fa19
9858055
a618d21
dd5c7e4
c7ca2c1
45141b3
8086b3f
521e7fd
450de94
73ec5ed
6c71b57
0757766
2882ce5
5a6d0e5
dcf1bb9
4088fa6
fafc2fe
703e3ab
73d1626
f5d209d
e7af0b3
5d63558
c499706
215f39c
7f7d582
0c13848
86bddda
1c1c8e5
a5007ef
f4d8fb5
f8458d1
cf851af
bc6ded6
504e853
11aa983
8bfc3f2
001ada4
671023c
4803d12
136a331
2e71c62
b80543d
45b6006
1333128
2904d1e
e22b1dc
f8ce69e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Catching
exceptions
might be expensive for large data. Is there a way to rather rely onif
conditions 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.
I think it's better to delegate to numpy here - otherwise we'd need to implement a check that
leaves
is homogenous and array-compatible, which would likely just be duplicating numpy's logic. This would also be slower in the ideal case thatleaves
is homogenous.I had the same concern about performance, but datasets are read a lot more than they're written, and
DatasetArray
is a lot more compact and performant thanDatasetList
. So the tradeoff makes sense IMOThere 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.
Any chances of this causing issues with doing shot-based measurements? The tests seem to be passing, so maybe no problem?
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.
It's hard to be 100% sure but it shouldn't cause a problem -
Sequence
is less restrictive thantuple
so anything that worked before will still work