-
Notifications
You must be signed in to change notification settings - Fork 190
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
nodepipeline : skip chunks when no peaks inside and skip_after_n_peaks #3356
nodepipeline : skip chunks when no peaks inside and skip_after_n_peaks #3356
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance we could start getting rudimentary docstrings? I've had to search through this to find a bug before and it was pretty painful. If we can start documenting a bit of the thought processes it will help us help people on the issue tracker :)
We can always edit them to make them tidy later. I'm really just asking for one liners for these functions.
pre-commit? :) |
…node_pipeline_skip_no_peaks
@zm711 : I push some doc tell if you want more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sam this is great. I really appreciate the work on this. :) Merci beaucoup.
@@ -467,31 +474,91 @@ def run_node_pipeline( | |||
nodes, | |||
job_kwargs, | |||
job_name="pipeline", | |||
mp_context=None, | |||
#mp_context=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we leave this for people that use positional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was a mistake because it should be in job_kwargs no ?
Alessio is it you that add this mp_context directly here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was you @samuelgarcia
|
||
This usefull in several use cases: | ||
* in sortingcomponents : detect peaks and make some computation on then (localize, pca, ...) | ||
* in sortingcomponents : replay some peaks and make some computation on then (localize, pca, ...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is like a replay of spikes by buffer. Maybe another term would be better I found this image more easy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless this is extremely technical I don't think we could quite say this. It sounds more like you are pulling some sound engineering terminology into the spike world. Like you're remixing your data analysis. I think I get the vibe, but I'm not sure of a better word. We can leave it for now and if I think of something I can put in a PR patch later.
Here a "peak" is a spike without any labels just a "detected". | ||
Here a "spike" is a spike with any a label so already sorted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is super helpful. Nice comment!
Co-authored-by: Zach McKenzie <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reformat please :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
As in title
@yger : this is a revival of #2011 but with new gather and avoid double searchsorted