AIDASoft / podio

PODIO
GNU General Public License v3.0
24 stars 60 forks source link

GenericParameters should be stored the same way for TTree and RNTuple backends #590

Closed tmadlener closed 4 months ago

tmadlener commented 6 months ago

Currently the ROOTWriter and ROOTReader (TTree) based store the GenericParameters of a Frame directly via a custom dictionary, e.g.:

https://github.com/AIDASoft/podio/blob/02a4b9d5d704eb572ce945594b4c85303e89e216/src/ROOTReader.cc#L42-L46

On the other hand the RNTuple backend splits these up into pairs of vectors for the keys and the values: https://github.com/AIDASoft/podio/blob/02a4b9d5d704eb572ce945594b4c85303e89e216/src/RNTupleWriter.cc#L249-L257

These should both use the same mechanism to avoid confusion. Given that RNTuple doesn't support writing map types and that pairs of vectors are easier to interpret without knowing about GenericParameters, we decided to switch the TTree based backend to that as well. See EDM4hep meeting on Apr 23, 2024

jmcarcell commented 6 months ago

At the time of writing the RNTupleWriter.cc there wasn't support for std::map but now there is support for it: https://github.com/root-project/root/pull/13904 So the cleaner choice is to move the RNTupleWriter to use std::map. However, someone complained today that uproot may not support std::map but I don't know if we want to have our files be compatible with uproot...

tmadlener commented 6 months ago

I would prefer the solution that makes the files more easy to read without root / with uproot actually. I also think that would probably play a bit nicer with RDataFrame (without having ever tested this).

Zehvogel commented 4 months ago

I also think that would probably play a bit nicer with RDataFrame (without having ever tested this).

For RDataFrame the easiest way to access GenericParameters is to include the header and use the real type.

ROOT.gInterpreter.Declare("#include <podio/GenericParameters.h>")
df = df.Define("xsec_fb", "PARAMETERS.getValue<float>(\"crossSection\")")

I.e. as long as GenericParameters makes it into the file as GenericParameters and not something like GenericParametersData I am happy :)

tmadlener commented 4 months ago

I am not sure we can really preserve the currently possible behavior for reading the parameters as a GenericParameter object as is possible now using TTrees. However, we might be able to simply offer similar functionality via the RDataSource in #593.

My currently favored options at the moment would be to either

Zehvogel commented 4 months ago

However, we might be able to simply offer similar functionality via the RDataSource in #593.

Yes please! But then it would be really good to have a clear time line for the RDataSource before we break stuff that works at the moment...

andresailer commented 4 months ago

Todo: Add something to make the use of GenericParameters easier in FCCAnalysis