-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[DF] Allow snapshotting from TTree to RNTuple #19364
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
base: master
Are you sure you want to change the base?
Conversation
Templated snapshotting has been removed, so the "*Templated" and "*JITted" tests have become identical.
46413f9
to
26dc6f0
Compare
Test Results 21 files 21 suites 3d 10h 44m 47s ⏱️ For more details on these failures, see this check. Results for commit 70e54ce. ♻️ This comment has been updated with latest results. |
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.
Great! LG from an RNTuple perspective, I let RDataFrame experts comment if there's anything else to test in particular (but I think you already discussed offline)
{ | ||
ASSERT_EQ(v1.size(), v2.size()) << "Vectors 'v1' and 'v2' are of unequal length"; | ||
for (std::size_t i = 0ull; i < v1.size(); ++i) { | ||
EXPECT_EQ(v1[i], v2[i]) << "Vectors 'v1' and 'v2' differ at index " << i; |
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.
Just a heads up that we may need EXPECT_FLOAT_EQ
here, depending on what T
is. I think for the moment it's fine because { 1.f, 2.f, 3.f }
are exactly representable.
26dc6f0
to
70e54ce
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.
Great! RDF owners should give final approval.
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.
Thank you!
Snapshotting from TTree to RNTuple was initially disabled, because it was assumed that this would require adopting (parts of) the machinery of the RNTupleImporter to make it work. However, this is all already taken care of by the TTree's datasource.
One caveat is the support of leaflist branches. It is not possible to snapshot these fully, and users always need to explicitly pass their leaf names to
Snapshot
. This, however, is something fundamentally unsupported by the RDataFrame. The reason is that for snapshotting to TTree, it is impossible/overly complex to correctly set the branch addresses in the output TTree for these branches at runtime, without JITting. For RNTuple, however, this is likely more doable by using untyped record fields (similar to how it is done in the importer). This requires some more investigation, however, and if indeed possible will be addressed in a follow-up PR.