Skip to content

nodepipeline : skip chunks when no peaks inside and skip_after_n_peaks#3356

Merged
yger merged 9 commits intoSpikeInterface:mainfrom
samuelgarcia:node_pipeline_skip_no_peaks
Oct 7, 2024
Merged

nodepipeline : skip chunks when no peaks inside and skip_after_n_peaks#3356
yger merged 9 commits intoSpikeInterface:mainfrom
samuelgarcia:node_pipeline_skip_no_peaks

Conversation

@samuelgarcia
Copy link
Copy Markdown
Member

As in title

@yger : this is a revival of #2011 but with new gather and avoid double searchsorted

Copy link
Copy Markdown
Member

@zm711 zm711 left a 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.

@alejoe91 alejoe91 added the core Changes to core module label Aug 31, 2024
@alejoe91
Copy link
Copy Markdown
Member

alejoe91 commented Sep 5, 2024

pre-commit? :)

@samuelgarcia samuelgarcia changed the title nodepipeline : skip chunks when no peaks inside nodepipeline : skip chunks when no peaks inside and skip_after_n_peaks Sep 6, 2024
@samuelgarcia
Copy link
Copy Markdown
Member Author

@zm711 : I push some doc tell if you want more

Copy link
Copy Markdown
Member

@zm711 zm711 left a 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.

job_kwargs,
job_name="pipeline",
mp_context=None,
#mp_context=None,
Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member Author

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 ?

Copy link
Copy Markdown
Member

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

Comment thread src/spikeinterface/core/node_pipeline.py Outdated
Comment thread src/spikeinterface/core/node_pipeline.py Outdated

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, ...)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

replay?

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member

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.

Comment on lines +494 to +495
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.
Copy link
Copy Markdown
Member

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!

Comment thread src/spikeinterface/core/node_pipeline.py Outdated
Comment thread src/spikeinterface/core/node_pipeline.py Outdated
Comment thread src/spikeinterface/core/node_pipeline.py Outdated
Comment thread src/spikeinterface/core/node_pipeline.py Outdated
Comment thread src/spikeinterface/core/tests/test_node_pipeline.py Outdated
Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
Copy link
Copy Markdown
Member

@alejoe91 alejoe91 left a comment

Choose a reason for hiding this comment

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

Reformat please :)

@alejoe91 alejoe91 modified the milestone: 0.101.2 Sep 25, 2024
Copy link
Copy Markdown
Collaborator

@yger yger left a comment

Choose a reason for hiding this comment

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

LGTM

@yger yger merged commit 69bf6e4 into SpikeInterface:main Oct 7, 2024
@samuelgarcia samuelgarcia deleted the node_pipeline_skip_no_peaks branch July 29, 2025 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Changes to core module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants