Skip to content
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

Make the TTree and RNTuple based backends write the GenericParameters the same way #625

Merged
merged 16 commits into from
Jun 20, 2024

Conversation

tmadlener
Copy link
Collaborator

@tmadlener tmadlener commented Jun 11, 2024

BEGINRELEASENOTES

  • Make the TTree based backend also write the GenericParameters as pairs of vectors (keys and values) to make the format the same for the RNTuple and the TTree based backends.

ENDRELEASENOTES

This was originally done in #615. See there for a bit more discussion.

@tmadlener tmadlener marked this pull request as draft June 11, 2024 14:29
@tmadlener tmadlener changed the title Uniform generic param storage Make the TTree and RNTuple based backends write the GenericParameters the same way Jun 11, 2024
@tmadlener tmadlener marked this pull request as ready for review June 11, 2024 18:19
@tmadlener tmadlener force-pushed the uniform-generic-param-storage branch from f8a353d to 8566bab Compare June 11, 2024 18:21
include/podio/utilities/RootHelpers.h Outdated Show resolved Hide resolved
src/ROOTWriter.cc Outdated Show resolved Hide resolved
@tmadlener tmadlener force-pushed the uniform-generic-param-storage branch from a9bc5a7 to 8850b63 Compare June 12, 2024 19:11
Comment on lines 11 to 14
df.Define(
"params",
"podio::root_utils::loadParamsFrom(GPIntKeys, GPIntValues, GPFloatKeys, GPFloatValues, GPDoubleKeys, GPDoubleValues, GPStringKeys, GPStringValues)",
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zehvogel, does this look like an acceptable compromise for using GenericParameters in RDataFrame until we have a proper RDataSource?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes thx!

@tmadlener tmadlener force-pushed the uniform-generic-param-storage branch from 894a6d3 to cff343b Compare June 13, 2024 08:24
src/ROOTWriter.cc Outdated Show resolved Hide resolved
Copy link
Collaborator

@hegner hegner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. That seems to do things in a very thorough way now.

What I really would like to see for the future is some documentation about the chosen file structure in the files created. Some file_structure_details.doc or similar

@tmadlener
Copy link
Collaborator Author

I agree, more documentation on this is necessary. Since, we also don't really have documentation on how we store collections, I would prefer a separate PR for the documentation. I can prepare that in parallel.

@tmadlener
Copy link
Collaborator Author

Documentation can be seen in #629, rendered by markdown here

@hegner hegner merged commit 82e6265 into AIDASoft:master Jun 20, 2024
18 checks passed
@tmadlener tmadlener deleted the uniform-generic-param-storage branch June 20, 2024 07:05
@jmcarcell
Copy link
Member

Something I found with this PR is that it fixes an issue I had only when making Debug builds with Clang 17. For some reason, the GenericParameters were not being saved in the file and all the tests for reading would fail. I picked up a working build of podio and read the files produced with this debug build and indeed the GenericParameters were not there. I never spent more time to figure out what was going on but out of all the build types and compilers (GCC and Clang) I only managed to reproduce it with Clang and -DCMAKE_BUILD_TYPE=Debug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GenericParameters should be stored the same way for TTree and RNTuple backends
4 participants