SimCalorimeter Hit Contribution Time After SimCalorimeterHitProcessor Should Include Travel Time #1907

[mhkim-anl]

Briefly, what does this PR introduce?

Currently we are (optionally) attenuation calorimeter hit contributions in SimCalorimeterHitProcessor to be evaluated at a certain position in the detector. However, we are not updating the time to correspond to this attenuated signal leaving the contributions in an inconsistent state. Moreover, this step has to be done at some point during pulse generation. This can easily be fixed by adding this to SimCalorimeterHitProcessor.

This PR is currently a draft.

What kind of change does this PR introduce?

• Bug fix (issue #__)
• New feature (issue SimCalorimeter Hit Contribution Time After SimCalorimeterHitProcessor Should Include Travel Time #1907)
• Documentation update
• Other: __

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?

[github-actions]

Capybara summary for PR 1923

• rec_dis_10x100_minQ2=0_craterlake_10x100
• rec_dis_10x100_minQ2=1000_craterlake_tracking_only
• rec_dis_18x275_minQ2=0_craterlake_18x275
• rec_dis_18x275_minQ2=1000_craterlake_18x275
• rec_dis_18x275_minQ2=1000_craterlake_18x275_MT
• rec_dis_5x41_minQ2=0_craterlake_5x41
• rec_e_1GeV_20GeV_craterlake
• rec_pi_1GeV_20GeV_craterlake
  ^{_{Last updated 2025-06-27T16:59-04:00 7eb76d4}}

[wdconinc]

Division-by-zero safety should be added; suggestions below. Ideally without adding branching.

[wdconinc]

Avoid abbreviations.

Suggested change

	const double propaTime =
	m_attenuationReferencePosition
	? std::abs(m_attenuationReferencePosition.value() - ih.getPosition().z) /
	m_cfg.propagationSpeed
	: 0.;
	hit_accum.add(contrib.getEnergy() * attFactor, contrib.getTime() + propaTime,
	ih.getPosition());
	const double propagationTime =
	m_attenuationReferencePosition
	? std::abs(m_attenuationReferencePosition.value() - ih.getPosition().z) /
	m_cfg.propagationSpeed
	: 0.;
	hit_accum.add(contrib.getEnergy() * attFactor, contrib.getTime() + propagationTime,
	ih.getPosition());

[wdconinc]

Hmm, you set this configuration parameter to zero by default, and then you divide by it without conditions. Please make sure the default value is not dangerous like that. I'd suggest that the default behavior should be 'infinite' propagation speed (but, of course, not std::infinite).

Another options is to specify the inverse propagation speed, and default that to zero. Since you would multiply instead of divide, that's inherently safer.

And once you're specifying inverse propagation speed, you can maybe just specify (effective) index of refraction, where a default of zero is infinite speed, a value of 1 is physical speed of light, and a value > 1 would be more realistic.

[ruse-traveler]

I would second the idea of the refraction index! I think that's a nice way of making things safe! (With some explanatory comments above the declaration, of course 😉)

Another (clunkier) approach could be to check the value of the parameter:

if (m_cfg.propagationSpeed > 0.0) {
  /* add propagation time */
}

[ruse-traveler]

Thanks for the development! I second Wouter's suggestions, and once those are addressed I think this is good!