Argument spike_retriver_kwargs (sic) is not sent to the SpikeRetriever in ComputeSpikeLocations.

[ecobost]

Smaller issue (typo): The argument in

spikeinterface/src/spikeinterface/postprocessing/spike_locations.py

Line 53 in 840c056

spike_retriver_kwargs=None,

should be spike_retriever_kwargs ("e" is missing).

Bigger issue: retriever_kwargs is not actually used during the initialization of the retriever (never was); the only kwarg used from it is peak_sign (to find the template extremum)

spikeinterface/src/spikeinterface/postprocessing/spike_locations.py

Lines 77 to 87 in 840c056

	peak_sign = self.params["spike_retriver_kwargs"]["peak_sign"]
	extremum_channels_indices = get_template_extremum_channel(
	self.sorting_analyzer, peak_sign=peak_sign, outputs="index"
	)
	retriever = SpikeRetriever(
	sorting,
	recording,
	channel_from_template=True,
	extremum_channel_inds=extremum_channels_indices,
	)

I'm happy to open a PR to fix it, just let me know whether:
a. You actually do wanna keep the retriever_kwargs and use it to init SpikeRetriever. I ask because ComputeSpikeAmplitudes receives only the peak_sign and calls SpikeRetriever with the default arguments so I wonder whether ComputeSpikeLocations should do the same

spikeinterface/src/spikeinterface/postprocessing/spike_amplitudes.py

Lines 43 to 45 in 840c056

	spike_retriever_node = SpikeRetriever(
	sorting, recording, channel_from_template=True, extremum_channel_inds=extremum_channels_indices
	)

or

b. You do wanna send the kwargs to SpikeRetriever() and are ok with renaming it to spike_retriever_kwargs (fixing the typo). Theoretically, it's a non-backwards-compatible change (people could be using the argument with the "retriver" name)

[alejoe91]

Thanks @ecobost!!
@samuelgarcia @yger what's your take?

[yger]

I think this is an advanced features, so I don't mind changing the name and fixing the typo. This is worth it I guess.

Regarding the fact that spike_retriever_kwargs is not fed to SpikeRetriever during the computations of the spike locations, I am not sure this is a bug. The idea was more that in this case, because we know the templates and the channel_indices of the peak, we really want to take the value of the samples centered at this particular channel_index. The node pipeline was more general and also had, if I recall correctly, the option to search for the best channel at a given time, but @samuelgarcia should comment on that

[samuelgarcia]

oups.
yes lets fix that
merci