Skip to content

Conversation

minjungkim12
Copy link
Contributor

Briefly, what does this PR introduce?

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • [x ] 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?

No, it should be transparent.

@minjungkim12 minjungkim12 requested a review from ShujieL July 22, 2025 01:28
@minjungkim12 minjungkim12 requested a review from bschmookler July 22, 2025 01:28
Copy link
Contributor

@simonge simonge left a comment

Choose a reason for hiding this comment

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

Nice contribution. A few cursory comments to get things going, I haven't dived into the details.

I think this would sit better as a separate set of hits e.g. there is no input collection just the requested readout. Then the CollectionCollector would combine them later. This would stop you needing to clone the hits and help maintain the associations. There might be other opinions on this.

The N noise hits per system might be ok for a first pass but should later be replaced by a frequency per channel or frequency per surface area so when a MC timeframe period can be used it can be directly configured.

}

// 3.3 duplicate check
if (hitMap.find(cid) != hitMap.end())
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be needed, as soon as you distribute hits across time there could be multiple noise hits in the same element. It's not quite that simple as it will be digitization dependent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what to do at this moment - I will bring this point to tracking or reco. meeting!

@wdconinc
Copy link
Contributor

Note the recently merged #1934 which imposes the use of the EventHeader info for any algorithms that use randomness, to ensure reproducibility in concurrent running.

@minjungkim12 minjungkim12 force-pushed the Random_Noise_Injection_forTracker branch from 23bb179 to a6dae27 Compare August 10, 2025 12:58
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please remove the calibrations, fieldmaps, and gdml directories from this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants