Fix aligned index variable metadata side effect

[benbovy]

• Closes Testing DataArray equality using built-in '==' operator leads to mutilated DataArray.attrs dictionary #6852
• Tests added
• User visible changes (including notable bug fixes) are documented in whats-new.rst

[keewis]

thanks for the quick fix, @benbovy.

[keewis]

would it make sense to also try this the other way round? As far as I understand #6852, the order mattered?

Edit: changing the order results in actual_noattr.x.attrs to be equal to actual.x.attrs.

This is probably for a separate issue, but I'm not sure what to do with differing attrs on aligned coordinates: should they stay the same as before the align, or should we use merge_attrs? At the moment, the result is hard-coded to "override", which might not be desirable.

[keewis]

Actually, I might be misunderstanding something, but doesn't this check a different issue? The point of #6852 was not that the result was wrong, but that one of the operands was modified. To check that, we'd need different asserts:

        assert ds.x.attrs == {"units": "m"}
        assert ds_noattr.x.attrs == {}

(this PR still fixes that issue, though)

[benbovy]

The point of #6852 was not that the result was wrong, but that one of the operands was modified.

Yes, you're right! The asserts you suggest are the right ones. I'll update it.

would it make sense to also try this the other way round?

No I think it would be redundant. With xr.align(ds, ds_noattr), ds.x has no attribute anymore and with xr.align(ds_noattr, ds), ds_noattr.x has a new attribute "units", so it's different but the side effect is the same (i.e., unwanted update of the attributes of the "x" coordinate of the 1st object, which is here picked up as the aligned "x" coordinate variable).

[keewis]

No I think it would be redundant

that was referring to the attrs of the resulting datasets (i.e. unrelated to #6852) so I agree that that would be redundant. I'll open a new issue to discuss my question.

[benbovy]

What is the issue with the attrs in the resulting datasets? The asserts on the results all pass with and without this fix:

assert actual.x.attrs == {"units": "m"}
assert actual_noattr.x.attrs == {}

[benbovy]

Hmm that's weird, the example in your last comment fails both in main and this PR for me.

[keewis]

not sure if that's a environment issue, but this is what I've been using to test:

import pytest
import xarray as xr


@pytest.mark.parametrize("join", ["left", "override"])
def test_align_index_var_attrs(join) -> None:
    ds = xr.Dataset(coords={"x": ("x", [1, 2, 3], {"units": "m"})})
    ds_noattr = xr.Dataset(coords={"x": ("x", [1, 2, 3])})
    actual, actual_noattr = xr.align(ds, ds_noattr, join=join)
    assert ds.x.attrs == {"units": "m"}
    assert ds_noattr.x.attrs == {}
    assert actual.x.attrs == {"units": "m"}
    assert actual_noattr.x.attrs == {}

join="left" fails on main because ds.x.attrs is cleared, but join="override" fails with:

________________________ test_align_index_var_attrs[override] ________________________

join = 'override'

    @pytest.mark.parametrize("join", ["left", "override"])
    def test_align_index_var_attrs(join) -> None:
        ds = xr.Dataset(coords={"x": ("x", [1, 2, 3], {"units": "m"})})
        ds_noattr = xr.Dataset(coords={"x": ("x", [1, 2, 3])})
        actual, actual_noattr = xr.align(ds, ds_noattr, join=join)
        assert ds.x.attrs == {"units": "m"}
        assert ds_noattr.x.attrs == {}
        assert actual.x.attrs == {"units": "m"}
>       assert actual_noattr.x.attrs == {}
E       AssertionError: assert {'units': 'm'} == {}
E         Left contains 1 more item:
E         {'units': 'm'}
E         Use -v to get more diff

.../test_issue6852.py:13: AssertionError

[keewis]

I'm probably misunderstanding what align with join="override" is doing, though

[benbovy]

Ah yes that makes sense: we need to clarify what join="override" means. Does it mean only override the index or does it mean override the index variables (i.e., their metadata) too? Currently it's the latter but I'm not sure this is what we want. Then indeed it is probably worth opening an issue.

[benbovy]

After thinking more about it, I think it is a bug. align shouldn't update any metadata, even with join="override". I opened #6860.