SpikeInterface / spikeinterface

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

`SortingAnalyzer` can't handle probegroup due to sparsity estimation #2503

Closed zm711 closed 8 months ago

zm711 commented 8 months ago

@samuelgarcia was testing the sorting_analyzer today, but on the very first step it was giving me this error. The WaveformExtractor used to always handle this automatically for me. I pretty much always do probegroups these days, but I'll see if I can find a dataset with just a probe to test.

sorting_analyzer = si.create_sorting_analyzer(sorting, rec_opto)
Traceback (most recent call last):

  Cell In[37], line 1
    sorting_analyzer = si.create_sorting_analyzer(sorting, rec_opto)

  File ~\Documents\GitHub\spikeinterface\src\spikeinterface\core\sortinganalyzer.py:107 in create_sorting_analyzer
    sparsity = estimate_sparsity(recording, sorting, **sparsity_kwargs)

  File ~\Documents\GitHub\spikeinterface\src\spikeinterface\core\sparsity.py:593 in estimate_sparsity
    len(recording.get_probes()) == 1

AssertionError: The 'radius' method of `estimate_sparsity()` can handle only one probe

Or is it now the responsibility of the user for this management?

zm711 commented 8 months ago

I can create if I use sparse=False.

alejoe91 commented 8 months ago

Hi Zach! I'll take a look at this tomorrow :) Thanks for testing!!

zm711 commented 8 months ago

Sounds good Alessio. I'll keep testing on my end when I can. :)

alejoe91 commented 8 months ago

@zm711

So the problem is in the compute_sparsity function, which needs a Templates object or a SortingAnalyzer as input.

The Templates object can only accept a single Probe, with the assumption that you wouldn't/shouldn't compute templates spanning multiple probes.

I think that the solution could be extending the function to accept a list of Templates objects, so that the estimate_sparsity function would loop through the probes, slice the template averages, and instantiate a list of Templates objecte here.

Alternatively, we could be less strict on the Templates creation an accept a probe_or_probegroup.

What do you think would be the best solution here? @samuelgarcia

Happy to make a PR for it :)

zm711 commented 8 months ago

This makes perfect sense different probes within a probegroup should be treated separately for sparsity. Maybe @JoeZiminski and I are the only ones doing a lot of sorting by group with different probegroups, but it would be super nice to just automatically handle it :)

samuelgarcia commented 8 months ago

@alejoe91 : Templates with multiple probes will be a total nightmare to handle. I really want to avoid it. I prefer to handle this directly in sparsity computation. Lets find something. recording with multiple probes is OK.

SortingAnalyzer with multiple probe is complicated. Templates with multiple probe is a bad idea I think.

zm711 commented 8 months ago

I'm trying to understand the potential plan. So,

I agree Template with multiple probes is bad. Although technically you could have probes close together in which case they could see the same spikes (e.g. if someone made shanks into probes and put them in a probegroup--just depends on how close I guess).

OR

samuelgarcia commented 8 months ago
  • But is the hope to allow SortingAnalyzer handle probegroups (ie put the strain on the developers)

Normaly SortingAnalyzer already handle probegroups but we could have corner bugs.

zm711 commented 8 months ago

I don't count this as a corner issue. For me not having sparsity makes all the widgets for templates and waveforms unusable. And it will slowdown the calculations (although they are still pretty quick so kudos). For me I would stick with WaveformExtractor until sparsity is worked out.

samuelgarcia commented 8 months ago

I did a simple fix also in #2514. Can you check this is now working ?

zm711 commented 8 months ago

Yeah I'll check when I get to lab and let you know later today :)