Add a new test producing histograms and improve comments in tests #208
Conversation
| f = ROOT.TFile.Open("functional_producer_hist.root") | ||
| for i in range(2): | ||
| if ( | ||
| str(f.GetListOfKeys()[i]) |
There was a problem hiding this comment.
Is this guaranteed to be always ordered?
There was a problem hiding this comment.
In the current test I left multithreading disabled not to have too many multithreaded jobs running at same time since ctest doesn't know about this but after running it several times I never got the other ordering. I think the histograms may be registered when the class is created and in that case it would always be ordered but I'm not sure.
There was a problem hiding this comment.
The alternative would be to simply make a set of the names in the files and the expected ones and do a set comparison to make sure everything is there to avoid any ordering issues.
| // Not thread-safe! | ||
| Rndm::Numbers rndu(randSvc(), Rndm::Flat(0, 1)); | ||
| ++m_histograms[rndu()]; |
There was a problem hiding this comment.
Which part here is not thread safe? I suppose it's randSvc? Is that only used for seeding here? Can we make this into a thread safe example without too much work?
There was a problem hiding this comment.
I think the engine comes from randSvc and since you can do randSvc()->engine()->setSeeds({1, 2}); any time then I assume it's not going to work. There is something for multithreading here: https://gitlab.cern.ch/gaudi/Gaudi/-/blob/master/GaudiHive/src/HiveNumbers.h#L29 but this is not even being installed and it's only being used in one test in Gaudi.
|
|
||
| #include <string> | ||
|
|
||
| struct ExampleFunctionalProducerHist final : k4FWCore::Producer<edm4hep::MCParticleCollection()> { |
There was a problem hiding this comment.
Since we only produce an empty collection here, could we not make this a Consumer (or something else that doesn't have an output)?
There was a problem hiding this comment.
I thought it's more common to have a producer or transformer producing something and then making some histograms so as a example, and possible starting point, I think it's closer to what most people will want (?) But indeed the output is not doing anything. Or it could be a slightly more complicated algorithm that reads some input and histograms that.
There was a problem hiding this comment.
It could take the MCParticles as inputs and then histogram their energy. Or a different datatype for which we already have a functional producer. That would also remove the threadsafety issue with the random numbers(?)
BEGINRELEASENOTES
ENDRELEASENOTES