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

Compute width again after saturation correction #542

Merged
merged 5 commits into from Jun 16, 2021

Conversation

zhut19
Copy link
Contributor

@zhut19 zhut19 commented Jun 9, 2021

What does the code in this PR do / what does it improve?

The peak width is now being re-computed after the saturation correction. Those peaks previously computed with smaller widths.

This will need AxFoundation/strax#465 merged first.

@skazama
Copy link
Contributor

skazama commented Jun 11, 2021

Thanks Tianyu for the PR. It looks fine once the AxFoundation/strax#465 is merged

@@ -316,6 +319,7 @@ def peak_saturation_correction(records, peaks, to_pe,
peaks[peak_i]['dt'] = dt

strax.sum_waveform(peaks, records, to_pe, peak_list)
return peak_list
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe makes sense to just do strax.compute_widths(peaklets, select_peaks_indicies=peak_list) here together with the sum_wf, also alleviates the need to return this variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

@zhut19 , did you see my suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry, I missed this. I couldn't do this because that strax.compute_width is not a numba function.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could use a numba.typed.List, but then you should check the impact on the performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zhut19 ah okay, yeah I see, then it's fine as far as I am concerned unless you feel like checking out Daniels suggestion

@zhut19 zhut19 marked this pull request as ready for review June 15, 2021 14:01
@@ -316,6 +319,7 @@ def peak_saturation_correction(records, peaks, to_pe,
peaks[peak_i]['dt'] = dt

strax.sum_waveform(peaks, records, to_pe, peak_list)
return peak_list
Copy link
Contributor

Choose a reason for hiding this comment

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

@zhut19 ah okay, yeah I see, then it's fine as far as I am concerned unless you feel like checking out Daniels suggestion

@zhut19
Copy link
Contributor Author

zhut19 commented Jun 15, 2021

@jorana Let's leave it like this now.

@JoranAngevaare JoranAngevaare merged commit c23664c into XENONnT:master Jun 16, 2021
@zhut19 zhut19 deleted the re_compute_width branch June 16, 2021 13:54
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.

None yet

4 participants