Spike location with true channel #1950
Conversation
|
I'll test asap. |
| params = self._params.copy() | ||
| channel_from_template = params.pop("channel_from_template") | ||
|
|
||
| # @alessio @pierre: where do we expose the parameters of radius for the retriever (this is not the same as the one for locatization it is smaller) ??? |
There was a problem hiding this comment.
I would expose it in the compute_spike_location function, otherwise this has no sense. In addition, to ensure comparison with/witout this extra mecanism, we should make sure that if radius_um is set to 0, a classical PeakRetriever is used here
There was a problem hiding this comment.
PeakRetriever will be not used here this is postprocessing.
| params.update(**method_kwargs) | ||
| print(params) |
| self.channel_from_template = channel_from_template | ||
|
|
||
| assert extremum_channel_inds is not None, "SpikeRetriever need the dict extremum_channel_inds" | ||
|
|
There was a problem hiding this comment.
Why the SpikeRetriever is not internally, automatically, getting the extremum_channel_inds? This seems like a useless argument no? You have use cases where users would like to provide their own extremum channels?
There was a problem hiding this comment.
no the instanciation has to be fast because it duplicate on several process this is external when building the node graph
| @@ -95,12 +116,16 @@ def get_extension_function(): | |||
|
|
|||
| WaveformExtractor.register_extension(SpikeLocationsCalculator) | |||
|
|
|||
| # @alessio @pierre: channel_from_template=True is the old behavior but this is not accurate | |||
| # what do we put by default ? | |||
|
|
|||
There was a problem hiding this comment.
I would go for the new behavior as a default, but we need to think on the impact with the metrics
| channel_from_template=channel_from_template, | ||
| extremum_channel_inds=extremum_channel_inds, | ||
| radius_um=50, | ||
| peak_sign=self._params.get("peaks_sign", "neg"), |
There was a problem hiding this comment.
This neg should not be hardcoded, and peak_sign should be an argument of compute_spike_locations
|
We need to provide a value for peaks['amplitudes'], otherwise some localizaiotn methods are not working. I propose to add |
…arcia/spikeinterface into spike_location_with_true_channel
|
@samuelgarcia can we proceed with this PR such that I can wrap up our next paper ? |
…spike_location_with_true_channel
for more information, see https://pre-commit.ci
|
@yger : I made some update on this. Can you test this ? |
…arcia/spikeinterface into spike_location_with_true_channel
for more information, see https://pre-commit.ci
|
@samuelgarcia some tests are still failing, waiting for merging |
for more information, see https://pre-commit.ci
|
@alejoe91 I did a fix. |
@alejoe91 @yger : there are some question in the code for you
@yger: can you test this for the localization benchmark ?