DPL Analysis: Extended histogram registry to use TH2 and TH3#4063
DPL Analysis: Extended histogram registry to use TH2 and TH3#4063ktf merged 2 commits intoAliceO2Group:devfrom
Conversation
|
why making the histogramspec templated, rather than having a templated helper to construct a non templated one? |
OK, still I am thinking how to cope with different histograms requring different arguments - shall we have one big HistogramSpec containing all possible fields matching different histograms? |
There was a problem hiding this comment.
Why making a template method, rather than a templated helper? Something like:
registry.insert(hs<TH1>());
This way you move the complexity on the helper rather than on the registry, which, IMHO it's more manageable / customizable.
There was a problem hiding this comment.
OK, then we are back to making HistogramSpec templated? IMHO HistogramSpec needs to be templated to create different histogram types in a nicer way.
a30beb5 to
e433607
Compare
|
Ciao @ktf , let me know if this is ok for you. I noticed also that we store unique ptrs to TH1, but THn and THnSparse aren't derived from it, so they cannot be stored here. Shall we forget about them for now? |
There was a problem hiding this comment.
Why do you you need to pass the type and the string with the type name?
There was a problem hiding this comment.
The string is a remainder from the primary version, if it didn't have any purpose, I will delete it.
There was a problem hiding this comment.
My point is that if you have histSpec<TH1F>() you know you need to put "TH1F" in the spec. Unless that serves some other usecase.
There was a problem hiding this comment.
To be honest, I don't know if the spec string is needed now?
Hm, for that I'd have to store hardcoded lists of types and strings to set string without many ifs etc.
Actually, then one could pass just string and no templates will be needed. If hardcoded list is not a problem, maybe it would be better from the user's point of view...?
There was a problem hiding this comment.
Please use fmt::format, not boost format. Why do you need the type as a string? Also, why do you return a tuple<uint32_t, TH1*>? A std::pair would have been ok, no? Why not just the HistogramSpec as before?
There was a problem hiding this comment.
Ok, I will adjust the strings and tuple.
If I return histogram spec, I lose the type information for HistogramRegistry constructor. I would need to have HistogramSpec or HistogramRegistry templated. Now I just pass ready histograms of proper types as TH1*.
3d76a61 to
4103f83
Compare
4103f83 to
6ae1ad7
Compare
…liceO2Group#4063)" This reverts commit 7c43a8a.
Ciao @ktf,
this is a small introductory PR to allow for using different histograms in the histogram registry.
THn/THnSparse have different set of arguments in the constructors so I'd need to add HistogramConfigSpec specialization for these. TH1/2/3 can also accept different arguments (arrays of bins instead of min and max values), so that would require yet another Config specializations.
Or perhaps we should not store histogram-dependent values in a generic type?