Skip to content
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

feat: Calorimeter hit digi simplification #794

Merged
merged 11 commits into from
Jul 27, 2023
Merged

Conversation

wdconinc
Copy link
Contributor

Briefly, what does this PR introduce?

This applies the #666 treatment to the CalorimeterHitDigi algorithms:

  • put all configuration in a single configuration struct for use with WithPodConfig mixin,
  • update the unit tests that are affected,
  • create a templated factory-factory to use in the plugins,
  • replace all old explicit RawCalorimeterHit factories with the new factory-factory.

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 @veprbl @nathanwbrei

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

No.

Does this PR change default behavior?

No.

Copy link
Contributor Author

@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.

No need to keep a pointer m_detector in the factory.

src/factories/calorimetry/CalorimeterHitDigi_factoryT.h Outdated Show resolved Hide resolved
src/factories/calorimetry/CalorimeterHitDigi_factoryT.h Outdated Show resolved Hide resolved
src/factories/calorimetry/CalorimeterHitDigi_factoryT.h Outdated Show resolved Hide resolved
@wdconinc
Copy link
Contributor Author

wdconinc commented Jul 26, 2023

Right... This is where randomness comes in. Unsurprisingly, the diffs are showing lots of small changes in the digitization, and then everything else. How can we avoid that?

@wdconinc
Copy link
Contributor Author

Maybe we need to consider a seed in the configuration for each digitization factory. Like https://github.com/eic/EICrecon/blob/main/src/algorithms/digi/PhotoMultiplierHitDigiConfig.h#L17.

@wdconinc wdconinc changed the title Calorimeter hit digi simplification feat: Calorimeter hit digi simplification Jul 26, 2023
@wdconinc wdconinc force-pushed the calorimeter-hit-digi-config branch from 23534eb to f0e4162 Compare July 27, 2023 00:11
@wdconinc
Copy link
Contributor Author

@veprbl Thanks! The threshold is exactly it, now all that's left in terms of diffs (for gun simulations) is changing collection IDs.

@wdconinc wdconinc requested a review from veprbl July 27, 2023 01:37
@wdconinc wdconinc enabled auto-merge (squash) July 27, 2023 03:06
@wdconinc wdconinc force-pushed the calorimeter-hit-digi-config branch from 06e6e1d to 5994e5e Compare July 27, 2023 19:22
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

Checked differences in edm4eic and flag artifacts, all looks good.

@wdconinc wdconinc merged commit 20c9d80 into main Jul 27, 2023
@wdconinc wdconinc deleted the calorimeter-hit-digi-config branch July 27, 2023 21:18
veprbl pushed a commit that referenced this pull request Jul 28, 2023
This applies the #666 treatment to the CalorimeterHitReco algorithms (on
top of #794):
- put all configuration in a single configuration struct for use with
WithPodConfig mixin,
- create a templated factory-factory to use in the plugins,
- replace all old explicit CalorimeterHit factories with the new
factory-factory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants