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

Add option to save first samples of peak(lets) waveform #867

Merged
merged 21 commits into from
Oct 11, 2024

Conversation

HenningSE
Copy link
Collaborator

@HenningSE HenningSE commented Aug 15, 2024

What is the problem / what does the code in this PR do

This PR adds the option to save the first n (default to 200) samples of the merged peak(lets) waveform without downsampling. In case of very wide S1s or S1s with a lot of afterpulses strax will downsample the peak waveforms to fit into the 200 sample summed waveform that we save with peaklets and peaks. This process removes important information of the S1 waveform. With this PR the first n samples of the waveforms are saved for peaklets that 1. get downsampled and 2. have a downsampling factor smaller 7. This should cover basically all S1s without introducing a large file size overhead.

@coveralls
Copy link

coveralls commented Aug 15, 2024

Coverage Status

coverage: 90.264% (-0.04%) from 90.302%
when pulling 1467463 on add_waveform_start
into 5360ab3 on master.

@HenningSE HenningSE marked this pull request as ready for review August 15, 2024 14:58
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.

Thanks, @HenningSE . The PR looks good I would add comments about this artificial threshold 6 of downsample_factor.

strax/processing/peak_building.py Outdated Show resolved Hide resolved
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.

Thanks @HenningSE . There is one missing argument.

strax/processing/peak_merging.py Outdated Show resolved Hide resolved
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.

Thanks @HenningSE . If you are sure this PR works well with your proposed change in straxen, please merge it after autotest finishes.

@HenningSE
Copy link
Collaborator Author

Thanks @dachengx, it looks like I'm not an authorized user and can not merge.

@dachengx dachengx merged commit a528e4b into master Oct 11, 2024
8 checks passed
@dachengx dachengx deleted the add_waveform_start branch October 11, 2024 13:11
p["data_start"] = wv_buffer[: len(p["data_start"])]
else:
p["data_start"][: p["length"]] = wv_buffer[: p["length"]]

else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe here you should also assign data_start.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @dachengx, I think this case is not needed as we do not downsample the waveform. The data field contains all needed information.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But if you look at data_start, it will be zeros or np.nan. People might misunderstand it.

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