Fix merge override attrs reference issue - #11511
Open
nocdoggo wants to merge 3 commits into
Open
Conversation
…hen combine_attrs='override', the merge function\ncreates a copy of the attrs dictionary rather than a reference. This ensures\nthat modifications to the merged dataset's attrs do not affect the original\ndataset's attrs.\n\nAlso includes a test for empty input cases.
|
Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an attrs aliasing bug in merge_attrs() when combine_attrs="override" by returning a copy of the first attrs mapping rather than the original object, and attempts to add regression tests to ensure merged results don’t share attrs references with inputs.
Changes:
- Update
merge_attrs(..., "override")to returndict(variable_attrs[0])(copy) instead ofvariable_attrs[0](reference). - Add new tests intended to verify
overridereturns independentattrsobjects and handles empty/single-input cases. - Restructure merge tests (currently replacing the prior
test_merge.pycontents with a minimal subset).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| xarray/core/merge.py | Copy attrs in override mode to avoid returning a shared reference. |
| xarray/xarray/tests/test_merge_attrs.py | Adds override attrs-copy tests, but currently placed in a likely non-collected path and contains incorrect expectations. |
| xarray/tests/test_merge.py | Adds override attrs-copy tests, but currently removes the prior merge test suite (major coverage loss). |
Suppressed comments (4)
xarray/xarray/tests/test_merge_attrs.py:20
combine_attrs='override'keeps the first object's attrs only (see existing semantics inmerge_attrs), so the merged dataset should not pick up key'd'from the second dataset. As written, this assertion will fail once the test is moved into the collected test tree.
assert xds3.attrs['a'] == 'b' # Should take value from first dataset
assert xds3.attrs['b'] == 'c'
assert xds3.attrs['d'] == 'e'
xarray/xarray/tests/test_merge_attrs.py:35
merge_attrsreturnsNonewhen given an emptyvariable_attrslist (independent ofcombine_attrs). This test currently expects{}, which doesn’t match the actual contract and will fail.
# Test with empty inputs
assert xr.core.merge.merge_attrs([], 'override') == {}
assert xr.core.merge.merge_attrs([{}], 'override') == {}
xarray/xarray/tests/test_merge_attrs.py:2
pytestis imported but never used in this module; this will fail linting (ruff/flake8) and should be removed.
import pytest
import xarray as xr
xarray/tests/test_merge.py:3
pytestis imported but never used in this test module; this will fail linting and should be removed.
import pytest
import xarray as xr
from xarray.core.merge import merge_attrs
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1
to
+5
| import pytest | ||
| import xarray as xr | ||
|
|
||
| def test_merge_override_attrs_copy(): | ||
| """Test that merge with combine_attrs='override' creates a copy of attrs""" |
Comment on lines
1
to
+5
| import pytest | ||
|
|
||
| import xarray as xr | ||
| from xarray.core import dtypes, merge | ||
| from xarray.core.merge import MergeError | ||
| from xarray.testing import assert_identical | ||
|
|
||
| from . import raises_regex | ||
| from .test_dataset import create_test_data | ||
|
|
||
|
|
||
| class TestMergeInternals: | ||
| def test_broadcast_dimension_size(self): | ||
| actual = merge.broadcast_dimension_size( | ||
| [xr.Variable("x", [1]), xr.Variable("y", [2, 1])] | ||
| ) | ||
| assert actual == {"x": 1, "y": 2} | ||
|
|
||
| actual = merge.broadcast_dimension_size( | ||
| [xr.Variable(("x", "y"), [[1, 2]]), xr.Variable("y", [2, 1])] | ||
| ) | ||
| assert actual == {"x": 1, "y": 2} | ||
|
|
||
| with pytest.raises(ValueError): | ||
| merge.broadcast_dimension_size( | ||
| [xr.Variable(("x", "y"), [[1, 2]]), xr.Variable("y", [2])] | ||
| ) | ||
|
|
||
|
|
||
| class TestMergeFunction: | ||
| def test_merge_arrays(self): | ||
| data = create_test_data() | ||
| actual = xr.merge([data.var1, data.var2]) | ||
| expected = data[["var1", "var2"]] | ||
| assert actual.identical(expected) | ||
|
|
||
| def test_merge_datasets(self): | ||
| data = create_test_data() | ||
|
|
||
| actual = xr.merge([data[["var1"]], data[["var2"]]]) | ||
| expected = data[["var1", "var2"]] | ||
| assert actual.identical(expected) | ||
|
|
||
| actual = xr.merge([data, data]) | ||
| assert actual.identical(data) | ||
|
|
||
| def test_merge_dataarray_unnamed(self): | ||
| data = xr.DataArray([1, 2], dims="x") | ||
| with raises_regex(ValueError, "without providing an explicit name"): | ||
| xr.merge([data]) | ||
|
|
||
| def test_merge_arrays_attrs_default(self): | ||
| var1_attrs = {"a": 1, "b": 2} | ||
| var2_attrs = {"a": 1, "c": 3} | ||
| expected_attrs = {} | ||
|
|
||
| data = create_test_data() | ||
| data.var1.attrs = var1_attrs | ||
| data.var2.attrs = var2_attrs | ||
| actual = xr.merge([data.var1, data.var2]) | ||
| expected = data[["var1", "var2"]] | ||
| expected.attrs = expected_attrs | ||
| assert actual.identical(expected) | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "combine_attrs, var1_attrs, var2_attrs, expected_attrs, expect_exception", | ||
| [ | ||
| ( | ||
| "no_conflicts", | ||
| {"a": 1, "b": 2}, | ||
| {"a": 1, "c": 3}, | ||
| {"a": 1, "b": 2, "c": 3}, | ||
| False, | ||
| ), | ||
| ("no_conflicts", {"a": 1, "b": 2}, {}, {"a": 1, "b": 2}, False), | ||
| ("no_conflicts", {}, {"a": 1, "c": 3}, {"a": 1, "c": 3}, False), | ||
| ( | ||
| "no_conflicts", | ||
| {"a": 1, "b": 2}, | ||
| {"a": 4, "c": 3}, | ||
| {"a": 1, "b": 2, "c": 3}, | ||
| True, | ||
| ), | ||
| ("drop", {"a": 1, "b": 2}, {"a": 1, "c": 3}, {}, False), | ||
| ("identical", {"a": 1, "b": 2}, {"a": 1, "b": 2}, {"a": 1, "b": 2}, False), | ||
| ("identical", {"a": 1, "b": 2}, {"a": 1, "c": 3}, {"a": 1, "b": 2}, True), | ||
| ( | ||
| "override", | ||
| {"a": 1, "b": 2}, | ||
| {"a": 4, "b": 5, "c": 3}, | ||
| {"a": 1, "b": 2}, | ||
| False, | ||
| ), | ||
| ], | ||
| ) | ||
| def test_merge_arrays_attrs( | ||
| self, combine_attrs, var1_attrs, var2_attrs, expected_attrs, expect_exception | ||
| ): | ||
| data = create_test_data() | ||
| data.var1.attrs = var1_attrs | ||
| data.var2.attrs = var2_attrs | ||
| if expect_exception: | ||
| with raises_regex(MergeError, "combine_attrs"): | ||
| actual = xr.merge([data.var1, data.var2], combine_attrs=combine_attrs) | ||
| else: | ||
| actual = xr.merge([data.var1, data.var2], combine_attrs=combine_attrs) | ||
| expected = data[["var1", "var2"]] | ||
| expected.attrs = expected_attrs | ||
| assert actual.identical(expected) | ||
|
|
||
| def test_merge_dicts_simple(self): | ||
| actual = xr.merge([{"foo": 0}, {"bar": "one"}, {"baz": 3.5}]) | ||
| expected = xr.Dataset({"foo": 0, "bar": "one", "baz": 3.5}) | ||
| assert actual.identical(expected) | ||
|
|
||
| def test_merge_dicts_dims(self): | ||
| actual = xr.merge([{"y": ("x", [13])}, {"x": [12]}]) | ||
| expected = xr.Dataset({"x": [12], "y": ("x", [13])}) | ||
| assert actual.identical(expected) | ||
|
|
||
| def test_merge_error(self): | ||
| ds = xr.Dataset({"x": 0}) | ||
| with pytest.raises(xr.MergeError): | ||
| xr.merge([ds, ds + 1]) | ||
|
|
||
| def test_merge_alignment_error(self): | ||
| ds = xr.Dataset(coords={"x": [1, 2]}) | ||
| other = xr.Dataset(coords={"x": [2, 3]}) | ||
| with raises_regex(ValueError, "indexes .* not equal"): | ||
| xr.merge([ds, other], join="exact") | ||
|
|
||
| def test_merge_wrong_input_error(self): | ||
| with raises_regex(TypeError, "objects must be an iterable"): | ||
| xr.merge([1]) | ||
| ds = xr.Dataset(coords={"x": [1, 2]}) | ||
| with raises_regex(TypeError, "objects must be an iterable"): | ||
| xr.merge({"a": ds}) | ||
| with raises_regex(TypeError, "objects must be an iterable"): | ||
| xr.merge([ds, 1]) | ||
|
|
||
| def test_merge_no_conflicts_single_var(self): | ||
| ds1 = xr.Dataset({"a": ("x", [1, 2]), "x": [0, 1]}) | ||
| ds2 = xr.Dataset({"a": ("x", [2, 3]), "x": [1, 2]}) | ||
| expected = xr.Dataset({"a": ("x", [1, 2, 3]), "x": [0, 1, 2]}) | ||
| assert expected.identical(xr.merge([ds1, ds2], compat="no_conflicts")) | ||
| assert expected.identical(xr.merge([ds2, ds1], compat="no_conflicts")) | ||
| assert ds1.identical(xr.merge([ds1, ds2], compat="no_conflicts", join="left")) | ||
| assert ds2.identical(xr.merge([ds1, ds2], compat="no_conflicts", join="right")) | ||
| expected = xr.Dataset({"a": ("x", [2]), "x": [1]}) | ||
| assert expected.identical( | ||
| xr.merge([ds1, ds2], compat="no_conflicts", join="inner") | ||
| ) | ||
|
|
||
| with pytest.raises(xr.MergeError): | ||
| ds3 = xr.Dataset({"a": ("x", [99, 3]), "x": [1, 2]}) | ||
| xr.merge([ds1, ds3], compat="no_conflicts") | ||
|
|
||
| with pytest.raises(xr.MergeError): | ||
| ds3 = xr.Dataset({"a": ("y", [2, 3]), "y": [1, 2]}) | ||
| xr.merge([ds1, ds3], compat="no_conflicts") | ||
|
|
||
| def test_merge_no_conflicts_multi_var(self): | ||
| data = create_test_data() | ||
| data1 = data.copy(deep=True) | ||
| data2 = data.copy(deep=True) | ||
|
|
||
| expected = data[["var1", "var2"]] | ||
| actual = xr.merge([data1.var1, data2.var2], compat="no_conflicts") | ||
| assert expected.identical(actual) | ||
|
|
||
| data1["var1"][:, :5] = np.nan | ||
| data2["var1"][:, 5:] = np.nan | ||
| data1["var2"][:4, :] = np.nan | ||
| data2["var2"][4:, :] = np.nan | ||
| del data2["var3"] | ||
|
|
||
| actual = xr.merge([data1, data2], compat="no_conflicts") | ||
| assert data.equals(actual) | ||
|
|
||
| def test_merge_no_conflicts_preserve_attrs(self): | ||
| data = xr.Dataset({"x": ([], 0, {"foo": "bar"})}) | ||
| actual = xr.merge([data, data]) | ||
| assert data.identical(actual) | ||
|
|
||
| def test_merge_no_conflicts_broadcast(self): | ||
| datasets = [xr.Dataset({"x": ("y", [0])}), xr.Dataset({"x": np.nan})] | ||
| actual = xr.merge(datasets) | ||
| expected = xr.Dataset({"x": ("y", [0])}) | ||
| assert expected.identical(actual) | ||
|
|
||
| datasets = [xr.Dataset({"x": ("y", [np.nan])}), xr.Dataset({"x": 0})] | ||
| actual = xr.merge(datasets) | ||
| assert expected.identical(actual) | ||
|
|
||
|
|
||
| class TestMergeMethod: | ||
| def test_merge(self): | ||
| data = create_test_data() | ||
| ds1 = data[["var1"]] | ||
| ds2 = data[["var3"]] | ||
| expected = data[["var1", "var3"]] | ||
| actual = ds1.merge(ds2) | ||
| assert expected.identical(actual) | ||
|
|
||
| actual = ds2.merge(ds1) | ||
| assert expected.identical(actual) | ||
|
|
||
| actual = data.merge(data) | ||
| assert data.identical(actual) | ||
| actual = data.reset_coords(drop=True).merge(data) | ||
| assert data.identical(actual) | ||
| actual = data.merge(data.reset_coords(drop=True)) | ||
| assert data.identical(actual) | ||
|
|
||
| with pytest.raises(ValueError): | ||
| ds1.merge(ds2.rename({"var3": "var1"})) | ||
| with raises_regex(ValueError, "should be coordinates or not"): | ||
| data.reset_coords().merge(data) | ||
| with raises_regex(ValueError, "should be coordinates or not"): | ||
| data.merge(data.reset_coords()) | ||
|
|
||
| def test_merge_broadcast_equals(self): | ||
| ds1 = xr.Dataset({"x": 0}) | ||
| ds2 = xr.Dataset({"x": ("y", [0, 0])}) | ||
| actual = ds1.merge(ds2) | ||
| assert ds2.identical(actual) | ||
|
|
||
| actual = ds2.merge(ds1) | ||
| assert ds2.identical(actual) | ||
|
|
||
| actual = ds1.copy() | ||
| actual.update(ds2) | ||
| assert ds2.identical(actual) | ||
|
|
||
| ds1 = xr.Dataset({"x": np.nan}) | ||
| ds2 = xr.Dataset({"x": ("y", [np.nan, np.nan])}) | ||
| actual = ds1.merge(ds2) | ||
| assert ds2.identical(actual) | ||
|
|
||
| def test_merge_compat(self): | ||
| ds1 = xr.Dataset({"x": 0}) | ||
| ds2 = xr.Dataset({"x": 1}) | ||
| for compat in ["broadcast_equals", "equals", "identical", "no_conflicts"]: | ||
| with pytest.raises(xr.MergeError): | ||
| ds1.merge(ds2, compat=compat) | ||
|
|
||
| ds2 = xr.Dataset({"x": [0, 0]}) | ||
| for compat in ["equals", "identical"]: | ||
| with raises_regex(ValueError, "should be coordinates or not"): | ||
| ds1.merge(ds2, compat=compat) | ||
|
|
||
| ds2 = xr.Dataset({"x": ((), 0, {"foo": "bar"})}) | ||
| with pytest.raises(xr.MergeError): | ||
| ds1.merge(ds2, compat="identical") | ||
|
|
||
| with raises_regex(ValueError, "compat=.* invalid"): | ||
| ds1.merge(ds2, compat="foobar") | ||
|
|
||
| assert ds1.identical(ds1.merge(ds2, compat="override")) | ||
|
|
||
| def test_merge_auto_align(self): | ||
| ds1 = xr.Dataset({"a": ("x", [1, 2]), "x": [0, 1]}) | ||
| ds2 = xr.Dataset({"b": ("x", [3, 4]), "x": [1, 2]}) | ||
| expected = xr.Dataset( | ||
| {"a": ("x", [1, 2, np.nan]), "b": ("x", [np.nan, 3, 4])}, {"x": [0, 1, 2]} | ||
| ) | ||
| assert expected.identical(ds1.merge(ds2)) | ||
| assert expected.identical(ds2.merge(ds1)) | ||
|
|
||
| expected = expected.isel(x=slice(2)) | ||
| assert expected.identical(ds1.merge(ds2, join="left")) | ||
| assert expected.identical(ds2.merge(ds1, join="right")) | ||
|
|
||
| expected = expected.isel(x=slice(1, 2)) | ||
| assert expected.identical(ds1.merge(ds2, join="inner")) | ||
| assert expected.identical(ds2.merge(ds1, join="inner")) | ||
|
|
||
| @pytest.mark.parametrize("fill_value", [dtypes.NA, 2, 2.0, {"a": 2, "b": 1}]) | ||
| def test_merge_fill_value(self, fill_value): | ||
| ds1 = xr.Dataset({"a": ("x", [1, 2]), "x": [0, 1]}) | ||
| ds2 = xr.Dataset({"b": ("x", [3, 4]), "x": [1, 2]}) | ||
| if fill_value == dtypes.NA: | ||
| # if we supply the default, we expect the missing value for a | ||
| # float array | ||
| fill_value_a = fill_value_b = np.nan | ||
| elif isinstance(fill_value, dict): | ||
| fill_value_a = fill_value["a"] | ||
| fill_value_b = fill_value["b"] | ||
| else: | ||
| fill_value_a = fill_value_b = fill_value | ||
|
|
||
| expected = xr.Dataset( | ||
| {"a": ("x", [1, 2, fill_value_a]), "b": ("x", [fill_value_b, 3, 4])}, | ||
| {"x": [0, 1, 2]}, | ||
| ) | ||
| assert expected.identical(ds1.merge(ds2, fill_value=fill_value)) | ||
| assert expected.identical(ds2.merge(ds1, fill_value=fill_value)) | ||
| assert expected.identical(xr.merge([ds1, ds2], fill_value=fill_value)) | ||
|
|
||
| def test_merge_no_conflicts(self): | ||
| ds1 = xr.Dataset({"a": ("x", [1, 2]), "x": [0, 1]}) | ||
| ds2 = xr.Dataset({"a": ("x", [2, 3]), "x": [1, 2]}) | ||
| expected = xr.Dataset({"a": ("x", [1, 2, 3]), "x": [0, 1, 2]}) | ||
|
|
||
| assert expected.identical(ds1.merge(ds2, compat="no_conflicts")) | ||
| assert expected.identical(ds2.merge(ds1, compat="no_conflicts")) | ||
|
|
||
| assert ds1.identical(ds1.merge(ds2, compat="no_conflicts", join="left")) | ||
|
|
||
| assert ds2.identical(ds1.merge(ds2, compat="no_conflicts", join="right")) | ||
|
|
||
| expected2 = xr.Dataset({"a": ("x", [2]), "x": [1]}) | ||
| assert expected2.identical(ds1.merge(ds2, compat="no_conflicts", join="inner")) | ||
|
|
||
| with pytest.raises(xr.MergeError): | ||
| ds3 = xr.Dataset({"a": ("x", [99, 3]), "x": [1, 2]}) | ||
| ds1.merge(ds3, compat="no_conflicts") | ||
|
|
||
| with pytest.raises(xr.MergeError): | ||
| ds3 = xr.Dataset({"a": ("y", [2, 3]), "y": [1, 2]}) | ||
| ds1.merge(ds3, compat="no_conflicts") | ||
|
|
||
| def test_merge_dataarray(self): | ||
| ds = xr.Dataset({"a": 0}) | ||
| da = xr.DataArray(data=1, name="b") | ||
|
|
||
| assert_identical(ds.merge(da), xr.merge([ds, da])) | ||
| from xarray.core.merge import merge_attrs | ||
|
|
||
| def test_merge_override_attrs_copy(): |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When combine_attrs='override', the merge function was returning a reference to the first object's attrs dictionary instead of a copy. This meant that modifications to the merged object's attrs would affect the original object.\n\nThis change makes merge_attrs return dict(variable_attrs[0]) instead of variable_attrs[0] to ensure a proper copy is made, consistent with other combine_attrs modes like 'no_conflicts' which already create copies.\n\nAlso adds a test to verify that when combine_attrs='override', the merge function creates a copy of the attrs dictionary rather than a reference. This ensures that modifications to the merged dataset's attrs do not affect the original dataset's attrs.\n\nFixes #4629