Skip to content

Hadrons calo entry merge#4

Merged
tmadlener merged 32 commits intomainfrom
Hadrons_Calo_entry_merge
Apr 29, 2026
Merged

Hadrons calo entry merge#4
tmadlener merged 32 commits intomainfrom
Hadrons_Calo_entry_merge

Conversation

@peter-mckeown
Copy link
Copy Markdown
Collaborator

@peter-mckeown peter-mckeown commented Jun 3, 2025

(Copied from Original Repo MR! 26
This merge request adds two features:

  1. 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

  2. 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.

Copy link
Copy Markdown
Member

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

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.

Comment thread include/DDML/LoadHdf5.h Outdated
Comment thread include/DDML/DDMLEventAction.h Outdated
Comment thread include/DDML/DDMLEventAction.h Outdated
Comment thread include/DDML/DDMLEventAction.h Outdated
Comment thread include/DDML/DDMLRunAction.h Outdated
Comment thread CMakeLists.txt Outdated
@peter-mckeown peter-mckeown mentioned this pull request Nov 14, 2025
@peter-mckeown
Copy link
Copy Markdown
Collaborator Author

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

@tmadlener
Copy link
Copy Markdown
Member

Is there anything of interest still left in this PR? Or have all the changes been picked up via the spinoff PRs we created?

@peter-mckeown
Copy link
Copy Markdown
Collaborator Author

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

@peter-mckeown peter-mckeown force-pushed the Hadrons_Calo_entry_merge branch from 455bd8d to ecde042 Compare April 22, 2026 12:29
@peter-mckeown
Copy link
Copy Markdown
Collaborator Author

@tmadlener I've cleaned this up a bit now, and made the option to record particle info at the calorimeter face a runtime option.

@peter-mckeown peter-mckeown requested a review from tmadlener April 22, 2026 13:37
Copy link
Copy Markdown
Member

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

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).

Comment thread src/Geant4FastHitMakerGlobal.cc Outdated
Comment thread CMakeLists.txt Outdated
Comment thread src/CMakeLists.txt Outdated
Comment thread src/CMakeLists.txt Outdated
Comment thread include/DDML/DDMLEventAction.h Outdated
Comment thread include/DDML/DDMLRunAction.h Outdated
peter-mckeown and others added 6 commits April 27, 2026 14:33
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>
@peter-mckeown
Copy link
Copy Markdown
Collaborator Author

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).

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.

@peter-mckeown peter-mckeown requested a review from tmadlener April 27, 2026 13:58
@tmadlener
Copy link
Copy Markdown
Member

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 declareProperty to avoid having to re-compile things for changing behavior?

@peter-mckeown
Copy link
Copy Markdown
Collaborator Author

@tmadlener Good spot- I've cleaned up FastMLShower.h now

@tmadlener tmadlener enabled auto-merge (squash) April 29, 2026 17:59
@tmadlener tmadlener merged commit 6bf0a11 into main Apr 29, 2026
7 checks passed
@tmadlener tmadlener deleted the Hadrons_Calo_entry_merge branch April 29, 2026 17:59
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.

2 participants