Try to remove get_template_extremum_channel() and peak_sign in many place by samuelgarcia · Pull Request #4374 · SpikeInterface/spikeinterface

samuelgarcia · 2026-02-06T16:06:22Z

This PR try to

change semantic extremum_channel to main_channel
remove get_template_extremum_channel() to use instead
- analyzer.get_main_channel() which is now a property
- templates.get_main_channel() which compute on the fly the old behavior
remove the peak_sign when it was use to get the main channel
use explicitly main_channel_peak_sign instead of peak_sign when it is for main channel
keep peak_sig when it makes sens, like peak detection or in sorter context.

The main consequences:

an analyzer is constructed with main_channel_index. The sparsity is estimated by default on it. This will be faster when
it is provided
The analyzer has always via the sorting object acces to this property so all metrics will consistently depend on it.

…tings to reload them

ecobost

Hi @samuelgarcia Thanks for taking time to work in this; I checked the code from analyzer-> metrics->widgets (anything before analyzer I don't really know).

After going through the code, the only extensions that still receive peak_sign are spike_amplitudes, quality metrics and template_metrics; and that argument can be dropped from all of them (in xxxxxx_metrics is not used at all, while in spike_amplitudes is used once but could be replaced with analyzer.main_channel_peak_sign). Given that, I would vote to rename main_channel_peak_sign to just peak_sign in the sorting analyzer (and essentially use it as the global peak_sign that will be used if any extension needs it, similar to sparsity), including ofc to find the main_channel.

In theory, main_channel_peak_mode falls in the same camp but in truth that seems to only be used to pick the main_channel so main_channel_peak_mode as name might be ok.

ecobost · 2026-02-07T01:19:36Z

src/spikeinterface/core/sortinganalyzer.py

    overwrite=False,
    backend_options=None,
-    **sparsity_kwargs,
+    sparsity_kwargs=None,


This argument could be dropped. It's not the responsibility of this function to create all types of sparsity. You could either provide an Sparsity object or let create_sorting_analyzer create it with the default method (from_radius_and_main_channel_index). I feel like that's fair enough (it's more of a convenience) and simplifies the sparsity logic here (curently there are three or four different ways to get sparsity). It also avoids some possible inconsistencies (like main_channel_peak_sign != sparsity_kwargs['peak_sign']) or num_spikes_for_main_channel vs sparsity_kwargs['num_spikes_for_sparsity']

The one common use case that will be affected is people that want to create a sorting analyzer with a bigger or smaller radius (the only sparsity_kwargs that would affect the radius_um method + main_index); but maybe they'll just have to estimate sparsity explicitly (and feed it here).

ecobost · 2026-02-07T01:34:27Z

src/spikeinterface/core/sparsity.py

+        )
+
+    elif method != "by_property":
        nbefore = int(ms_before * recording.sampling_frequency / 1000.0)


This code (L.816-849) is a duplicate of template_tools.estimate_main_channel_from_recording (used in create_sorting_analyzer to find the main_channel). If it's too bothersome to unify both pieces of code somehow, maybe we can at least refactor so radius method here uses estimate_main_channel_from_recording so we can guarantee that:

sparsity = create_sparsity(peak_sign=my_peak_sign, num_spikes_for_sparsity=my_num_spikes) analyzer1 = create_sorting_analyzer(sparsity=sparsity) analyzer2 = create_sorting_analyzer(main_channel_peak_sign=my_peak_sign, num_spikes_for_main_channel=my_num_spikes) assert analyzer1.sparsity == analyzer2.sparsity

Essentially refactor it as

if method== radius: if main_channel_index is None: main_channel_index = estimate_main_channel_from_recording(...) sparsity = .... from_radius_and_main_channel(...) elif method != 'property' # the repeated code here (any changes won't affect radius method)

ecobost · 2026-02-07T01:36:25Z

src/spikeinterface/core/sparsity.py

            sparsity = ChannelSparsity.from_closest_channels(templates, num_channels)
        elif method == "radius":
            assert radius_um is not None, "For the 'radius' method, 'radius_um' needs to be given"
            sparsity = ChannelSparsity.from_radius(templates, radius_um, peak_sign=peak_sign)


this does not receive peak_sign anymore

ecobost · 2026-02-07T01:37:25Z

src/spikeinterface/core/sparsity.py

        sparsity = ChannelSparsity.from_closest_channels(templates_or_sorting_analyzer, num_channels)
    elif method == "radius":
        assert radius_um is not None, "For the 'radius' method, 'radius_um' needs to be given"
        sparsity = ChannelSparsity.from_radius(templates_or_sorting_analyzer, radius_um, peak_sign=peak_sign)


from_radius does not receive peak_sign anymore

ecobost · 2026-02-07T01:39:52Z

src/spikeinterface/core/sparsity.py

    ms_before: float = 1.0,
    ms_after: float = 2.5,
    method: "radius" | "best_channels" | "closest_channels" | "amplitude" | "snr" | "by_property" = "radius",
    peak_sign: "neg" | "pos" | "both" = "neg",


consider changing default to "both" to agree with default in create_sorting_analyzer (main_channel_peak_sign)

ecobost · 2026-02-07T02:09:41Z

src/spikeinterface/postprocessing/localization_tools.py


    if sorting_analyzer_or_templates.sparsity is None:
        sparsity = compute_sparsity(
            sorting_analyzer_or_templates, peak_sign=peak_sign, method="radius", radius_um=radius_um


peak_sign is no longer used for method radius ((compute_sparsity uses analyzer/templates.get_main_channels) so not needed here. Can also be dropped as an argument (only was used in this line)

ecobost · 2026-02-07T02:10:44Z

src/spikeinterface/postprocessing/localization_tools.py

    unit_ids: str | int | None
        A list of unit_id to restrci the computation
    peak_sign : "neg" | "pos" | "both", default: "neg"
        Sign of the template to compute best channels


not true anymore; now it is only used to invert the gaussian prototype template (in l. 275); I think peak_sign='neg' default makes sense in this case

ecobost · 2026-02-07T02:11:35Z

src/spikeinterface/metrics/template/template_metrics.py


        if unit_ids is None:
            unit_ids = sorting_analyzer.unit_ids
        peak_sign = self.params["peak_sign"]


not used anywhere

ecobost · 2026-02-07T02:12:47Z

src/spikeinterface/metrics/template/template_metrics.py

        metrics_to_compute: list[str] | None = None,
        periods=None,
        # common extension kwargs
        peak_sign="neg",


In theory you can drop peak_sign (it is no longer used anywhere here); in practice, it will be used in the bombcell PR (and probably by Alessio/Chris when they re-work the template metrics) but perhaps rather than as an argument, it can be read from analyzer

ecobost · 2026-02-07T02:13:40Z

src/spikeinterface/metrics/quality/quality_metrics.py

        use_valid_periods=False,
        periods=None,
        # common extension kwargs
        peak_sign=None,


this is no longer needed (it was only used for the extremum_channel in the PCA metrics)

ecobost · 2026-02-07T02:33:16Z

Also, given that you are touching this code anyway, do you mind sending peak_sign here too

spikeinterface/src/spikeinterface/curation/remove_redundant.py

Line 84 in 25940b4

unit_peak_shifts = get_template_extremum_channel_peak_shift(sorting_analyzer)

I don't mean to make it work with the analyzer (although maybe it makes sense to forward the analyzer.main_channel_peak_sign here too) but just to forward the peak_sign argument like

unit_peak_shifts = get_template_extremum_channel_peak_shift(sorting_analyzer, peak_sign=peak_sign)

thanks 👍🏽

ecobost · 2026-02-08T04:01:38Z

src/spikeinterface/core/template.py

+        templates_array = self.get_dense_templates()
+        main_channel_index = _get_main_channel_from_template_array(templates_array, mode, main_channel_peak_sign, self.nbefore)
+
+        if outputs is "index":


this should use == (rather than is)

ecobost · 2026-02-08T04:01:46Z

src/spikeinterface/core/template.py

+
+        if outputs is "index":
+            main_chans = main_channel_index
+        elif outputs is "id":


this should use == (rather than is)

ecobost · 2026-02-08T04:02:21Z

src/spikeinterface/core/sortinganalyzer.py

+
+        """
+        main_channel_index = self.get_sorting_property("main_channel_index")
+        if outputs is "index":


this should use == (rather than is)

ecobost · 2026-02-08T04:02:28Z

src/spikeinterface/core/sortinganalyzer.py

+        main_channel_index = self.get_sorting_property("main_channel_index")
+        if outputs is "index":
+            main_chans = main_channel_index
+        elif outputs is "id":


this should use == (rather than is)

samuelgarcia added 2 commits February 6, 2026 16:59

Try to remove get_template_extremum_channel()

6dd7269

Put main_channel_peak_sign and main_channel_peak_mode in analyzer set…

eede722

…tings to reload them

ecobost reviewed Feb 7, 2026

View reviewed changes

ecobost reviewed Feb 8, 2026

View reviewed changes

Conversation

samuelgarcia commented Feb 6, 2026

Uh oh!

ecobost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecobost commented Feb 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants