From 5affeb7a535ac78645f1b3740af126762dcf8104 Mon Sep 17 00:00:00 2001 From: Giacomo Caria <44147817+gcaria@users.noreply.github.com> Date: Mon, 8 Nov 2021 23:28:31 +0100 Subject: [PATCH 1/3] Fix bug and add test --- xarray/core/variable.py | 2 ++ xarray/tests/test_dataarray.py | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/xarray/core/variable.py b/xarray/core/variable.py index a96adb31e64..64b73bfeacb 100644 --- a/xarray/core/variable.py +++ b/xarray/core/variable.py @@ -2418,6 +2418,8 @@ def _inplace_binary_op(self, other, f): if dims != self.dims: raise ValueError("dimensions cannot change for in-place operations") with np.errstate(all="ignore"): + if isinstance(self_data, np.ndarray): + self.values = f(self_data.copy(), other_data) self.values = f(self_data, other_data) return self diff --git a/xarray/tests/test_dataarray.py b/xarray/tests/test_dataarray.py index 53c650046e7..cc3f638ea1c 100644 --- a/xarray/tests/test_dataarray.py +++ b/xarray/tests/test_dataarray.py @@ -1987,6 +1987,16 @@ def test_inplace_math_basics(self): assert_array_equal(b.values, x) assert source_ndarray(b.values) is x + def test_inplace_math_error(self): + data = np.random.rand(4) + times = np.arange(4) + foo = DataArray(data, coords=[times], dims=["time"]) + b = times.copy() + with pytest.raises(ValueError, match=r"Cannot assign to the .values"): + foo.coords["time"] += 1 + # Check error throwing prevented inplace operation + assert_array_equal(foo.coords["time"], b) + def test_inplace_math_automatic_alignment(self): a = DataArray(range(5), [("x", range(5))]) b = DataArray(range(1, 6), [("x", range(1, 6))]) From 48c6ee2242649caf8441cf759c6338bc44ae1fa1 Mon Sep 17 00:00:00 2001 From: Giacomo Caria <44147817+gcaria@users.noreply.github.com> Date: Mon, 8 Nov 2021 23:59:28 +0100 Subject: [PATCH 2/3] Add comment --- xarray/core/variable.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/xarray/core/variable.py b/xarray/core/variable.py index 64b73bfeacb..3b2f5b4638e 100644 --- a/xarray/core/variable.py +++ b/xarray/core/variable.py @@ -2418,6 +2418,8 @@ def _inplace_binary_op(self, other, f): if dims != self.dims: raise ValueError("dimensions cannot change for in-place operations") with np.errstate(all="ignore"): + # Give a chance to Variable.values.setter to throw error if needed + # without updating self_data inplace if isinstance(self_data, np.ndarray): self.values = f(self_data.copy(), other_data) self.values = f(self_data, other_data) From b3541198d9a489aecf80e889aa1681c6210da059 Mon Sep 17 00:00:00 2001 From: Giacomo Caria <44147817+gcaria@users.noreply.github.com> Date: Tue, 9 Nov 2021 19:53:35 +0100 Subject: [PATCH 3/3] Edit only _inplace_binary_op --- xarray/core/variable.py | 9 +++++---- xarray/tests/test_dataarray.py | 4 +++- xarray/tests/test_variable.py | 8 ++++++++ 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/xarray/core/variable.py b/xarray/core/variable.py index 3b2f5b4638e..52125ec4113 100644 --- a/xarray/core/variable.py +++ b/xarray/core/variable.py @@ -2418,10 +2418,6 @@ def _inplace_binary_op(self, other, f): if dims != self.dims: raise ValueError("dimensions cannot change for in-place operations") with np.errstate(all="ignore"): - # Give a chance to Variable.values.setter to throw error if needed - # without updating self_data inplace - if isinstance(self_data, np.ndarray): - self.values = f(self_data.copy(), other_data) self.values = f(self_data, other_data) return self @@ -2817,6 +2813,11 @@ def name(self): def name(self, value): raise AttributeError("cannot modify name of IndexVariable in-place") + def _inplace_binary_op(self, other, f): + raise TypeError( + "Values of an IndexVariable are immutable and can not be modified inplace" + ) + # for backwards compatibility Coordinate = utils.alias(IndexVariable, "Coordinate") diff --git a/xarray/tests/test_dataarray.py b/xarray/tests/test_dataarray.py index cc3f638ea1c..5e9c1b87ce2 100644 --- a/xarray/tests/test_dataarray.py +++ b/xarray/tests/test_dataarray.py @@ -1992,7 +1992,9 @@ def test_inplace_math_error(self): times = np.arange(4) foo = DataArray(data, coords=[times], dims=["time"]) b = times.copy() - with pytest.raises(ValueError, match=r"Cannot assign to the .values"): + with pytest.raises( + TypeError, match=r"Values of an IndexVariable are immutable" + ): foo.coords["time"] += 1 # Check error throwing prevented inplace operation assert_array_equal(foo.coords["time"], b) diff --git a/xarray/tests/test_variable.py b/xarray/tests/test_variable.py index 9c0e45c5da9..3267af8b45b 100644 --- a/xarray/tests/test_variable.py +++ b/xarray/tests/test_variable.py @@ -1656,6 +1656,14 @@ def test_inplace_math(self): with pytest.raises(ValueError, match=r"dimensions cannot change"): v += Variable("y", np.arange(5)) + def test_inplace_math_error(self): + x = np.arange(5) + v = IndexVariable(["x"], x) + with pytest.raises( + TypeError, match=r"Values of an IndexVariable are immutable" + ): + v += 1 + def test_reduce(self): v = Variable(["x", "y"], self.d, {"ignored": "attributes"}) assert_identical(v.reduce(np.std, "x"), Variable(["y"], self.d.std(axis=0)))