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

Option to give range to sum_waveform #322

Merged
merged 7 commits into from
Oct 8, 2020
Merged

Option to give range to sum_waveform #322

merged 7 commits into from
Oct 8, 2020

Conversation

zhut19
Copy link
Contributor

@zhut19 zhut19 commented Oct 7, 2020

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

The desaturation requires re-calculating the sum waveform of only a small fraction of the peaklets (that are saturated), once the (temporary copy of) records are corrected. It is best to be able to do partial re-calculation.

Can you briefly describe how it works?

func: sum_waveform now takes a keyword argument peak_list of the type numpy.array. When not specified, default value would be converted into np.arange(len(peaks))

@JoranAngevaare
Copy link
Contributor

JoranAngevaare commented Oct 7, 2020

Hi @zhut19 , thanks for the PR, having the desaturation correction in strax will be most helpfull.

In this PR, the style seems very convoluted. Can you explain why you need -3 and -14? I think you can more easily and clearly do this with a simple None kwarg?

By the way, travis is failing untill we merge #321. Don't worry about that too much.

@zhut19
Copy link
Contributor Author

zhut19 commented Oct 7, 2020

Thanks, @jorana. I wasn't thinking. The keyword argument was one way to keep compatibility with the original code, a silly one tbh. With the switching to *arg in 6aeb711, is this good for you?

Copy link
Contributor

@JoranAngevaare JoranAngevaare left a comment

Choose a reason for hiding this comment

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

Sure, would work! But if at all possible I'd be a little more explicit, would you like 6275049? Otherwise I can revert it?

An empty list should be allowed, it just means no peak needs to apply sum waveform to.
@zhut19
Copy link
Contributor Author

zhut19 commented Oct 7, 2020

This is even better! I remove the raise error, as an empty list is okay and does exist.

@JoranAngevaare
Copy link
Contributor

Sure thanks, I was about to remove that raise statement for exactly the reason you mention. Just for completeness I've added this to the tests in f92086f

@JoranAngevaare JoranAngevaare merged commit 0087ed5 into AxFoundation:master Oct 8, 2020
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.

2 participants