Pulse generation from time-clustered Simcalorimeterhits using a shared template #2004

mhkim-anl · 2025-08-07T23:33:57Z

Briefly, what does this PR introduce?

This PR is for pulse generation from time-clustered SimCalorimeterHits. To generate a pulse from the contributions that are close in time, a timeID is assigned in the SimCalorimeterHitProcessor. Since both SimCalorimeterHit and SimTrackerHit share a common pulse generation mechanism, a shared template algorithm, PulseGeneration, has been implemented.

What kind of change does this PR introduce?

Bug fix (issue #__)
New feature (issue Pulse generation from time-clustered SimCalorimeterHits using a shared template #2001)
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?

for more information, see https://pre-commit.ci

…mcalorimeterhits-using

Associations are 3rd party objects that relate to two other objects that they connect. Relations are just fields pointing to another objects. This method works with relations and not associations.

for more information, see https://pre-commit.ci

…d template (fix: iwyu) (#2025) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/17116586226. Please merge this PR into the branch `2001-pulse-generation-from-time-clustered-simcalorimeterhits-using` to resolve failures in PR #2004. Auto-generated by [create-pull-request][1] [1]: https://github.com/peter-evans/create-pull-request Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions · 2025-08-22T03:53:37Z

Capybara summary for PR 2004

veprbl · 2025-08-22T05:13:47Z

Looks like this increases number of hit contributions and hits:

for more information, see https://pre-commit.ci

mhkim-anl · 2025-08-22T19:28:40Z

@veprbl
Thank you for checking how this PR effects to the number of BIC hits and contributions, and also the output file size. I'm sorry that I checked whether the algorithms were working properly, but didn't check how much the output file size increased.

I checked in more detail referring to your comments and commits. First, the increased number of hits and contributions are due to the timeID because it defines additional hits depending on their times.

It seems that setting the ignore_thres too low increased the output file size too much. I confirmed that many pulses reached the maximum number of time bins, which is 10000. I updated the ignore_thres to be 1/10 of the average pulse height for 1 GeV electrons generated at eta = 0.

Could you check if the file size from this change is acceptable? For now, I added EcalBarrelScFiPPulses and EcalBarrelScFiNPulses again for a possible test at Github. For reference, the output file size of my small pythia sample decreased as follows.

No BIC pulses: 27M
BIC pulses with ignore_thres of 1.0e-10: 2.6G
BIC pulses with ignore_thres of 1.0e-6: 104M

wdconinc · 2025-08-23T01:44:31Z

No BIC pulses: 27M\nBIC pulses with ignore_thres of 1.0e-10: 2.6G\nBIC pulses with ignore_thres of 1.0e-6: 104M

In simulation production campaigns we cannot afford a factor 4 increase in data volume for a single detector (or even in general).

I'd suggest removing this data collection from the default output, enable it in a CI test job to exercise the algorithm (ASAN, TSAN), and at some point we have a downstream algorithm that condenses the data volume again.

veprbl · 2025-08-23T20:28:46Z

Since we now running unit tests on PulseGenerator, we can just disable EcalBarrelScFi{P,N}Pulses in default running. Like @wdconinc says, the downstream factories will be able to produce those as intermediate collections without having them saved. And you can always enable storing those pulses in your local running for private analysis needs.

mhkim-anl · 2025-08-24T23:20:12Z

@wdconinc @veprbl
Thank you for your answers and suggestions. I understood the factor 4 increase is too much to save the EcalBarrelScFi{P, N}Pulses. I removed them from the output.

Then, regarding the next step, I would like to discuss it including the downstream algorithm at the coming simulation meeting. I would appreciate it if you could join the meeting on Aug 26.

By the way, could you let me know up to what factor of data size increase would be acceptable? I think there may still be a possible way to reduce the output size by optimizing both timestep and ignore_thres.

wdconinc · 2025-08-25T00:14:39Z

By the way, could you let me know up to what factor of data size increase would be acceptable? I think there may still be a possible way to reduce the output size by optimizing both timestep and ignore_thres.

As a rule of thumb, we don't want a single detector subsystem to take more than 20% of resources (storage, computing).

veprbl

This is in good shape now.

…mcalorimeterhits-using

veprbl

Approval still stands

…d template (fix: iwyu) (#2073) This PR applies the include-what-you-use fixes as suggested by https://github.com/eic/EICrecon/actions/runs/17595549256. Please merge this PR into the branch `2001-pulse-generation-from-time-clustered-simcalorimeterhits-using` to resolve failures in PR #2004. Auto-generated by [create-pull-request][1] [1]: https://github.com/peter-evans/create-pull-request Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

mhkim-anl · 2025-09-10T19:40:33Z

Thank you for reviewing this PR and merging it into the main branch. Actually, I was planning to leave a comment about my study on the BIC data size and request merging this PR without including EcalBarrelScFi{P, N}Pulses in the default output. So I would like to share my study on the BIC data size. I'll keep monitoring and optimizing the BIC data size. Thanks!

==============================================
Regarding the BIC data size in the output, I studied how much space it occupies and how much it can be reduced. Please refer to the following slides.
https://indico.bnl.gov/event/29441/contributions/112597/attachments/64382/111077/BIC_data_size.pdf

The slides show how much space the BIC data takes in the eicrecon output depending on

Q2 value of the sample
included data type

and how much the size can be reduced by

zero-filling branches that are unlikely to be used (e.g., CaloHitContribution's stepPosition.x and stepPosition.y)
optimizing parameters
excluding unnecessary date type.

Please refer to the followings for the captions located above each table.

Without Att.Hits and Pulses: EcalBarrelScFi{P, N}AttenuatedHits and EcalBarrelScFi{P, N}Pulses not included
With Att. Hits: EcalBarrelScFiAttenuatedHits included
Unnecessary values were set to 0: Zero-filling branches that are unlikely to be used
thres = 1.0e-5: Corresponds to 1p.e. level
Excluding RawHits: EcalBarrelScFiRawHits was excluded for testing because it will likely be removed once the digitization and new CALOROC data type (eic/EDM4eic#116) are implemented.

veprbl · 2025-09-10T19:54:23Z

Thank you, @minjungkim12 . From what I understand, the result of this is a 10-20 percent file increase? If yes, can we implement some of those extra optimizations you came up with?

mhkim-anl · 2025-09-11T16:47:59Z

@veprbl
Sure, I'll confirm it at the simulation meeting and submit the corresponding PR. I'll invite you when I discuss it at the simulation meeting.

mhkim-anl added 4 commits August 7, 2025 14:25

clustering contributions that sit in the same time window

32c732f

apply code formatter

fb4af8e

generate pulse from SimCalorimeterHit using a shared template algorithm

b9ee625

generate pulse from SimCalorimeterHit using a shared template algorithm

c3a9514

mhkim-anl linked an issue Aug 7, 2025 that may be closed by this pull request

Pulse generation from time-clustered SimCalorimeterHits using a shared template #2001

Closed

github-actions bot added topic: calorimetry relates to calorimetry topic: barrel topic: digitization labels Aug 7, 2025

[pre-commit.ci] auto fixes from pre-commit.com hooks

0c01c98

for more information, see https://pre-commit.ci

mhkim-anl self-assigned this Aug 7, 2025

mhkim-anl requested review from wdconinc, veprbl, simonge, sly2j and mariakzurek August 8, 2025 00:56

mhkim-anl and others added 6 commits August 14, 2025 18:24

Add addAssociations() in the HitAdapter

67876d3

fix a typo

60a56fd

lower the ignore_thres sufficiently

6268ba5

Merge branch 'main' into 2001-pulse-generation-from-time-clustered-si…

4b23a5d

…mcalorimeterhits-using

addAssociations() -> addRelations()

61417c8

Associations are 3rd party objects that relate to two other objects that they connect. Relations are just fields pointing to another objects. This method works with relations and not associations.

[pre-commit.ci] auto fixes from pre-commit.com hooks

de3dd84

for more information, see https://pre-commit.ci

wdconinc mentioned this pull request Aug 21, 2025

Pulse generation from time-clustered Simcalorimeterhits using a shared template (fix: iwyu) #2025

Merged

wdconinc and others added 2 commits August 21, 2025 00:04

disable saving EcalBarrelScFi{P,N}Pulses - file size is too large

05fafa1

veprbl force-pushed the 2001-pulse-generation-from-time-clustered-simcalorimeterhits-using branch from 4698884 to 05fafa1 Compare August 22, 2025 03:10

veprbl temporarily deployed to github-pages August 22, 2025 03:52 — with GitHub Actions Inactive

veprbl and others added 2 commits August 22, 2025 01:15

replace SiliconPulseGeneration with PulseGeneration

2557228

[pre-commit.ci] auto fixes from pre-commit.com hooks

3c62d7f

for more information, see https://pre-commit.ci

veprbl had a problem deploying to github-pages August 22, 2025 19:27 — with GitHub Actions Error

mhkim-anl and others added 2 commits August 22, 2025 14:27

updated the ignore_thres value

fcb1c3f

[pre-commit.ci] auto fixes from pre-commit.com hooks

71c4ee0

for more information, see https://pre-commit.ci

pre-commit-ci bot temporarily deployed to github-pages August 22, 2025 20:52 Inactive

Remove EcalBarrelScFi{P,N}Pulses

027c3d3

mhkim-anl temporarily deployed to github-pages August 24, 2025 23:45 — with GitHub Actions Inactive

veprbl previously approved these changes Aug 25, 2025

View reviewed changes

Optimized ignore_thres and timestep

e9a7938

mhkim-anl dismissed veprbl’s stale review via e9a7938 August 27, 2025 19:58

Merge branch 'main' into 2001-pulse-generation-from-time-clustered-si…

0cff6a7

…mcalorimeterhits-using

veprbl previously approved these changes Sep 9, 2025

View reviewed changes

wdconinc mentioned this pull request Sep 9, 2025

Pulse generation from time-clustered Simcalorimeterhits using a shared template (fix: iwyu) #2073

Merged

mhkim-anl temporarily deployed to github-pages September 9, 2025 22:06 — with GitHub Actions Inactive

veprbl dismissed their stale review via e7792ab September 9, 2025 22:40

veprbl approved these changes Sep 9, 2025

View reviewed changes

veprbl enabled auto-merge September 9, 2025 22:41

veprbl added this pull request to the merge queue Sep 9, 2025

veprbl temporarily deployed to github-pages September 9, 2025 23:21 — with GitHub Actions Inactive

Merged via the queue into main with commit 6e3493b Sep 9, 2025
121 of 122 checks passed

veprbl deleted the 2001-pulse-generation-from-time-clustered-simcalorimeterhits-using branch September 9, 2025 23:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Pulse generation from time-clustered Simcalorimeterhits using a shared template #2004

Pulse generation from time-clustered Simcalorimeterhits using a shared template #2004

Uh oh!

mhkim-anl commented Aug 7, 2025

Uh oh!

github-actions bot commented Aug 22, 2025 •

edited

Loading

Uh oh!

veprbl commented Aug 22, 2025

Uh oh!

mhkim-anl commented Aug 22, 2025

Uh oh!

wdconinc commented Aug 23, 2025

Uh oh!

veprbl commented Aug 23, 2025

Uh oh!

mhkim-anl commented Aug 24, 2025

Uh oh!

wdconinc commented Aug 25, 2025

Uh oh!

veprbl left a comment

Uh oh!

veprbl left a comment

Uh oh!

Uh oh!

mhkim-anl commented Sep 10, 2025

Uh oh!

veprbl commented Sep 10, 2025

Uh oh!

mhkim-anl commented Sep 11, 2025

Uh oh!

Uh oh!

Pulse generation from time-clustered Simcalorimeterhits using a shared template #2004

Pulse generation from time-clustered Simcalorimeterhits using a shared template #2004

Uh oh!

Conversation

mhkim-anl commented Aug 7, 2025

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

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

Does this PR change default behavior?

Uh oh!

github-actions bot commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Capybara summary for PR 2004

Uh oh!

veprbl commented Aug 22, 2025

Uh oh!

mhkim-anl commented Aug 22, 2025

Uh oh!

wdconinc commented Aug 23, 2025

Uh oh!

veprbl commented Aug 23, 2025

Uh oh!

mhkim-anl commented Aug 24, 2025

Uh oh!

wdconinc commented Aug 25, 2025

Uh oh!

veprbl left a comment

Choose a reason for hiding this comment

Uh oh!

veprbl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhkim-anl commented Sep 10, 2025

Uh oh!

veprbl commented Sep 10, 2025

Uh oh!

mhkim-anl commented Sep 11, 2025

Uh oh!

Uh oh!

github-actions bot commented Aug 22, 2025 •

edited

Loading