Refactor concat to use merge for non-concatenated variables #3239

dcherian · 2019-08-22T15:29:57Z

This PR adds two changes:

First, it refactors concat so that non-concatenated variables are passed to merge. concat gains a compat kwarg which tells merge how to merge things. concat's previous behaviour was effectively compat='equals' which is now the default.
Also adds compat="override" to skip equality checking in merge. concat's data_vars='minimal' and coords='minimal' options are now more useful because you can specify compat='override' to skip equality checking and just pick the first one. Previously this would raise an error if any of the variables differed by floating point noise.

xref concat_dim getting added to *all* variables of multifile datasets #2064, closes We need a fast path for open_mfdataset #1823
Tests added
Passes black . && mypy . && flake8
Fully documented, including whats-new.rst for all changes and api.rst for new API

xarray/core/concat.py

dcherian

Ready for review!

dcherian · 2019-08-30T17:57:17Z

xarray/tests/test_combine.py

@@ -567,8 +567,8 @@ def test_combine_concat_over_redundant_nesting(self):

    def test_combine_nested_but_need_auto_combine(self):
        objs = [Dataset({"x": [0, 1]}), Dataset({"x": [2], "wall": [0]})]
-        with raises_regex(ValueError, "cannot be combined"):
-            combine_nested(objs, concat_dim="x")
+        # with raises_regex(ValueError, "cannot be combined"):


This works now. With this PR, I think concat is very similar to auto_combine

Actually this works because wall and x are both indexes and since combine calls align, this will work.

xarray/tests/test_concat.py

dcherian · 2019-09-04T04:55:19Z

Now with docs! I updated the mfdataset and parallel reading sections in dask.rst and io.rst.

doc/dask.rst

shoyer · 2019-09-05T06:47:27Z

xarray/core/concat.py

+    if variables_to_merge:
+        to_merge = []
+        for ds in datasets:
+            if variables_to_merge - set(ds.variables):


Note that set difference is not symmetric. I think you might want:

Suggested change

if variables_to_merge - set(ds.variables):

if variables_to_merge != set(ds.variables):

I think this is right. We are looking for variables that need to be merged but aren't present in ds.

xarray/core/concat.py

shoyer · 2019-09-05T07:02:02Z

xarray/core/merge.py

-                # TODO: add preservation of attrs into fillna
-                out = getattr(out, combine_method)(var)
-                out.attrs = var.attrs
+            equals_list.append(getattr(out, compat)(var.compute()))


Is there a reason why you're now explicitly computing everything ahead of time? In the previous implementation, at least we would not complete everything if there was a failure in comparison between the first two variables.

It seems like the ideal thing to do (in the case of dask) might be to build up a single computation graph and call compute once? Certainly we can save that for later, though.

Is there a reason why you're now explicitly computing everything ahead of time?

This was a mistake. I've fixed it.

I don't see any changes to this logic?

We do have inconsistent behavior here currently (on master), which your PR would resolve:

Inside merge(), variables are computed after doing the equality comparison.

Inside concat(), variables are computed before doing the equality comparison.

We could certainly change this for consistency, but I'm not 100% sure which is the better option.

Ah now I understand, we want to check attrs, shapes etc. before comparing values. How about

if equals is None: out = out.compute() for var in variables[1:]: equals = getattr(out, compat)(var) if not equals: break

This still passes the dask test (test_concat_loads_variables in test_dask.py) but out is still being computed before doing any checks. Not sure how to get around this.

This looks pretty reasonable to me. @crusaderky any thoughts?

I guess the proper solution (for a different PR) is to have a function that loops through a list of variables and compares everything but values; and then if needed, loops again and compares values.

xarray/core/concat.py

xarray/tests/test_concat.py

pep8speaks · 2019-09-10T03:41:14Z

Hello @dcherian! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-09-16 14:47:29 UTC

dcherian · 2019-09-10T21:06:16Z

Test failure is #3300

* upstream/master: ignore h5py 2.10.0 warnings and fix invalid_netcdf warning test. (pydata#3301) Update why-xarray.rst with clearer expression (pydata#3307) Updater to testing environment name (pydata#3253)

xarray/core/merge.py

xarray/core/concat.py

Co-Authored-By: Stephan Hoyer <shoyer@google.com>

…pat-override * 'compat-override' of github.com:dcherian/xarray: Update xarray/core/merge.py

dcherian · 2019-09-16T14:49:23Z

Thanks for the review @shoyer

* master: Fix whats-new date :/ Revert to dev version Release v0.13.0 auto_combine deprecation to 0.14 (pydata#3314) Deprecation: groupby, resample default dim. (pydata#3313) Raise error if cmap is list of colors (pydata#3310) Refactor concat to use merge for non-concatenated variables (pydata#3239) Honor `keep_attrs` in DataArray.quantile (pydata#3305) Fix DataArray api doc (pydata#3309) Accept int value in head, thin and tail (pydata#3298) ignore h5py 2.10.0 warnings and fix invalid_netcdf warning test. (pydata#3301) Update why-xarray.rst with clearer expression (pydata#3307) Compat and encoding deprecation to 0.14 (pydata#3294) Remove deprecated concat kwargs. (pydata#3288) allow np-array levels and colors in 2D plots (pydata#3295) Remove some deprecations (pydata#3292) Make argmin/max work lazy with dask (pydata#3244) Add head, tail and thin methods (pydata#3278) Updater to testing environment name (pydata#3253)

* upstream/master: (43 commits) Add hypothesis support to related projects (#3335) More doc fixes (#3333) Improve the documentation of swap_dims (#3331) fix the doc names of the return value of swap_dims (#3329) Fix isel performance regression (#3319) Allow weakref (#3318) Clarify that "scatter" is a plotting method in what's new. (#3316) Fix whats-new date :/ Revert to dev version Release v0.13.0 auto_combine deprecation to 0.14 (#3314) Deprecation: groupby, resample default dim. (#3313) Raise error if cmap is list of colors (#3310) Refactor concat to use merge for non-concatenated variables (#3239) Honor `keep_attrs` in DataArray.quantile (#3305) Fix DataArray api doc (#3309) Accept int value in head, thin and tail (#3298) ignore h5py 2.10.0 warnings and fix invalid_netcdf warning test. (#3301) Update why-xarray.rst with clearer expression (#3307) Compat and encoding deprecation to 0.14 (#3294) ...

dcherian mentioned this pull request Aug 22, 2019

concat_dim getting added to *all* variables of multifile datasets #2064

Open

dcherian force-pushed the compat-override branch from 7afbe51 to 1b4858f Compare August 22, 2019 18:44

dcherian commented Aug 22, 2019

View reviewed changes

xarray/core/concat.py Outdated Show resolved Hide resolved

dcherian force-pushed the compat-override branch 2 times, most recently from da23a33 to d3191d9 Compare August 29, 2019 17:13

dcherian added 6 commits August 29, 2019 11:13

Add compat = 'override' and data_vars/coords='sensible'

09292b0

concat tests.

69feaaa

Update docstring.

0bf8597

Begin merge, combine.

4c994e2

Merge non concatenated variables.

4cb7d02

Fix tests.

9dc340e

dcherian force-pushed the compat-override branch from d3191d9 to 9dc340e Compare August 29, 2019 17:14

dcherian added 2 commits August 29, 2019 13:02

Fix tests 2

dd24b00

Fix test 3

09746d9

dcherian changed the title ~~[WIP] compat="override", data_vars/coords="sensible"~~ [WIP] Refactor concat to use merge for non-concatenated variables Aug 29, 2019

dcherian added 6 commits August 30, 2019 11:40

Cleanup: reduce number of times we loop over datasets.

b090e82

unique_variable does minimum number of loads: fixes dask test

4ba681b

docstrings for compat='override'

75bd59b

concat compat docstring.

79ee32d

remove the sensible option.

e7b152c

reduce silly changes.

c114143

dcherian force-pushed the compat-override branch from 25b8659 to c114143 Compare August 30, 2019 17:59

dcherian commented Aug 30, 2019

View reviewed changes

dcherian changed the title ~~[WIP] Refactor concat to use merge for non-concatenated variables~~ Refactor concat to use merge for non-concatenated variables Aug 30, 2019

dcherian added 2 commits August 30, 2019 12:39

fix groupby order test.

9f849d2

cleanup: var names + remove one loop through datasets.

40707ca

dcherian requested a review from TomNicholas August 30, 2019 18:55

dcherian added 3 commits September 3, 2019 16:56

fix warning.

633efe9

Add whats-new entry.

6a6daab

Add note in io.rst

eb154f5

dcherian mentioned this pull request Sep 5, 2019

0.13.0 release #3257

Closed

shoyer reviewed Sep 5, 2019

View reviewed changes

dcherian and others added 7 commits September 6, 2019 13:07

simplify parse_datasets.

3154b06

Avoid using merge_variables. unique_variable instead.

02e35e6

small stuff.

d7988c9

Update docs.

628974e

minor fix.

73a203b

minor fix.

6b40c1f

Merge branch 'master' into compat-override

d849a0f

dcherian added 5 commits September 9, 2019 21:43

lint.

e4c12e8

Better error message.

935089a

rename to shorter variable names.

c13dcff

Cleanup: fillna preserves attrs now.

6e4727a

Look for concat dim in data_vars also.

71ec88d

Merge remote-tracking branch 'upstream/master' into compat-override

223c998

* upstream/master: ignore h5py 2.10.0 warnings and fix invalid_netcdf warning test. (pydata#3301) Update why-xarray.rst with clearer expression (pydata#3307) Updater to testing environment name (pydata#3253)

dcherian requested a review from shoyer September 13, 2019 16:05

shoyer reviewed Sep 15, 2019

View reviewed changes

xarray/core/merge.py Outdated Show resolved Hide resolved

xarray/core/concat.py Outdated Show resolved Hide resolved

dcherian and others added 3 commits September 15, 2019 00:58

Update xarray/core/merge.py

d77bf93

Co-Authored-By: Stephan Hoyer <shoyer@google.com>

avoid unnecessary computes.

bdd388f

Merge branch 'compat-override' of github.com:dcherian/xarray into com…

c6670e4

…pat-override * 'compat-override' of github.com:dcherian/xarray: Update xarray/core/merge.py

shoyer approved these changes Sep 15, 2019

View reviewed changes

minor cleanups.

50d3b8f

dcherian merged commit 756c941 into pydata:master Sep 16, 2019

dcherian deleted the compat-override branch September 16, 2019 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor concat to use merge for non-concatenated variables #3239

Refactor concat to use merge for non-concatenated variables #3239

dcherian commented Aug 22, 2019 •

edited

Loading

dcherian left a comment

dcherian Aug 30, 2019

dcherian Sep 13, 2019

dcherian commented Sep 4, 2019

shoyer Sep 5, 2019

dcherian Sep 6, 2019

shoyer Sep 5, 2019

dcherian Sep 6, 2019

shoyer Sep 15, 2019

dcherian Sep 15, 2019

shoyer Sep 15, 2019

dcherian Sep 15, 2019

pep8speaks commented Sep 10, 2019 •

edited

Loading

dcherian commented Sep 10, 2019

dcherian commented Sep 16, 2019

	if variables_to_merge - set(ds.variables):
	if variables_to_merge != set(ds.variables):

Refactor concat to use merge for non-concatenated variables #3239

Refactor concat to use merge for non-concatenated variables #3239

Conversation

dcherian commented Aug 22, 2019 • edited Loading

dcherian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcherian commented Sep 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Sep 10, 2019 • edited Loading

Comment last updated at 2019-09-16 14:47:29 UTC

dcherian commented Sep 10, 2019

dcherian commented Sep 16, 2019

dcherian commented Aug 22, 2019 •

edited

Loading

pep8speaks commented Sep 10, 2019 •

edited

Loading