8000 LGADPulseGeneration -> SiliconPulseGeneration, LGADPulseDigitization -> EICROCDigitization by ssedd1123 · Pull Request #1778 · eic/EICrecon · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

LGADPulseGeneration -> SiliconPulseGeneration, LGADPulseDigitization -> EICROCDigitization #1778

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
merged 15 commits into from
Apr 9, 2025
Merged

LGADPulseGeneration -> SiliconPulseGeneration, LGADPulseDigitization -> EICROCDigitization #1778

merged 15 commits into from
Apr 9, 2025

Conversation

ssedd1123
Copy link
Contributor

Briefly, what does this PR introduce?

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?

LGADPulseGeneration class is removed in favor of SiliconPulseGeneration class. Added a SiliconPulseDiscretization class to convert the smooth pulse from SiliconPulseGeneration to it's discretized form with size of time bin set to be equal to that of BTOF LGAD clock cycle.

LGADPulseDigitization is renamed to SiliconPulseDigitization for a unified naming scheme.

Does this PR change default behavior?

BTOF now uses SiliconPulseGeneration.

@ssedd1123 ssedd1123 mentioned this pull request Apr 1, 2025
Merged
7 tasks
@simonge
Copy link
Contributor
simonge commented Apr 1, 2025

With regards to the naming, if the pulse generation is adopted by more than silicon detectors this might be subject to change.

By the time you get to the Digitization algorithms these are will be strongly ASIC dependent so here I would change the name to EICROCDigitization or similar

@ssedd1123 ssedd1123 mentioned this pull request Apr 1, 2025
Open
7 tasks
@ssedd1123
Copy link
Contributor Author

With regards to the naming, if the pulse generation is adopted by more than silicon detectors this might be subject to change.

By the time you get to the Digitization algorithms these are will be strongly ASIC dependent so here I would change the name to EICROCDigitization or similar

Name is changed as suggested.

@github-actions GitHub Actions
Copy link
Contributor
github-actions bot commented Apr 1, 2025 •
edited
Loa 8000 ding

Capybara summary for PR 1778

@veprbl
Copy link
Member
veprbl commented Apr 4, 2025

Please rebase on top of the approved version:

git fetch origin
git checkout pr/Silicon-Discretization
git rebase -i origin/master
# remove lines corresponding to commits until the "Fix LandauPulse conditions", save and exit
git push -f origin pr/Silicon-Discretization

@ssedd1123 ssedd1123 force-pushed the pr/Silicon-Discretization branch from 1af4c27 to 99cb23d Compare April 4, 2025 17:57
@veprbl veprbl changed the title Pr/silicon discretization LGADPulseGeneration -> SiliconPulseGeneration, LGADPulseDigitization -> EICROCDigitization Apr 4, 2025
veprbl
veprbl reviewed Apr 4, 2025
View reviewed changes
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.

Looks generally good, I have some comments.

ssedd1123 and others added 4 commits April 4, 2025 17:31
@ssedd1123 @veprbl
Update src/factories/digi/SiliconPulseDiscretization_factory.h
824e46b 
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
@ssedd1123 @veprbl
Update src/algorithms/digi/EICROCDigitization.cc
8000
a162347 
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
@ssedd1123
Fixed authorship
9789ef9
@ssedd1123
Merge branch 'pr/Silicon-Discretization' of https://github.com/eic/EI…
243b16e 
…Crecon into pr/Silicon-Discretization
veprbl
veprbl approved these changes Apr 9, 2025
View reviewed changes
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.

LGTM

@veprbl veprbl enabled auto-merge April 9, 2025 05:26
@veprbl veprbl added this pull request to the merge queue Apr 9, 2025
Merged via the queue into main with commit 38c39d9 Apr 9, 2025
89 checks passed
@veprbl veprbl deleted the pr/Silicon-Discretization branch April 9, 2025 05:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veprbl veprbl veprbl approved these changes

Assignees
No one assigned
Labels
topic: barrel topic: digitization topic: infrastructure topic: PID Relates to PID reconstruction
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ssedd1123 @simonge @veprbl
0