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

Save first samples of peak(lets) waveform #1406

Merged
merged 22 commits into from
Oct 16, 2024
Merged

Save first samples of peak(lets) waveform #1406

merged 22 commits into from
Oct 16, 2024

Conversation

HenningSE
Copy link
Contributor

@HenningSE HenningSE commented Aug 15, 2024

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

This PR uses the code changes in AxFoundation/strax#867 to save the first n_sum_wv_samples of not-downsampled waveforms.

@coveralls
Copy link

coveralls commented Oct 14, 2024

Coverage Status

coverage: 90.168% (-0.08%) from 90.252%
when pulling 2f1a6af on add_waveform_start
into c2c0400 on master.

@HenningSE HenningSE marked this pull request as ready for review October 14, 2024 10:46
Copy link
Collaborator

@dachengx dachengx left a comment

Choose a reason for hiding this comment

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

Do we also need to add lone_hits to the start of waveform like

strax.add_lone_hits(merged_s2s, lh, self.to_pe, n_top_channels=n_top_pmts_if_digitize_top)
?

straxen/plugins/merged_s2s/merged_s2s.py Outdated Show resolved Hide resolved
straxen/plugins/merged_s2s/merged_s2s.py Outdated Show resolved Hide resolved
straxen/plugins/peaklets/peaklets.py Outdated Show resolved Hide resolved
@HenningSE
Copy link
Contributor Author

Do we also need to add lone_hits to the start of waveform like

strax.add_lone_hits(merged_s2s, lh, self.to_pe, n_top_channels=n_top_pmts_if_digitize_top)

?

Thanks @dachengx, yes we should add it! When checking the corresponding code I discovered a bug in strax that we should also fix: AxFoundation/strax#906

@HenningSE
Copy link
Contributor Author

Needs AxFoundation/strax#908 to merge lone hits into the start of the waveforms

@yuema137
Copy link
Collaborator

yuema137 commented Oct 15, 2024

Thanks @HenningSE ! On top of @dachengx's suggestions, I have a few comments

  • The autotest would only work with Add option to merge lone_hits into data_start AxFoundation/strax#908 (that one looks good to me, and I already approved.), so please rerun the GitHub action after that PR is merged.
  • Sorry that I may miss the context of this PR - is it widely needed by general analysis? I'm wondering if it's necessary to set the default value of save_waveform_start to True As @dachengx mentioned, we always want to compute the low-level plugins at once.

@dachengx
Copy link
Collaborator

Thanks @HenningSE ! On top of @dachengx's suggestions, I have a few comments

  • The autotest would only work with Add option to merge LHs into data_start AxFoundation/strax#908 (that one looks good to me, and I already approved.), so please rerun the GitHub action after that PR is merged.
  • Sorry that I may miss the context of this PR - is it widely needed by general analysis? I'm wondering if it's necessary to set the default value of save_waveform_start to be True

We can not process peaklets twice. So we'd better same everything during that only round of processing.

dachengx
dachengx previously approved these changes Oct 16, 2024
@dachengx dachengx merged commit 3b7d54a into master Oct 16, 2024
5 of 8 checks passed
@dachengx dachengx deleted the add_waveform_start branch October 16, 2024 21:00
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.

4 participants