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 plugin to get data of multiple peaks at event level #1382

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

HenningSE
Copy link
Contributor

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

This PR adds a plugin that propagates multiple peak properties to event level.

Please include the following if applicable:

  • Update the docstring(s)
  • Update the documentation
  • Tests to check the (new) code is working as desired.
  • Does it solve one of the open issues on github?

@coveralls
Copy link

coveralls commented May 16, 2024

Coverage Status

coverage: 89.757% (-0.5%) from 90.215%
when pulling 7e49388 on multi_peak
into 1f80838 on master.

@yuema137 yuema137 self-requested a review June 4, 2024 17:59
@HenningSE
Copy link
Contributor Author

Need to wait for AxFoundation/strax#867 and #1406

@yuema137
Copy link
Collaborator

yuema137 commented Oct 17, 2024

Hi, @HenningSE, thanks for this high-level PR. On top of the small comments that I made for the input parameters and docstring, I have a more general one - if we really need such a plugin here?

This plugin reads the relative time, position, and AFT of every peak within events. For a plugin named multiple_peak_data, other users may expect to get more information about those peaks but not just a few selected features. So I would suggest moving it to the analyses module if it's something needed for certain analyses. Basically, we just need to put a function to read from peaks and events data.

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.

3 participants