also apply combine_attrs to the attrs of the variables #4902
Conversation
|
I tried to fix the tests but the remaining failures in |
| @@ -272,7 +282,7 @@ def test_merge_no_conflicts_multi_var(self): | |||
|
|
|||
| def test_merge_no_conflicts_preserve_attrs(self): | |||
| data = xr.Dataset({"x": ([], 0, {"foo": "bar"})}) | |||
| actual = xr.merge([data, data]) | |||
| actual = xr.merge([data, data], combine_attrs="no_conflicts") | |||
There was a problem hiding this comment.
does this make sense? I think the no_conflicts refers to compat and not combine_attrs, but otherwise this will keep failing.
#4749 suggested to change the default merge strategy to drop_conflicts, so we could revert all these tiny test changes after that (but maybe we need to deprecate the current default first?)
There was a problem hiding this comment.
is the old default "override"?
There was a problem hiding this comment.
yes. This is done by compat, which delegates to fillna, which in turn calls apply_ufunc. This raises the bigger question whether compat should affect attrs, or whether it would be better to completely separate compat from combine_attrs / keep_attrs.
There was a problem hiding this comment.
actually, the result of compat seems to always be to either raise for conflicting attributes ("identical") or to keep the first object's attrs ("override"). Completely separating compat won't be easy because compat="identical" will still raise. Not sure what to do about that – should we deprecate it?
Edit: it seems that conflict is fine, compat="identical" can override any value for combine_attrs
|
I just noticed this would do the same as #4017, which suggests merging attrs in |
|
I've got a few small questions left (see the comments), but otherwise this should be ready for reviewing |
| ): | ||
| """check that combine_attrs is used on data variables and coords""" | ||
| data1 = Dataset( | ||
| {"x": ("a", [0], attrs1), "y": ("a", [0], attrs1), "a": ("a", [0], attrs1)} |
There was a problem hiding this comment.
should we also check for Dataset.attrs?
There was a problem hiding this comment.
I had thought we already did that. Turns out we don't, which I didn't realize because that file is in urgent need of a refactor: some test names still reference the removed auto_combine, combine_by_coords and combine_nested are ordered in a way I can't figure out, and I accidentally contributed to the chaos by adding all new tests to TestCombineAuto. Since this seems like a bigger effort I'll try to send in a PR which cleans this up.
Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
once the backwards compatibility issue has been resolved this should be ready for merging |
|
once CI passes this should be ready for merging. |
|
Thanks @keewis |
Follow-up to #4827. The current behavior is to always combine with
combine_attrs="override", even if the caller requested something else.pre-commit run --all-fileswhats-new.rst