Repository: rsagroup/rsatoolbox

Issue:
`RDMs.sort_by()` take a `reindex` boolean argument to replace the `"index"` pattern descriptor
I just became rather confused because `RDMs.sort_by()` followed by `RDMs.subset_patterns(by="index", ...)` didn't have the expected effect.  In other words, sorting the conditions _also_ sorts `.pattern_descriptors["index"]`, so then `.subset_patterns(by="index", ...)` doesn't subset the patterns by their _current_ indices, but by their indices _prior to sorting_.

I'm sure there are many cases where it makes sense to sort the `"index"` pattern descriptor too, but in my case it'd be useful to automatically keep the patterns with an incremental index.  I propose `RDMs.sort_by()` takes a new boolean argument `reindex` (default `False`), which when `True` resets the `"index"` pattern descriptor to 0, 1, ..., n_cond-1.

Is there a reason or current usage of the `"index"` pattern descriptor which would make this problematic?

Hints from issue comments:
@caiw: I'm happy to implement this, but thought it worth the discussion first.@JasperVanDenBosch: I think this makes sense. Maybe the `reindex` argument should even be `True` by default. We could throw a deprecation warning.@caiw: `sort_by()` takes pattern_descriptors in the form of kwargs.   This means if the `RDMs` object happens to have a `pattern_descriptor` named `"reindex"`, we may have a conflict.  E.g. one couldn't specify `RDMs.sort_by(reindex='alpha', reindex=False)`.

Solution patch:
diff --git a/src/rsatoolbox/rdm/rdms.py b/src/rsatoolbox/rdm/rdms.py
index 655ad3d7..3ce09a62 100644
--- a/src/rsatoolbox/rdm/rdms.py
+++ b/src/rsatoolbox/rdm/rdms.py
@@ -422,9 +422,13 @@ class RDMs:
         for dname, descriptors in self.pattern_descriptors.items():
             self.pattern_descriptors[dname] = [descriptors[idx] for idx in new_order]
 
-    def sort_by(self, **kwargs):
+    def sort_by(self, reindex: bool = True, **kwargs):
         """Reorder the patterns by sorting a descriptor
 
+        Args:
+            reindex (bool): whether to reset the 'index' descriptor 
+                following sorting
+
         Pass keyword arguments that correspond to descriptors,
         with value indicating the sort type. Supported methods:
 
@@ -454,7 +458,7 @@ class RDMs:
         for dname, method in kwargs.items():
             if method == 'alpha':
                 descriptor = self.pattern_descriptors[dname]
-                self.reorder(np.argsort(descriptor))
+                self.reorder(np.argsort(descriptor, kind='stable'))
             elif isinstance(method, (list, np.ndarray)):
                 # in this case, `method` is the desired descriptor order
                 new_order = method
@@ -466,6 +470,8 @@ class RDMs:
                 self.reorder([list(descriptor).index(x) for x in new_order])
             else:
                 raise ValueError(f'Unknown sorting method: {method}')
+        if reindex:
+            self.pattern_descriptors['index'] = list(range(self.n_cond))
 
     def mean(self, weights=None):
         """Average rdm of all rdms contained
@@ -569,8 +575,14 @@ def concat(*rdms):
         rdm.dissimilarities
         for rdm in rdms_list
         ], axis=0)
+    # Set dissimilarity measure if it's the same for all rdms in list
+    if len(set(r.dissimilarity_measure for r in rdms_list)) == 1:
+        dissimilarity_measure = rdms_list[0].dissimilarity_measure
+    else:
+        dissimilarity_measure = None
     rdm = RDMs(
         dissimilarities=dissimilarities,
+        dissimilarity_measure=dissimilarity_measure,
         rdm_descriptors=rdm_descriptors,
         descriptors=rdms_list[0].descriptors,
         pattern_descriptors=rdms_list[0].pattern_descriptors


Test patch:
diff --git a/tests/test_rdm.py b/tests/test_rdm.py
index b1429970..d62061f4 100644
--- a/tests/test_rdm.py
+++ b/tests/test_rdm.py
@@ -324,6 +324,7 @@ class TestRDM(unittest.TestCase):
         rdms = concat((rdms1, rdms2))
         self.assertEqual(rdms.n_rdm, 16)
         assert len(rdms.rdm_descriptors['session']) == 16
+        self.assertEqual(rdms.dissimilarity_measure, mes)
 
     def test_concat_varargs_multiple_rdms(self):
         from rsatoolbox.rdm import concat
@@ -429,6 +430,44 @@ class TestRDM(unittest.TestCase):
             )
         )
 
+    def test_sort_by_reindex_resets_index(self):
+        from rsatoolbox.rdm import RDMs
+        rdm = np.array([
+            [0., 1., 2., 3.],
+            [1., 0., 1., 2.],
+            [2., 1., 0., 1.],
+            [3., 2., 1., 0.]]
+        )
+        conds = ['b', 'a', 'c', 'd']
+        rdms = RDMs(
+            np.atleast_2d(squareform(rdm)),
+            pattern_descriptors=dict(conds=conds)
+        )
+        rdms.sort_by(conds='alpha', reindex=True)
+        self.assertEqual(
+            rdms.pattern_descriptors['index'],
+            list(range(rdms.n_cond))
+        )
+
+    def test_sort_by_not_reindex_does_not_reset_index(self):
+        from rsatoolbox.rdm import RDMs
+        rdm = np.array([
+            [0., 1., 2., 3.],
+            [1., 0., 1., 2.],
+            [2., 1., 0., 1.],
+            [3., 2., 1., 0.]]
+        )
+        conds = ['b', 'a', 'c', 'd']
+        rdms = RDMs(
+            np.atleast_2d(squareform(rdm)),
+            pattern_descriptors=dict(conds=conds)
+        )
+        rdms.sort_by(conds='alpha', reindex=False)
+        self.assertNotEqual(
+            rdms.pattern_descriptors['index'],
+            list(range(rdms.n_cond))
+        )
+
     def test_sort_by_list(self):
         from rsatoolbox.rdm import RDMs
         rdm = np.array([
@@ -460,6 +499,35 @@ class TestRDM(unittest.TestCase):
             )
         )
 
+    def test_sort_stable(self):
+        from rsatoolbox.rdm import RDMs
+        rdm = np.array([
+            [0., 1., 2., 3., 4., 5., 6., 7., 8., 9.],
+            [1., 0., 1., 2., 3., 4., 5., 6., 7., 8.],
+            [2., 1., 0., 1., 2., 3., 4., 5., 6., 7.],
+            [3., 2., 1., 0., 1., 2., 3., 4., 5., 6.],
+            [4., 3., 2., 1., 0., 1., 2., 3., 4., 5.],
+            [5., 4., 3., 2., 1., 0., 1., 2., 3., 4.],
+            [6., 5., 4., 3., 2., 1., 0., 1., 2., 3.],
+            [7., 6., 5., 4., 3., 2., 1., 0., 1., 2.],
+            [8., 7., 6., 5., 4., 3., 2., 1., 0., 1.],
+            [9., 8., 7., 6., 5., 4., 3., 2., 1., 0.],
+        ]
+        )
+        conds = list(reversed("abcdefghij"))
+        cats  = list("ababababab")
+        rdms = RDMs(
+            np.atleast_2d(squareform(rdm)),
+            pattern_descriptors=dict(conds=conds, cats=cats)
+        )
+        rdms.sort_by(index=np.random.permutation(rdms.n_cond).tolist())  # Randomise the condition labels first
+        rdms.sort_by(conds='alpha')
+        rdms.sort_by(cats="alpha")
+        self.assertListEqual(
+            list(rdms.pattern_descriptors["conds"]),
+            list("bdfhj") + list("acegi"),
+        )
+
     def test_copy(self):
         from rsatoolbox.rdm import RDMs
         orig = RDMs(