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") | |||
keewis
Feb 13, 2021
Author
Collaborator
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?)
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?)
dcherian
Apr 5, 2021
Contributor
is the old default "override"?
is the old default "override"?
keewis
Apr 19, 2021
Author
Collaborator
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.
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.
keewis
Apr 19, 2021
•
Author
Collaborator
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
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)} |
dcherian
Apr 5, 2021
Contributor
should we also check for Dataset.attrs?
should we also check for Dataset.attrs?
keewis
Apr 19, 2021
•
Author
Collaborator
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.
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 hidden.
This comment has been hidden.
|
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