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` fails when trying to compute waveforms. #2504

Closed zm711 closed 8 months ago

zm711 commented 8 months ago

@samuelgarcia, I'm on the current dev branch. What does this mean that I should run select_random_spikes. The waveform extension says it depends on nothing, but then this error occurs?

sorting_analyzer.compute(input='waveforms', save=False)
Traceback (most recent call last):

  Cell In[42], line 1
    sorting_analyzer.compute(input='waveforms', save=False)

  File ~\Documents\GitHub\spikeinterface\src\spikeinterface\core\sortinganalyzer.py:793 in compute
    return self.compute_one_extension(extension_name=input, save=save, **kwargs)

  File ~\Documents\GitHub\spikeinterface\src\spikeinterface\core\sortinganalyzer.py:854 in compute_one_extension
    extension_instance.run(save=save, **job_kwargs)

  File ~\Documents\GitHub\spikeinterface\src\spikeinterface\core\sortinganalyzer.py:1376 in run
    self._run(**kwargs)

  File ~\Documents\GitHub\spikeinterface\src\spikeinterface\core\analyzer_extension_core.py:45 in _run
    raise ValueError("compute_waveforms need SortingAnalyzer.select_random_spikes() need to be run first")

ValueError: compute_waveforms need SortingAnalyzer.select_random_spikes() need to be run first

I just set sparse=False during my setup so that I could try to compute waveforms from #2503.

alejoe91 commented 8 months ago

Thanks Zach!

The select_random_spikes is supposed to give you control on how the spikes are selected. It's not an extension per se. I agree qith you that if an extension needs this, it should compute it by default on the fly (similarly to what the old waveform extarctor did). @samuelgarcia what do you think?

zm711 commented 8 months ago

I think that level of control is great for a subset of power users. I definitely think the average user would just be fine to let the defaults be used. So maybe a kwarg dict to control random spikes if desired (or the person can pre-run for even finer control). But that would just be my two cents.

samuelgarcia commented 8 months ago

@alejoe91 : no. I really want that every thing is explicit and not hidden somewhere in this new API. Because if we compute on the fly then the parameters of selection will not be controlled.

I plan to make an help function that make a few steps like this which is more or less the extract_waveforms() we use to have. So it will avoid this kind of question for users.

job_kwargs = dict(n_jobs=-1, progress_bar=True, chunk_duration="1s")

sorting_analyzer = si.create_sorting_analyzer(sorting, recording,
                                              format="binary_folder", folder="/my_sorting_analyzer",
                                              **job_kwargs)
sorting_analyzer.select_random_spikes(method="uniform", max_spikes_per_unit=500)
sorting_analyzer.compute("waveforms", **job_kwargs)
sorting_analyzer.compute("templates")
sorting_analyzer.compute("noise_levels")
zm711 commented 8 months ago

Hey Sam then why not sorting_analyzer.compute('random_spikes',....)

It seems a little strange that I can do everything except the first step using compute, no?

samuelgarcia commented 8 months ago

Hey Sam then why not sorting_analyzer.compute('random_spikes',....)

This is eally a good point. I though about it was a bit too late in the design. Then I convince myself that selected spikes were a special case but you are right this should be a extension like others and then the depency will be more clear! I was too lazy to change the code when I realized it but if you discover it at the fiorst read then it need to be done this way. I think we should do this before dev go to main. Need some work but this is not too big.

thanks a lot for this crash test.

samuelgarcia commented 8 months ago

Done in #2514