Conversation
tmadlener
left a comment
There was a problem hiding this comment.
Can this potentially be split into two separate PRs? It looks a bit like the actions are somewhat orthogonal to the HDF5 loading functionality changes.
Yes good idea- I've split the hadron shower functionality into MR! 5. Let's come back to this once that is merged |
|
Is there anything of interest still left in this PR? Or have all the changes been picked up via the spinoff PRs we created? |
I think the option to record information at the calo face is still useful- I'll revisit now that PR!5 is resolved |
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
455bd8d to
ecde042
Compare
|
@tmadlener I've cleaned this up a bit now, and made the option to record particle info at the calorimeter face a runtime option. |
tmadlener
left a comment
There was a problem hiding this comment.
There is some commented / dead code that could be removed (see below).
Looking through the code I am wondering whether we need the event action at all? It seems entirely unused(?) and the main setup parts for the analysis manager seem to happen in the run action.
Also for the (timing) instrumented build we dump to podio::UserDataCollections directly. Could we do something similar? In principle one could even record this information as an edm4hep::MCParticleCollection as that has all the necessary (and more) fields to store the information. That would have the advantage that one could potentially feed the output file back to ddsim directly (not entirely sure though).
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Thanks @tmadlener for the comments and the code clean up! I've added the suggestions. Regarding the need for the event action, I would prefer to hold onto it for now, as this is where we record the primary particle (i.e. initial particles given to ddsim) info in the event which does not necessarily match what is recorded at the point of triggering fast sim. Regarding using edm4hep, I agree using edm4hep here could be useful for feeding particles back into the event, if a user wanted to isolate just the particles at the calo face. I would suggest we open a new PR once this one is resolved. |
|
Ah that is a subtlety that wasn't clear to me. Makes sense to keep it then. Going to EDM4hep in a separate PR is fine for me. There is the one open comment about the unused getters (their use is commented out at the moment). Is that intentional, or is that functionality that would in the end be "toggled"? And if so can we make that toggle a runtime decision via some |
|
@tmadlener Good spot- I've cleaned up |
(Copied from Original Repo MR! 26
This merge request adds two features:
The possibility to configure models for hadronic shower simulation (including placement in ECAL + HCAL). So far testing has been performed for single particle events loaded from HDF5 - Now split into MR! 5
The option to record information about the particle which triggers fast sim at the calorimeter face (including PiD, Energy, position, direction).
This is achieved via Run/Event actions. It is configurable via ccmake, with the output being stored in a ROOT file.