Repository: SpikeInterface/spikeinterface
Pull request #2845 (2024-05-14 14:37:52)

Avoid duplicating channel names as property in neo base extractor when `all_annotations=True`
As in the title. 

There is always a `channel_names` annotation in neo that we can load with `all_annotations=True`:

https://github.com/h-mayorquin/python-neo/blob/223e22edb917d8ad5ef8aff4fe7197afc414d5f6/neo/rawio/baserawio.py#L282-L293

This PR modifieds the code so when `all_annotations=True` the property is not duplicated. 

Plus, I am changing the name of the property to `channel_names` because that's how it is neo and I think is more consistent overall. Another option is to instead change the property  name to "channel_name" when loaded from neo annotations so we don't duplicate. I think that the first approach is better.


Patch:
diff --git a/pyproject.toml b/pyproject.toml
index cb71bb499..8fc4c449f 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -23,10 +23,9 @@ dependencies = [
     "numpy",
     "threadpoolctl>=3.0.0",
     "tqdm",
-    "zarr>=2.16,<2.18",
+    "zarr>=0.2.16",
     "neo>=0.13.0",
     "probeinterface>=0.2.21",
-    "packaging",
 ]
 
 [build-system]
diff --git a/src/spikeinterface/core/channelsaggregationrecording.py b/src/spikeinterface/core/channelsaggregationrecording.py
index b8735dff3..9ec7ca8d4 100644
--- a/src/spikeinterface/core/channelsaggregationrecording.py
+++ b/src/spikeinterface/core/channelsaggregationrecording.py
@@ -14,31 +14,18 @@ class ChannelsAggregationRecording(BaseRecording):
     """
 
     def __init__(self, recording_list, renamed_channel_ids=None):
-
-        # Generate a default list of channel ids that are unique and consecutive numbers as strings.
         channel_map = {}
-        num_all_channels = sum(rec.get_num_channels() for rec in recording_list)
 
+        num_all_channels = sum([rec.get_num_channels() for rec in recording_list])
         if renamed_channel_ids is not None:
-            assert (
-                len(np.unique(renamed_channel_ids)) == num_all_channels
-            ), "'renamed_channel_ids' doesn't have the right size or has duplicates!"
+            assert len(np.unique(renamed_channel_ids)) == num_all_channels, (
+                "'renamed_channel_ids' doesn't have the " "right size or has duplicates!"
+            )
             channel_ids = list(renamed_channel_ids)
         else:
-            # Collect channel IDs from all recordings
-            all_channels_have_same_type = np.unique([rec.channel_ids.dtype for rec in recording_list]).size == 1
-            all_channel_ids_are_unique = False
-            if all_channels_have_same_type:
-                combined_ids = np.concatenate([rec.channel_ids for rec in recording_list])
-                all_channel_ids_are_unique = np.unique(combined_ids).size == num_all_channels
-
-            if all_channels_have_same_type and all_channel_ids_are_unique:
-                channel_ids = combined_ids
-            else:
-                # If IDs are not unique or not of the same type, use default as stringify IDs
-                default_channel_ids = [str(i) for i in range(num_all_channels)]
-                channel_ids = default_channel_ids
+            channel_ids = list(np.arange(num_all_channels))
 
+        # channel map maps channel indices that are used to get traces
         ch_id = 0
         for r_i, recording in enumerate(recording_list):
             single_channel_ids = recording.get_channel_ids()
@@ -62,9 +49,7 @@ class ChannelsAggregationRecording(BaseRecording):
                 break
 
         if not (ok1 and ok2 and ok3 and ok4):
-            raise ValueError(
-                "Recordings do not have consistent sampling frequency, number of segments, data type, or number of samples."
-            )
+            raise ValueError("Sortings don't have the same sampling_frequency/num_segments/dtype/num samples")
 
         BaseRecording.__init__(self, sampling_frequency, channel_ids, dtype)
 
@@ -106,7 +91,7 @@ class ChannelsAggregationRecording(BaseRecording):
             self.add_recording_segment(sub_segment)
 
         self._recordings = recording_list
-        self._kwargs = {"recording_list": recording_list, "renamed_channel_ids": renamed_channel_ids}
+        self._kwargs = {"recording_list": [rec for rec in recording_list], "renamed_channel_ids": renamed_channel_ids}
 
     @property
     def recordings(self):
@@ -188,11 +173,11 @@ def aggregate_channels(recording_list, renamed_channel_ids=None):
     recording_list: list
         List of BaseRecording objects to aggregate
     renamed_channel_ids: array-like
-        If given, channel ids are renamed as provided.
+        If given, channel ids are renamed as provided. If None, unit ids are sequential integers.
 
     Returns
     -------
-    aggregate_recording: ChannelsAggregationRecording
-        The aggregated recording object
+    aggregate_recording: UnitsAggregationSorting
+        The aggregated sorting object
     """
     return ChannelsAggregationRecording(recording_list, renamed_channel_ids)
diff --git a/src/spikeinterface/core/template_tools.py b/src/spikeinterface/core/template_tools.py
index fead805ab..11581a49c 100644
--- a/src/spikeinterface/core/template_tools.py
+++ b/src/spikeinterface/core/template_tools.py
@@ -58,7 +58,6 @@ def get_template_amplitudes(
     peak_sign: "neg" | "pos" | "both" = "neg",
     mode: "extremum" | "at_index" = "extremum",
     return_scaled: bool = True,
-    abs_value: bool = True,
 ):
     """
     Get amplitude per channel for each unit.
@@ -74,8 +73,6 @@ def get_template_amplitudes(
         "at_index": take value at spike index
     return_scaled: bool, default True
         The amplitude is scaled or not.
-    abs_value: bool = True
-        Whether the extremum amplitude should be returned as an absolute value or not
 
     Returns
     -------
@@ -99,18 +96,17 @@ def get_template_amplitudes(
             if peak_sign == "both":
                 values = np.max(np.abs(template), axis=0)
             elif peak_sign == "neg":
-                values = np.min(template, axis=0)
+                values = -np.min(template, axis=0)
             elif peak_sign == "pos":
                 values = np.max(template, axis=0)
         elif mode == "at_index":
             if peak_sign == "both":
                 values = np.abs(template[before, :])
-            elif peak_sign in ["neg", "pos"]:
+            elif peak_sign == "neg":
+                values = -template[before, :]
+            elif peak_sign == "pos":
                 values = template[before, :]
 
-        if abs_value:
-            values = np.abs(values)
-
         peak_values[unit_id] = values
 
     return peak_values
@@ -164,7 +160,7 @@ def get_template_extremum_channel(
     extremum_channels_id = {}
     extremum_channels_index = {}
     for unit_id in unit_ids:
-        max_ind = np.argmax(np.abs(peak_values[unit_id]))
+        max_ind = np.argmax(peak_values[unit_id])
         extremum_channels_id[unit_id] = channel_ids[max_ind]
         extremum_channels_index[unit_id] = max_ind
 
@@ -231,7 +227,6 @@ def get_template_extremum_amplitude(
     templates_or_sorting_analyzer,
     peak_sign: "neg" | "pos" | "both" = "neg",
     mode: "extremum" | "at_index" = "at_index",
-    abs_value: bool = True,
 ):
     """
     Computes amplitudes on the best channel.
@@ -246,9 +241,6 @@ def get_template_extremum_amplitude(
         Where the amplitude is computed
         "extremum":  max or min
         "at_index": take value at spike index
-    abs_value: bool = True
-        Whether the extremum amplitude should be returned as an absolute value or not
-
 
     Returns
     -------
@@ -268,7 +260,7 @@ def get_template_extremum_amplitude(
         return_scaled = True
 
     extremum_amplitudes = get_template_amplitudes(
-        templates_or_sorting_analyzer, peak_sign=peak_sign, mode=mode, return_scaled=return_scaled, abs_value=abs_value
+        templates_or_sorting_analyzer, peak_sign=peak_sign, mode=mode, return_scaled=return_scaled
     )
 
     unit_amplitudes = {}
diff --git a/src/spikeinterface/extractors/neoextractors/neobaseextractor.py b/src/spikeinterface/extractors/neoextractors/neobaseextractor.py
index 1620a6882..c30c6b94f 100644
--- a/src/spikeinterface/extractors/neoextractors/neobaseextractor.py
+++ b/src/spikeinterface/extractors/neoextractors/neobaseextractor.py
@@ -179,6 +179,7 @@ class NeoBaseRecordingExtractor(_NeoBaseExtractor, BaseRecording):
             If True, include all annotations in the extracted data.
         use_names_as_ids : Optional[bool], default: None
             If True, use channel names as IDs. Otherwise, use default IDs.
+            In NEO the ids are guaranteed to be unique. Names are user defined and can be duplicated.
         neo_kwargs : Dict[str, Any]
             Additional keyword arguments to pass to the NeoBaseExtractor for initialization.
 
@@ -275,8 +276,8 @@ class NeoBaseRecordingExtractor(_NeoBaseExtractor, BaseRecording):
 
         self.set_property("gain_to_uV", final_gains)
         self.set_property("offset_to_uV", final_offsets)
-        if not use_names_as_ids:
-            self.set_property("channel_name", signal_channels["name"])
+        if not use_names_as_ids and not all_annotations:
+            self.set_property("channel_names", signal_channels["name"])
 
         if all_annotations:
             block_ann = self.neo_reader.raw_annotations["blocks"][self.block_index]
diff --git a/src/spikeinterface/extractors/neoextractors/spikegadgets.py b/src/spikeinterface/extractors/neoextractors/spikegadgets.py
index 7d6b49232..e835edf96 100644
--- a/src/spikeinterface/extractors/neoextractors/spikegadgets.py
+++ b/src/spikeinterface/extractors/neoextractors/spikegadgets.py
@@ -1,10 +1,9 @@
 from __future__ import annotations
-from pathlib import Path
 
-import packaging
+from pathlib import Path
 
-import packaging.version
 import probeinterface
+
 from spikeinterface.core.core_tools import define_function_from_class
 
 from .neobaseextractor import NeoBaseRecordingExtractor
@@ -39,9 +38,7 @@ class SpikeGadgetsRecordingExtractor(NeoBaseRecordingExtractor):
         )
         self._kwargs.update(dict(file_path=str(Path(file_path).absolute()), stream_id=stream_id))
 
-        probegroup = None  # TODO remove once probeinterface is updated to 0.2.22 in the pyproject.toml
-        if packaging.version.parse(probeinterface.__version__) > packaging.version.parse("0.2.21"):
-            probegroup = probeinterface.read_spikegadgets(file_path, raise_error=False)
+        probegroup = probeinterface.read_spikegadgets(file_path, raise_error=False)
 
         if probegroup is not None:
             self.set_probes(probegroup, in_place=True)


Test patch:
diff --git a/src/spikeinterface/core/tests/test_channelsaggregationrecording.py b/src/spikeinterface/core/tests/test_channelsaggregationrecording.py
index 118b6092a..944c3aa0b 100644
--- a/src/spikeinterface/core/tests/test_channelsaggregationrecording.py
+++ b/src/spikeinterface/core/tests/test_channelsaggregationrecording.py
@@ -23,6 +23,7 @@ def test_channelsaggregationrecording():
 
     # test num channels
     recording_agg = aggregate_channels([recording1, recording2, recording3])
+    print(recording_agg)
     assert len(recording_agg.get_channel_ids()) == 3 * num_channels
 
     assert np.allclose(recording_agg.get_times(0), recording1.get_times(0))
@@ -36,21 +37,21 @@ def test_channelsaggregationrecording():
         traces2_0 = recording2.get_traces(channel_ids=[channel_ids[0]], segment_index=seg)
         traces3_2 = recording3.get_traces(channel_ids=[channel_ids[2]], segment_index=seg)
 
-        assert np.allclose(traces1_1, recording_agg.get_traces(channel_ids=[str(channel_ids[1])], segment_index=seg))
+        assert np.allclose(traces1_1, recording_agg.get_traces(channel_ids=[channel_ids[1]], segment_index=seg))
         assert np.allclose(
-            traces2_0, recording_agg.get_traces(channel_ids=[str(num_channels + channel_ids[0])], segment_index=seg)
+            traces2_0, recording_agg.get_traces(channel_ids=[num_channels + channel_ids[0]], segment_index=seg)
         )
         assert np.allclose(
-            traces3_2, recording_agg.get_traces(channel_ids=[str(2 * num_channels + channel_ids[2])], segment_index=seg)
+            traces3_2, recording_agg.get_traces(channel_ids=[2 * num_channels + channel_ids[2]], segment_index=seg)
         )
         # all traces
         traces1 = recording1.get_traces(segment_index=seg)
         traces2 = recording2.get_traces(segment_index=seg)
         traces3 = recording3.get_traces(segment_index=seg)
 
-        assert np.allclose(traces1, recording_agg.get_traces(channel_ids=["0", "1", "2"], segment_index=seg))
-        assert np.allclose(traces2, recording_agg.get_traces(channel_ids=["3", "4", "5"], segment_index=seg))
-        assert np.allclose(traces3, recording_agg.get_traces(channel_ids=["6", "7", "8"], segment_index=seg))
+        assert np.allclose(traces1, recording_agg.get_traces(channel_ids=[0, 1, 2], segment_index=seg))
+        assert np.allclose(traces2, recording_agg.get_traces(channel_ids=[3, 4, 5], segment_index=seg))
+        assert np.allclose(traces3, recording_agg.get_traces(channel_ids=[6, 7, 8], segment_index=seg))
 
     # test rename channels
     renamed_channel_ids = [f"#Channel {i}" for i in range(3 * num_channels)]
@@ -80,41 +81,5 @@ def test_channelsaggregationrecording():
     print(recording_agg_prop.get_property("brain_area"))
 
 
-def test_channel_aggregation_preserve_ids():
-
-    recording1 = generate_recording(num_channels=3, durations=[10], set_probe=False)  # To avoid location check
-    recording1 = recording1.rename_channels(new_channel_ids=["a", "b", "c"])
-    recording2 = generate_recording(num_channels=2, durations=[10], set_probe=False)
-    recording2 = recording2.rename_channels(new_channel_ids=["d", "e"])
-
-    aggregated_recording = aggregate_channels([recording1, recording2])
-    assert aggregated_recording.get_num_channels() == 5
-    assert list(aggregated_recording.get_channel_ids()) == ["a", "b", "c", "d", "e"]
-
-
-def test_channel_aggregation_does_not_preserve_ids_if_not_unique():
-
-    recording1 = generate_recording(num_channels=3, durations=[10], set_probe=False)  # To avoid location check
-    recording1 = recording1.rename_channels(new_channel_ids=["a", "b", "c"])
-    recording2 = generate_recording(num_channels=2, durations=[10], set_probe=False)
-    recording2 = recording2.rename_channels(new_channel_ids=["a", "b"])
-
-    aggregated_recording = aggregate_channels([recording1, recording2])
-    assert aggregated_recording.get_num_channels() == 5
-    assert list(aggregated_recording.get_channel_ids()) == ["0", "1", "2", "3", "4"]
-
-
-def test_channel_aggregation_does_not_preserve_ids_not_the_same_type():
-
-    recording1 = generate_recording(num_channels=3, durations=[10], set_probe=False)  # To avoid location check
-    recording1 = recording1.rename_channels(new_channel_ids=["a", "b", "c"])
-    recording2 = generate_recording(num_channels=2, durations=[10], set_probe=False)
-    recording2 = recording2.rename_channels(new_channel_ids=[1, 2])
-
-    aggregated_recording = aggregate_channels([recording1, recording2])
-    assert aggregated_recording.get_num_channels() == 5
-    assert list(aggregated_recording.get_channel_ids()) == ["0", "1", "2", "3", "4"]
-
-
 if __name__ == "__main__":
     test_channelsaggregationrecording()