[DF] Fix Snapshot of RVecs read from a TTree#10268
[DF] Fix Snapshot of RVecs read from a TTree#10268eguiraud merged 2 commits intoroot-project:masterfrom
Conversation
|
Starting build on |
|
Build failed on ROOT-ubuntu2004/soversion. Failing tests: |
|
Build failed on windows10/cxx14. Failing tests: |
and and for TBranchObject, we probably need to look at |
|
Ok, that seems more complex than the current logic 😅 |
Until v6.26, this could never happen: RVecs were written out as std::vectors. Now we write RVecs as RVecs, and we want to be able not only to read them back, but to correctly wire the output branches of a Snapshotted tree to RVec input branches.
Before this commit, the logic in SnapshotHelper did not take into account the case of RVecs that are being read from an input TTree (that was not possible until v6.26). Because of how the logic was structured, that meant that Snapshot was trying to write out those RVecs as C-style arrays instead, obviously badly failing at it. This patch tries to make the flow of this logic a bit more readable and fixes the problem mentioned above by treating input branches of RVec type the same way we already treat std::vectors. This fixes root-project#10225.
|
Starting build on |
pcanal
left a comment
pcanal
left a comment
There was a problem hiding this comment.
We should probably also have (add if not there already) a test of what happens if trying to read other STL collection into a RVec.
|
@pcanal I quickly tried with #include <ROOT/RDataFrame.hxx>
#include <array>
int foo() {
{
std::array<int, 4> a{1, 2, 3, 4};
TFile f("f.root", "recreate");
TTree t("t", "t");
t.Branch("a", &a);
t.Fill();
t.Write();
}
auto c = ROOT::RDataFrame("t", "f.root")
.Filter([](std::array<int, 4> &a) { return a[0] == 1; }, {"a"})
.Filter(
[](ROOT::RVecI &v) {
return v.size() == 4 && v[0] == 1 && v[3] == 4;
},
{"a"})
.Count()
.GetValue();
return c;
}yields I'll put it on my to-do list to investigate further. As I mentioned above the safest way to fix this would be to switch from "if this is not a Define, or a std::vector, or an RVec, or a TClonesArray, then it must be a C-style array and it needs special care" to "if this is not a C-style array, then no special care needed", but even with your help I could not figure out how to code "is this a C-style array?". |
|
Finally got around to check this and the situation is pretty much ok. std::vector, std::array and std::deque branches can all be written out from a Snapshot correctly as those same types. Only std::vector can be written out as an RVec as well, with Snapshot doing the conversion on the fly. There are a few warts in error reporting for unsupported cases but that's another issue. |
4 participants
Before this commit, the logic in SnapshotHelper did not take into
account the case of RVecs that are being read from an input TTree
(that was not possible until v6.26). Because of how the logic was
structured, that meant that Snapshot was trying to write out those
RVecs as C-style arrays instead, obviously badly failing at it.
This patch tries to make the flow of this logic a bit more readable
and fixes the problem mentioned above by treating input branches
of RVec type the same way we already treat std::vectors.
This fixes #10225.
@pcanal the logic in this commit could be simplified if instead of "if it's not every other case, it must be a C-style array" I could do instead "if it's not a C-style array, then it must be one of the other cases". But I'm not sure how to check for "branch is a C-style array", see https://mattermost.web.cern.ch/root/pl/4n1qbh363tdfjc1hs3khe6y84o