Skip to content
This repository has been archived by the owner on Jun 5, 2024. It is now read-only.

Double PMT ap probability for photons emitting double PE. #340

Merged
merged 10 commits into from
Apr 5, 2022
Merged

Conversation

zhut19
Copy link
Contributor

@zhut19 zhut19 commented Mar 28, 2022

What is the problem / what does the code in this PR do
The double photo-electron in PMTs also contributes to generating PMT afterpulse, in this pr, we store a boolean array noting photons emitting dpe, in the last step of generating S1s and S2s. While we double the PMT AP probability for those photons when generating PMT afterpulses.

@coveralls
Copy link

coveralls commented Mar 28, 2022

Coverage Status

Coverage decreased (-0.02%) to 81.459% when pulling 182c9fd on ap_by_dpe into f917fb5 on master.

@zhut19 zhut19 marked this pull request as ready for review March 28, 2022 14:43
ramirezdiego
ramirezdiego previously approved these changes Mar 28, 2022
@ramirezdiego ramirezdiego self-requested a review March 28, 2022 15:38
@ramirezdiego ramirezdiego dismissed their stale review March 28, 2022 15:39

Pending syntax fix

@zhut19 zhut19 requested a review from terliuk March 29, 2022 13:32
@zhut19 zhut19 requested a review from hoetzsch March 31, 2022 14:20
@hoetzsch
Copy link

hoetzsch commented Apr 4, 2022

Hey @zhut19,

thanks for the fix, now the population with the additional delay is gone!
The first plot below shows the ratio of the He afterpulse rates I get from the wfsim data after and before the DPE fix, and it turns out to be close to the dpe probability, so this seems to work well.

wfsimaps_ratioafterbeforedpe

I still can't quite reproduce the afterpulse rates from the input file though, but the discrepancy has decreased significantly with the DPE addition, from -33% to -18%, shown in the second plot. Still not sure where this comes from, could be pile-up. But I don't think it's related to the DPE thing.

wfsimaps_comptoinput

Copy link
Contributor

@terliuk terliuk left a comment

Choose a reason for hiding this comment

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

It seems like it does what this PR is supposed to do, so let's merge it and figure out why there is still remaining mismatch separately.

Copy link
Collaborator

@ramirezdiego ramirezdiego left a comment

Choose a reason for hiding this comment

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

Agreed

@terliuk terliuk merged commit 8c58d16 into master Apr 5, 2022
@terliuk terliuk deleted the ap_by_dpe branch April 5, 2022 09:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants