Skip to content

Conversation

@peter-mckeown
Copy link
Collaborator

Splitting part 1 from MR! 4:

Adds 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

Copy link
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.

I think this looks good in general. there are a few (more or less) minor comments below for some specifics.

Some other general remarks:

  • There is quite a few places where commented code is still included. I would have a preference for removing that
  • There are a few places where std::cout is used. Can we change those to use the dd4hep logging facilities?

In the end the PionCloudsModel is rather similar to the CaloCloudsTwoAngleModel and we could think about trying to factor out the similarities of those into a separate class. But that is definitely not in the scope of this PR for me.

@peter-mckeown
Copy link
Collaborator Author

peter-mckeown commented Nov 25, 2025

Thanks a lot for the review @tmadlener ! I think all your comments should be resolved now- I also went through and replaced all instances where std::cout is used with dd4hep logging facilities. Therefore I hope this will resolve Issue 1 as well.
I agree- there is a lot of commonality between the PionClouds loading and CaloClouds. I think as this is currently more about the functionality for including hadron showers it is probably best to wait with a common class as you say, especially because this is currently just loading from a hdf5 file rather than actually running inference.

Copy link
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.

Finally managed to get around to resolving the merge conflict here. I think it was straight forward enough for me to not mess up, but a second look would still be greatly appreciated.

@peter-mckeown
Copy link
Collaborator Author

@tmadlener looks good to go to me

@tmadlener tmadlener enabled auto-merge (squash) December 16, 2025 10:22
@tmadlener tmadlener merged commit 5f5f738 into main Dec 16, 2025
4 checks passed
@tmadlener tmadlener deleted the Hadrons_merge branch December 16, 2025 10:24
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.

3 participants