-
Notifications
You must be signed in to change notification settings - Fork 60
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
Refactor the RNTupleWriter internals and reduce some code duplication #614
Refactor the RNTupleWriter internals and reduce some code duplication #614
Conversation
0514a03
to
35713a9
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.
Merging this to unblock #615 later today unless there are comments.
- Lift common type defs into one header to not redefine them - Keep all root related utilitie classes in one header
35713a9
to
ba9f090
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.
Thanks. Looks like boring, but important refactoring.
I would like to see some more documentation as mentioned in a comment
{ | ||
auto& mtx = getMutex<T>(); | ||
const auto& map = getMap<T>(); | ||
std::lock_guard lock{mtx}; |
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.
Whenever locks are being used one should document where why and when they are needed
TBranch* data{nullptr}; | ||
std::vector<TBranch*> refs{}; | ||
std::vector<TBranch*> vecs{}; | ||
std::vector<std::string> refNames{}; ///< The names of the relation branches |
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.
in principle one could do indices instead of strings here, but for the sake of simplicity we stay with this.
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.
Indices would be harder to work with here IMHO, because we don't maintain a list of all TBranch
es that we have (let alone their order). We simply use this to group the pointers that we get from the TTree
and let that do all the management.
BEGINRELEASENOTES
RNTupleWriter
internals a bit and reduce some code duplication between theRNTupleWriter
and theROOTWriter
getKeys
andgetValues
methods toGenericParameters
CollectionBranches.h
header and move all root related helpers intoutilities/RootHelpers.h
ENDRELEASENOTES
In more detail:
RNTupleWriter::CollectionInfo
struct toRNTupleWriter::CategoryInfo
and group the members that are used to store and write the GenericParameters data into that, such that all data belonging to a category are grouped togetherParamStorage
to more easily refer to a pair of keys and values (vectors). This is done in preparation for another PR that is in the works for addressing GenericParameters should be stored the same way for TTree and RNTuple backends #590.ROOTWriter
and theRNTupleWriter
into the newly createdutilities/RootHelpers.h
header