Skip to content

Don't forget unattenuated energy and restore discarded sim variables in SimCalorimeterHitProcessor #2198

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.

Sign up for GitHub

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

Jump to bottom
Merged
sly2j merged 1 commit into from
Nov 24, 2025
Merged

Don't forget unattenuated energy and restore discarded sim variables in SimCalorimeterHitProcessor #2198

sly2j merged 1 commit into from
Nov 24, 2025
+14 −7

Conversation

@sly2j
Copy link
Contributor

@sly2j sly2j commented Nov 22, 2025 •
edited by wdconinc
Loading

Briefly, what does this PR introduce?

This PR does addresses two issues in SimCalorimeterHitProcessor:

  1. The unattenuated energy is necessary to benchmark e.g. clustering algorithm performance, or to train AI algorithms. While it was originally foreseen to be available through 2-step simulated hit processor, but was then not added once we combined both steps for performance reasons.
  2. Some variables in the SimCalorimeterHit were silently removed in Plugging in Pulsecombiner and Pulsenoise for BIC #2076 as they were deemed "not needed". However, this was not documented, and leaves the actual SimCalorimeterHits that we use for our digi/reco in a partially filled state with some information irretrievably lost.
    I addressed the issues as follows
  3. Keep track of the unattenuated energy and store it with the contribution members of the SimCalorimeterHit so it can still be accessed. It means that SimCalorimeterHit.energy/sum(contribution.energies) equals the effective attenuation.
  4. Restored the discarded PID/position.xyz/... fields

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • 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 github-actions bot added the topic: calorimetry relates to calorimetry label Nov 22, 2025
@sly2j sly2j requested a review from mariakzurek November 22, 2025 00:15
@sly2j sly2j requested a review from wdconinc November 22, 2025 00:25
@github-actions
Copy link
Contributor

github-actions bot commented Nov 22, 2025

Capybara summary for PR 2198

Last updated 2025-11-21T19:58-05:00 ea8a23c

wdconinc
wdconinc approved these changes Nov 22, 2025
View reviewed changes
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.

This looks sensible to me.

I assume the impact on performance is near-zero. We presumably would have worse compression due the storage of non-zero x,y only, which is likely negligible even for large hit contribution count in the BIC.

@wdconinc
Copy link
Contributor

wdconinc commented Nov 22, 2025

(Failing eicweb trigger is known issue, and being resolved.)

@mhkim-anl
Copy link
Contributor

mhkim-anl commented Nov 24, 2025

My apologies for not documenting that the corresponding variables were discarded. I'll make sure to document such changes more thoroughly when a PR is submitted so that they are more visible to everyone.

I've confirmed that the unattenuated energies are correctly restored in EcalBarrelScFi{P, N}AttenuatedHitContributions by comparing the edm4hep and eicrecon outputs. This algorithm should work as intended once it is merged.

@sly2j sly2j added this pull request to the merge queue Nov 24, 2025
Merged via the queue into main with commit b9f268d Nov 24, 2025
125 of 126 checks passed
@sly2j sly2j deleted the fix-calo-dont-discard-true-energy branch November 24, 2025 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wdconinc wdconinc wdconinc approved these changes

@mariakzurek mariakzurek Awaiting requested review from mariakzurek

Assignees

@sly2j sly2j

@mhkim-anl mhkim-anl

Labels

topic: calorimetry relates to calorimetry

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sly2j @wdconinc @mhkim-anl