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 top channel at peak_basics and above #1270

Closed
wants to merge 5 commits into from
Closed

Conversation

shenyangshi
Copy link
Contributor

Add n_top_channel to let s1 bdt get rid of the peaks computational-expensive area_per_channel dependence.

Resolve #1263.

@shenyangshi shenyangshi marked this pull request as ready for review September 25, 2023 12:43
@shenyangshi
Copy link
Contributor Author

image image

straxen/plugins/peaks/peak_basics.py Outdated Show resolved Hide resolved
straxen/plugins/peaks/peak_basics.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Sep 25, 2023

Coverage Status

coverage: 92.691% (+0.02%) from 92.669% when pulling da8a79b on add_channel into 11cdb5c on master.

@shenyangshi
Copy link
Contributor Author

@dachengx Do you think this is good to merge?

@dachengx
Copy link
Collaborator

@dachengx Do you think this is good to merge?

I suggest putting the new fields in another plugin. Because the already processed data will be affected.

@shenyangshi
Copy link
Contributor Author

We'll start the global_v13 reprocessing soon, how about only merging this PR before releasing the new container? @jingqiangye

@dachengx
Copy link
Collaborator

We'll start the global_v13 reprocessing soon, how about only merging this PR before releasing the new container? @jingqiangye

Please discuss those in other place than public platform.

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.

Add top_n_channels and bottom_n_channels at the peak level
3 participants