Repository: spikeinterface/spikeinterface
Issue: Possible bug in MultiSortingComparison._populate_spiketrains

Hi! I have been attempting to generate an `AgreementSorting` object based on putative units derived from KiloSort2 and Mountainsort4.

For some units, `agreement_spiketrains` derived from `MultiSortingComparison.get_agreement_sorting` of KS2 and MS4 putative units do not correctly reflect the intersection of the input spiketrains.

I think this is because of the re-use of variable name `unit_id` on lines 97 and 98 in the method `_populate_spiketrains`

From reading the code, it seems there are three relevant 'reference frames' for unit ids. Each sorting has its own 'reference frame', and there is a third, global reference frame that combines the units from each sorting.

In the inner loop of `_populate_spiketrains`, `unit_id` references this 'global' reference frame. This global `unit_id` is later used to save the agreement_spiketrain in `spike_trains_segment[unit_id]`.

On line 97, `unit_id` is reassigned to a value from one of the original sorting objects' reference frames. This ends up overwriting some agreement_spiketrains with totally unrelated spiketrains that happen to share the same `unit_id` but from the other sorting.

As an example from my sortings, KS2's unit 183 is a good match with MS4's unit 112. When `unit_id == 183`, `spike_trains_segment[183] `is appropriately saved as the intersection of spikes between `KS2[183]` and `MS4[112]`.

However, when `unit_id == 820` in the global reference frame, line 97: `unit_id == list(sorter_unit_ids.values())[0]` sets `unit_id` equal to 183 because 820 in the global reference frame maps to 183 in the MS4 reference frame.

Because the variable `unit_id` is reassigned here, the subsequent saving of the spiketrain, which should have been to `spike_trains_segment[820]`, ends up overwriting `spike_trains_segment[183]`.

I think this can easily be fixed by renaming the `unit_id` variable on lines 97 and 98 to something else, i.e. `this_sorter_unit_id`.


As an aside, the language in the tutorial notebooks and readthedocs was a little confusing to me. An important feature of the comparisons module is that it can let you extract spiketrains containing only the spikes agreed upon by multiple sorters. I assumed that this would be the default behavior, but it's necessary to change the default parameter of `spiketrain_mode` from 'union' to 'intersection' to achieve this. It might be helpful to make this clear in the documentation or to change the default from 'union' to 'intersection'.

Thanks so much!


Changed files:
src/spikeinterface/comparison/multicomparisons.py

diff --git a/src/spikeinterface/comparison/multicomparisons.py b/src/spikeinterface/comparison/multicomparisons.py
index fe1b6b27..b3aa457c 100644
--- a/src/spikeinterface/comparison/multicomparisons.py
+++ b/src/spikeinterface/comparison/multicomparisons.py
@@ -94,8 +94,8 @@ class MultiSortingComparison(BaseMultiComparison, MixinSpikeTrainComparison):
                 # Append correct spike train
                 if len(sorter_unit_ids.keys()) == 1:
                     sorting = self.object_list[self.name_list.index(list(sorter_unit_ids.keys())[0])]
-                    unit_id = list(sorter_unit_ids.values())[0]
-                    spike_train = sorting.get_unit_spike_train(unit_id, seg_index)
+                    this_sorting_unit_id = list(sorter_unit_ids.values())[0]
+                    spike_train = sorting.get_unit_spike_train(this_sorting_unit_id, seg_index)
                 else:
                     max_edge = edges[int(np.argmax([d['weight']
                                         for u, v, d in edges]))]