SpikeInterface / spikeinterface

A Python-based module for creating flexible and robust spike sorting pipelines.
https://spikeinterface.readthedocs.io
MIT License
524 stars 187 forks source link

Extension of peak localization to custom pipelines #1487

Closed yger closed 1 year ago

yger commented 1 year ago

It would be good to have a way to extend the localize_peaks function in order to deal with custom pipelines. Ideally, what would you think of something like localize_peaks(....extra_nodes=None, ...)

If extra_nodes is None, then we simply create a DenseWaveformExtractor nodes, and localize from it. Otherwise, ideally one would like to be able to pass its own pipeline chains, that might involve denoising/funky operations on the waveforms. For example, this morning I want to compare localization with/without savgol denoiser in the middle, and this is currently not straightforward at all using default functions. I think this is a pity...

alejoe91 commented 1 year ago

Can't you use the run_peak_pipeline() directly? https://github.com/SpikeInterface/spikeinterface/blob/master/spikeinterface/sortingcomponents/peak_pipeline.py#L231

That's exactly for this purpose :)

alejoe91 commented 1 year ago

See example in the tests here: https://github.com/SpikeInterface/spikeinterface/blob/master/spikeinterface/sortingcomponents/tests/test_peak_pipeline.py#L88-L108

yger commented 1 year ago

Yes of course, I know. But I think that having something more integrated could have been better. Maybe I'm wrong let me know what you guys think. Like at the level of compute_spike_locations or as said localize_peaks. You would like to provide a pipeline and use these functions out of the box. Because otherwise, this is a bit more for advanced users.

alejoe91 commented 1 year ago

Imo the sorting components API is for advanced users. Meaning that it's ok to have a few extra lines of code (should be well documented) to enable full flexibility. What do @samuelgarcia @h-mayorquin and @pauladkisson think?

samuelgarcia commented 1 year ago

I am doing another big refactoring #1486 The run_peak_pipeline() should be replace by run_pipeline()

In my mind I would not propose extra nodes in localize_peaks() in api but rather than intuitive kwargs that internally make moe complicated pipeline like use_denoise=True/False

alejoe91 commented 1 year ago

This is done and now in core: https://github.com/SpikeInterface/spikeinterface/blob/main/src/spikeinterface/core/node_pipeline.py