Add an RDataSource for podio files and collections #593
Conversation
There was a problem hiding this comment.
Thanks @kjvbrt. I wasn't aware this was possible in this generality. Very nice. I am probably still missing quite a few things, but I tried to summarize my current understanding below and added and a few comments / questions inline
For my general understanding, what this does is basically:
- Create a reader for each thread / slot
- Keep a vector of Frames, again one per thread / slot
- Populate a vector of collections per thread / slot, where
- each collection always is at the same position, so that we can simply index into it
I am wondering whether we could do with just one reader instead of multiple, but I suppose then we would probably have to do our own locking around that, because the RDataSource doesn't do that for us?
If I understand correctly, we could in the end even replace the current ROOTReader with the Reader that is introduced in #522 and effectively support all backends + RDataFrame?
|
The sanitizer workflows are most likely failing because the tests that produce the input files are not running. You can disable them by adding the new tests also to the list in |
src/ROOTDataSource.cc
Outdated
| // Get collections stored in the files | ||
| std::vector<std::string> collNames = frame.getAvailableCollections(); | ||
| // std::cout << "podio::ROOTDataSource: Found following collections:\n"; | ||
| for (auto& collName : collNames) { | ||
| const podio::CollectionBase* coll = frame.get(collName); | ||
| if (coll->isValid()) { | ||
| m_columnNames.emplace_back(collName); | ||
| m_columnTypes.emplace_back(coll->getValueTypeName()); | ||
| // std::cout << " - " << collName << "\n"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we also make the parameters of the frame available as columns, e.g. to access the cross section? :)
There was a problem hiding this comment.
Will do this in a follow up PR.
|
I continue with testing the changes made here in FCCAnalyses |
There was a problem hiding this comment.
As far as I can tell this is ready, right? From which ROOT version onwards should this work? Then we could add a simple check in CMake (similar to RNTuple).
I will push the minor code changes immediately. Once we have this, we can the add support for parameters in a follow-up PR, I would suggest.
| list(APPEND root_components_needed ROOTDataFrame) | ||
| endif() | ||
| find_package(ROOT REQUIRED COMPONENTS ${root_components_needed}) | ||
| if((ENABLE_RNTUPLE) AND (${ROOT_VERSION} VERSION_LESS 6.28.02)) |
There was a problem hiding this comment.
Is there a minimal version for RDataSource that we could / should check here for the RDataSource as well?
There was a problem hiding this comment.
The RDataSource is in ROOT for a long time (not experimental since ROOT 6.14), I tested this in nightlies stack (6.32)
There was a problem hiding this comment.
Alright, then I think we can also skip the check here. Thanks for checking.
|
EDM4hep CI workflows are fixed with #658 |
tmadlener
changed the title
|
I tested this in FCCAnalysis, seems to work... |
BEGINRELEASENOTES
podio::DataSourceas anRDataSourcefor working with podio (generated EDM) files and collections. ThisRDataSourceexposes the podio collections directly as columns forRDataFrames. We consider this fairly stable, but still in early stages, there might still be some (breaking) changes to thisENDRELEASENOTES