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

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

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

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)

…less_peak_sign_more_main_channel

chrishalcrow · 2026-03-26T16:25:03Z

src/spikeinterface/core/sortinganalyzer.py

+        if "main_channel_peak_sign" not in settings:
+            # before 0.104.0 was not in main_channel_peak_sign
+            # TODO make something more fancy that exlore the previous params of extension
+            new_settings["main_channel_peak_sign"] = "both"
+            new_settings["main_channel_peak_mode"] = "extremum"


Hello - Sam asked me to think about backwards compatibility. Here are my thoughts!

Before this PRs, we had peak_sign as a parameter for lots of postprocessig methods. Now we're getting rid of it. We want to ensure backwards compatibility. Note: we cannot be fully backwards compatible - we used to allow for different peak_sign methods for different extensions. Note: THIS WAS BAD. It meant there could be confusing inconsistencies between the results.

When you load an "old" analyzer, you get a sparsity.mask but no main_channel_indices. Use cases of loading an "old" analyzer:

Have a look at it

Do some curation

Recompute extensions with new parameters

The existing analyzer has a sparsity mask. We should respect it. If we do that, 1. is resolved - the analyzer will basically look the same as before.

Currently, when we do a curation, sparsity of the new units is re-computed as a union or intersection. At this stage, we'll also need to recompute the main_channel_index for the merge/split. For this we'll need to know which peak_sign to use.

Recomputing is fine. If a user is recomputing, they know results might be different (improved!!) on a new version.

So, we just need to decide how to compute main_channel_indices in case of curation or re-computation. So that we respect the existing sparsity, I think we should only allow main_channel_indices which exist on the sparsity mask. The options are then:

Set peak_sign to some reasonable default value ('both'!!)

Try to reverse engineer the peak_sign using the templates (note: this is not guaranteed to work: the sparsity estimation that runs on analyzer creation generates templates on the fly - these are not the same as the templates in the analyzer. Or "both" and "neg" will give the same result => problem not invertible. Note: even if we reverse-engineer the peak_sign, the user might have set a different peak_sign for e.g. snr computation)

I think going for "both" is fine. It's easier to explain and the other option is too awkward.

Ok. So my proposal for backwards compat is: on load, compute main_channel_indices using peak_sign = 'both' but only on the channels included in the sparsity mask.

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

alejoe91 added the core Changes to core module label Feb 10, 2026

samuelgarcia added this to the 0.104.0 milestone Feb 16, 2026

samuelgarcia added 2 commits February 17, 2026 15:26

Merge branch 'main' of github.com:SpikeInterface/spikeinterface into …

e7a32b5

…less_peak_sign_more_main_channel

continue the tedious refactoring

7b6a109

alejoe91 modified the milestones: 0.104.0, 0.105.0 Feb 19, 2026

ecobost mentioned this pull request Feb 21, 2026

Use peak_sign in quality metrics.prepare_data to find template extremum #4403

Merged

samuelgarcia added 2 commits March 25, 2026 18:25

Merge branch 'main' of github.com:SpikeInterface/spikeinterface into …

99da9a1

…less_peak_sign_more_main_channel

wip peak sign remove

84e8d37

chrishalcrow reviewed Mar 26, 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants