Skip to content

Building pulses from SimCalorimeterHitProcessor #1924

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

mhkim-anl
Copy link
Contributor

@mhkim-anl mhkim-anl commented Jun 20, 2025

Briefly, what does this PR introduce?

This algorithm, CalorimeterPulseGeneration, builds pulses for each hit implemented by SimCalorimeterHitProcessor. Sub-pulses were created for each contribution and combined to build pulse of a hit. Regarding the pulse shape, template pulse implemented by SiliconPulseGeneration was used. Before the sub-pulses are created, the energy deposit can be converted to number of photoelectrons if necessary.

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

Does this PR change default behavior?

@mhkim-anl mhkim-anl self-assigned this Jun 20, 2025
@mhkim-anl mhkim-anl linked an issue Jun 20, 2025 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Jun 27, 2025

@mhkim-anl mhkim-anl marked this pull request as ready for review June 27, 2025 19:55
Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

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

In my opinion this is much too similar to SiliconPulseGeneration to allow this level of code duplication. The main difference is in the input type, so I would propose to template it based on that.


public:
virtual ~SignalPulse() = default; // Virtual destructor
virtual double operator()(double time, double charge) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

This getter must be const. It should have always been const, and the EvaluatorPulse will need to deal with the map of parameters in a different way.

Comment on lines +84 to 86
if (type == "LandauPulse") {
return std::make_unique<LandauPulse>(params);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Factory must support all types.


void CalorimeterPulseGeneration::init() {
// Initialize seed
m_gen.seed(std::random_device{}());
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes the algorithm irreproducible.

@wdconinc wdconinc requested a review from veprbl June 28, 2025 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building pulse from the edm4hep::SimCalorimeterHit
2 participants