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

Update multi scatter Ignore nan in the sum of peaks. #1162

Merged
merged 20 commits into from May 11, 2023
Merged

Conversation

michaweiss89
Copy link
Contributor

Before you submit this PR: make sure to put all operations-related information in a wiki-note, a PR should be about code and is publicly accessible

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

It ignores nan in the sum.

Can you briefly describe how it works?

It ignores nan in the sum.

Can you give a minimal working example (or illustrate with a figure)?

Same as before

considerations._

All italic comments can be removed from this template.

straxen/plugins/events/multi_scatter.py Outdated Show resolved Hide resolved
straxen/plugins/events/multi_scatter.py Outdated Show resolved Hide resolved
straxen/plugins/events/multi_scatter.py Outdated Show resolved Hide resolved
straxen/plugins/events/multi_scatter.py Outdated Show resolved Hide resolved
straxen/plugins/events/multi_scatter.py Outdated Show resolved Hide resolved
straxen/plugins/events/multi_scatter.py Outdated Show resolved Hide resolved
straxen/plugins/events/multi_scatter.py Outdated Show resolved Hide resolved
straxen/plugins/events/multi_scatter.py Outdated Show resolved Hide resolved
straxen/plugins/events/multi_scatter.py Outdated Show resolved Hide resolved
straxen/plugins/events/multi_scatter.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Apr 25, 2023

Coverage Status

Coverage: 93.473% (+0.02%) from 93.448% when pulling 76a38c7 on MSevents_fixbug into 5fa734a on master.

@dachengx dachengx self-requested a review April 26, 2023 14:46
straxen/plugins/events/multi_scatter.py Outdated Show resolved Hide resolved
@dachengx
Copy link
Collaborator

dachengx commented May 8, 2023

@HenningSE I guess we are good to merge if we make some decisions on the FDC correction. : )

@dachengx
Copy link
Collaborator

Or I guess we can also implement the FDC-corrected z later.

@dachengx
Copy link
Collaborator

@HenningSE Do you think this PR is in good shape to merge? Or what are the ongoing or pending development?

@HenningSE
Copy link
Contributor

Hi @dachengx, sorry I think I missed your question a few days ago. As far as I can tell, the FDC correction was basically copy and paste from the existing straxen code.

From my perspective the PR can be merged! Thanks for you help!

@dachengx
Copy link
Collaborator

dachengx commented May 11, 2023

Hi @dachengx, sorry I think I missed your question a few days ago. As far as I can tell, the FDC correction was basically copy and paste from the existing straxen code.

From my perspective the PR can be merged! Thanks for you help!

Hey @HenningSE . The z_obs_ms in peak_corrections.py is only corrected by electron_drift_time_gate. So the FDC correction is not done. But anyway we can do it later and I believe this PR was meant to fix bugs.

I will merge it after the test passes.

@dachengx dachengx merged commit 0aeb683 into master May 11, 2023
7 checks passed
@dachengx dachengx deleted the MSevents_fixbug branch May 11, 2023 15:47
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