Change ComputeSpikeLocations to use only peak_sign

[ecobost]

Fixes #4320.

Instead of receiving spike_retriever_kwargs (which was not used anyway), receive peak_sign (mirroring ComputeSpikeAmplitudes). Previous default behavior and capabilities will not change (as peak_sign was the only parameter actually used from spike_retriever_kwargs).

Code will change from

analyzer.compute('spike_locations', spike_retriver_kwargs={'peak_sign': 'both'}) # previous version
analyzer.compute('spike_locations', peak_sign='both') # new version

[ecobost]

Forgot to change references to spike_retriever_kwargs in other code/docs/test.

I found there is only one place where it was used:

spikeinterface/src/spikeinterface/benchmark/benchmark_peak_localization.py

Lines 27 to 30 in 5b2e738

	if not self.channel_from_template:
	self.params["spike_retriver_kwargs"] = {"channel_from_template": False}
	else:
	## TODO

As said in #4320, this didn't actually have any effect (cause spike_retriever_kwargs was not passed to SpikeRetriever()), so I commented this code out.I think spike_locations.py is cleaner/easier-to-understand receiving only peak_sign as argument but if you think the use case in benchmark_peak_localization.py warrants the extra complexity, let me know and I'll revert the changes (and make sure spike_retriever_kwargs is forwarded to the SpikeRetriever).

[samuelgarcia]

This was very important for our benchmark. Lets keep it!
We need the peak channel to be estimated for each peak to take in account the variability.
@yger

[ecobost]

I guess this is the reason that the spike_retriever_kwargs argument exists.

I am unsure what's the purpose of the Benchmark class, but finding the extremum channel on a single waveform (

spikeinterface/src/spikeinterface/core/node_pipeline.py

Line 241 in 679cb2e

sparse_wfs = traces[peak["sample_index"], chans]

) might be pretty noisy (it is limited to channels within 50 um of the original template extremum channel so maybe not so bad).

Also, you might need to check whether results change a lot; I actually doubt it because the spike_location method is still computed the same way as before just centered at a slightly different channel (the per-spike channel_index rather than the template channel_index.)

[samuelgarcia]

Thank you for this.

We need to be ver very carfull when changing parameters in extension because then when we reload an old analyzer from disck from previous version this will lead to bugs because the params are inconsistent.

In case we change parameters we need to ensure a backward compatibitility using th _handle_backward_compatibility_on_load() method. Also we try to change to often parameters in extension as less as we can for this reason.

In this particular I think we should keep the previous signature with no change and only ensure that this parameters is propagated correctly.

What do you think ?

[ecobost]

I'll reinstate spike_retriver_kwargs and make sure it is passed to SpikeRetriever. It's up to you guys whether you wanna rename it, personally, I think it looks sloppy to have a misspelled argument. Also, if you do fix the typo, it might make sense to also add a peak_sign argument anyway to keep it explicit and consistent with other extensions, like:

analyzer.compute("spike_amplitudes", peak_sign='both')
analyzer.compute('template_metrics', peak_sign='both')
analyzer.compute('spike_locations', peak_sign='both')

rather than using the peak_sign from spike_retriever_kwargs.

I am happy to add a commit for that too (with the _handle_backward_compatibility_on_load()), if you want

[yger]

I think that indeed, we should fix the typo, and maybe add the peak_sign option, as you are suggesting. In fact, you are right than in the benchmark, this is the only place where this spike_retriever params are used, because we need to detect the peaks of the spikes knowing the ground truth as they would have been detected without. This is why, when we search for peaks, once we have it at a proper place, we allow to search for other max in a certain spatio-temporal neighboorhood to capture the channel noise that might happen in reality, when detecting peaks. But agreed this is an advanced use case

[ecobost]

Ok, be02fbd has peak_sign as a diff argument, spike_retriever_kwargs fixed and backwards compatibiliy handled. I like this version.