Hotfix for #2662 (#2678)

* Hotfix for #2662 * Separate keyfunc * Added a test, and made others slightly stricter * Comment explaining test better
pydata · Jan 24, 2019 · 55fbd41 · 55fbd41
1 parent 8d37dc3
commit 55fbd41
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 4 deletions.
diff --git a/xarray/core/combine.py b/xarray/core/combine.py
@@ -493,16 +493,21 @@ def _auto_combine_all_along_first_dim(combined_ids, dim, data_vars,
     return new_combined_ids
 
 
+def vars_as_keys(ds):
+    return tuple(sorted(ds))
+
+
 def _auto_combine_1d(datasets, concat_dim=_CONCAT_DIM_DEFAULT,
                      compat='no_conflicts',
                      data_vars='all', coords='different'):
     # This is just the old auto_combine function (which only worked along 1D)
     if concat_dim is not None:
         dim = None if concat_dim is _CONCAT_DIM_DEFAULT else concat_dim
-        grouped = itertools.groupby(datasets, key=lambda ds: tuple(sorted(ds)))
+        sorted_datasets = sorted(datasets, key=vars_as_keys)
+        grouped_by_vars = itertools.groupby(sorted_datasets, key=vars_as_keys)
         concatenated = [_auto_concat(list(ds_group), dim=dim,
                                      data_vars=data_vars, coords=coords)
-                        for id, ds_group in grouped]
+                        for id, ds_group in grouped_by_vars]
     else:
         concatenated = datasets
     merged = merge(concatenated, compat=compat)

diff --git a/xarray/tests/test_combine.py b/xarray/tests/test_combine.py
@@ -650,7 +650,7 @@ def test_merge_one_dim_concat_another(self):
         expected = Dataset({'foo': ('x', [0, 1, 2, 3]),
                             'bar': ('x', [10, 20, 30, 40])})
 
-        actual = auto_combine(objs, concat_dim=['x', None])
+        actual = auto_combine(objs, concat_dim=['x', None], compat='equals')
         assert_identical(expected, actual)
 
         actual = auto_combine(objs)
@@ -661,7 +661,19 @@ def test_merge_one_dim_concat_another(self):
                  Dataset({'foo': ('x', [2, 3])})],
                 [Dataset({'bar': ('x', [10, 20])}),
                  Dataset({'bar': ('x', [30, 40])})]]
-        actual = auto_combine(objs, concat_dim=[None, 'x'])
+        actual = auto_combine(objs, concat_dim=[None, 'x'], compat='equals')
+        assert_identical(expected, actual)
+
+    def test_internal_ordering(self):
+        # This gives a MergeError if _auto_combine_1d is not sorting by
+        # data_vars correctly, see GH #2662
+        objs = [Dataset({'foo': ('x', [0, 1])}),
+                Dataset({'bar': ('x', [10, 20])}),
+                Dataset({'foo': ('x', [2, 3])}),
+                Dataset({'bar': ('x', [30, 40])})]
+        actual = auto_combine(objs, concat_dim='x', compat='equals')
+        expected = Dataset({'foo': ('x', [0, 1, 2, 3]),
+                            'bar': ('x', [10, 20, 30, 40])})
         assert_identical(expected, actual)
 
     def test_combine_concat_over_redundant_nesting(self):