add a drop_conflicts strategy for merging attrs #4827

keewis · 2021-01-19T22:54:10Z

As described in #4749, this adds a "drop_conflicts" strategy for merging attrs. For now, only merge works, combine_by_attrs, combine_nested, and concat still fail because they rely on variable.concat to always use "override".

Closes Option for combine_attrs with conflicting values silently dropped #4749
Tests added
Passes pre-commit run --all-files
User visible changes (including notable bug fixes) are documented in whats-new.rst
New functions/methods are listed in api.rst

_{Overriding CI behaviors

By default, the upstream dev CI is disabled on pull request and push events. You can override this behavior per commit by adding a [test-upstream] tag to the first line of the commit message. For documentation-only commits, you can skip the CI per commit by adding a [skip-ci] tag to the first line of the commit message}

changed functions: concat, merge, combine_by_coords, combine_nested, merge_core

keewis · 2021-01-23T23:00:36Z

this seems a bit tricky to get right: combine_attrs is only applied to the main object, not to its variables:

xarray/xarray/core/merge.py

Line 597 in fb67358

variables, out_indexes = merge_collected(collected, prioritized, compat=compat)

and similar for concat (and probably combine_*). For now, I'll mark the tests as "skipped".

keewis · 2021-01-23T23:32:27Z

I only modified merge, concat and combine_* (binary arithmetic and apply_ufunc only support a boolean (keep_attrs), which means the choice is between "drop" and "override"). Dataset.merge currently does not have a combine_attrs kwarg (I think it uses the default of merge_core, which is "override"), and I will leave where and Dataset.where to #4687.

As mentioned above, merge_attrs is currently only applied to the objects themselves, the attrs of variables and coordinates are merged using "override" (I think?).

keewis · 2021-01-23T23:32:52Z

Unless I'm missing something this should be ready for review

max-sixty

While I don't have that much context — the code looks excellent, and the tests very complete!

mathause

LGTM.

mathause · 2021-01-29T16:27:34Z

xarray/tests/test_concat.py

+            ),
+        ],
+    )
+    def test_concat_combine_attrs_kwarg_variables(


Suggested change

def test_concat_combine_attrs_kwarg_variables(

def test_concat_combine_attrs_kwarg_dataarrays(

or data_vars (took me a while to figure out what was different from above)

thanks, I forgot to check the coords. The purpose of these tests is to check whether combine_attrs is applied to the variables (coords and data variables) in addition to the top-level Dataset / DataArray. Would it help to have a comment explaining the difference?

mathause · 2021-01-29T16:28:32Z

xarray/tests/test_merge.py

+            ),
+        ],
+    )
+    def test_merge_arrays_attrs_variables(


xarray/core/merge.py

dcherian · 2021-02-10T19:50:10Z

Thanks @keewis

…_and_bounds_as_coords * upstream/master: (51 commits) Ensure maximum accuracy when encoding and decoding cftime.datetime values (pydata#4758) Fix `bounds_error=True` ignored with 1D interpolation (pydata#4855) add a drop_conflicts strategy for merging attrs (pydata#4827) update pre-commit hooks (mypy) (pydata#4883) ensure warnings cannot become errors in assert_ (pydata#4864) update pre-commit hooks (pydata#4874) small fixes for the docstrings of swap_dims and integrate (pydata#4867) Modify _encode_datetime_with_cftime for compatibility with cftime > 1.4.0 (pydata#4871) vélin (pydata#4872) don't skip the doctests CI (pydata#4869) fix da.pad example for numpy 1.20 (pydata#4865) temporarily pin dask (pydata#4873) Add units if "unit" is in the attrs. (pydata#4850) speed up the repr for big MultiIndex objects (pydata#4846) dim -> coord in DataArray.integrate (pydata#3993) WIP: backend interface, now it uses subclassing (pydata#4836) weighted: small improvements (pydata#4818) Update related-projects.rst (pydata#4844) iris update doc url (pydata#4845) Faster unstacking (pydata#4746) ...

* upstream/master: (24 commits) Compatibility with dask 2021.02.0 (pydata#4884) Ensure maximum accuracy when encoding and decoding cftime.datetime values (pydata#4758) Fix `bounds_error=True` ignored with 1D interpolation (pydata#4855) add a drop_conflicts strategy for merging attrs (pydata#4827) update pre-commit hooks (mypy) (pydata#4883) ensure warnings cannot become errors in assert_ (pydata#4864) update pre-commit hooks (pydata#4874) small fixes for the docstrings of swap_dims and integrate (pydata#4867) Modify _encode_datetime_with_cftime for compatibility with cftime > 1.4.0 (pydata#4871) vélin (pydata#4872) don't skip the doctests CI (pydata#4869) fix da.pad example for numpy 1.20 (pydata#4865) temporarily pin dask (pydata#4873) Add units if "unit" is in the attrs. (pydata#4850) speed up the repr for big MultiIndex objects (pydata#4846) dim -> coord in DataArray.integrate (pydata#3993) WIP: backend interface, now it uses subclassing (pydata#4836) weighted: small improvements (pydata#4818) Update related-projects.rst (pydata#4844) iris update doc url (pydata#4845) ...

keewis added 15 commits January 19, 2021 02:02

add the drop_conflicts option for merging attrs

767a668

mention drop_conflicts in a few docstrings

909c5cd

changed functions: concat, merge, combine_by_coords, combine_nested, merge_core

add a test for merge_attrs

d35a213

rewrite the drop_conflicts algorithm

7f9481c

verify that drop_conflicts works correctly when passed to concat

1930d68

add a test for combine_nested with combine_attrs="drop_conflicts"

a14a4d9

remove test_merge_attrs in favor of test_merge_arrays_attrs

edc8428

use a dict comprehension instead of a dict intersection

9344b93

reorganize the drop_conflicts concat test

1791111

use different data for the drop_conflicts merge test

2ec123f

add another test to make sure attrs on variables is also merged

fad94a0

correctly set expect_exception

86c7c5f

skip the combine_attrs merge variables test

3531198

rewrite the concat combine_attrs test and use different data

be5ad6a

also skip the concat combine_attrs variables test

5a6b551

max-sixty approved these changes Jan 24, 2021

View reviewed changes

keewis added 3 commits January 24, 2021 13:56

use utils.equivalent instead of == so ndarray objects can be compared

af72603

make sure conflicting keys cannot be added back by other objects

dbc510a

don't compare the value with itself if key is not in attrs

344d822

mathause reviewed Jan 29, 2021

View reviewed changes

keewis added 5 commits February 5, 2021 02:30

also check the coordinates

6ed88f7

add docstrings to explain the purpose of the tests

f43c0be

update whats-new.rst

ac3c102

add a test for combine_attrs="drop_conflicts" with more than two attrs

ea24b32

Merge branch 'master' into keep_attrs-drop_conflicts

5dd73be

dcherian reviewed Feb 5, 2021

View reviewed changes

xarray/core/merge.py Show resolved Hide resolved

Merge branch 'master' into keep_attrs-drop_conflicts

81c8271

Merge branch 'master' into keep_attrs-drop_conflicts

4a4faa5

dcherian approved these changes Feb 10, 2021

View reviewed changes

dcherian merged commit 47889ee into pydata:master Feb 10, 2021

keewis deleted the keep_attrs-drop_conflicts branch February 10, 2021 21:34

keewis mentioned this pull request Feb 11, 2021

add a combine_attrs parameter to Dataset.merge #4895

Merged

4 tasks

keewis mentioned this pull request Feb 12, 2021

also apply combine_attrs to the attrs of the variables #4902

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a drop_conflicts strategy for merging attrs #4827

add a drop_conflicts strategy for merging attrs #4827

keewis commented Jan 19, 2021 •

edited

Loading

keewis commented Jan 23, 2021

keewis commented Jan 23, 2021

keewis commented Jan 23, 2021

max-sixty left a comment

mathause left a comment

mathause Jan 29, 2021

keewis Feb 5, 2021 •

edited

Loading

mathause Jan 29, 2021

dcherian commented Feb 10, 2021

	def test_concat_combine_attrs_kwarg_variables(
	def test_concat_combine_attrs_kwarg_dataarrays(

add a drop_conflicts strategy for merging attrs #4827

add a drop_conflicts strategy for merging attrs #4827

Conversation

keewis commented Jan 19, 2021 • edited Loading

Overriding CI behaviors

keewis commented Jan 23, 2021

keewis commented Jan 23, 2021

keewis commented Jan 23, 2021

max-sixty left a comment

Choose a reason for hiding this comment

mathause left a comment

Choose a reason for hiding this comment

mathause Jan 29, 2021

Choose a reason for hiding this comment

keewis Feb 5, 2021 • edited Loading

Choose a reason for hiding this comment

mathause Jan 29, 2021

Choose a reason for hiding this comment

dcherian commented Feb 10, 2021

keewis commented Jan 19, 2021 •

edited

Loading

keewis Feb 5, 2021 •

edited

Loading