From 90e29550757db00685d459fe67a65e08fee4b7c0 Mon Sep 17 00:00:00 2001 From: "tobias.pitters" Date: Wed, 29 Jun 2022 18:27:07 +0200 Subject: [PATCH 01/12] fix pre-commit issues --- pandas/core/computation/eval.py | 7 ++++++- pandas/tests/computation/test_eval.py | 8 ++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/pandas/core/computation/eval.py b/pandas/core/computation/eval.py index d82cc37b90ad4..6a9d4330337d2 100644 --- a/pandas/core/computation/eval.py +++ b/pandas/core/computation/eval.py @@ -20,6 +20,7 @@ from pandas.core.computation.scope import ensure_scope from pandas.io.formats.printing import pprint_thing +import numpy as np def _check_engine(engine: str | None) -> str: @@ -384,7 +385,11 @@ def eval( try: with warnings.catch_warnings(record=True): # TODO: Filter the warnings we actually care about here. - target[assigner] = ret + if inplace is True and hasattr(target, 'columns') and assigner in target.columns: + loc = target.columns.get_loc(assigner) + target._iset_item_mgr(loc, np.array(ret), inplace=inplace) + else: + target[assigner] = ret except (TypeError, IndexError) as err: raise ValueError("Cannot assign expression output to target") from err diff --git a/pandas/tests/computation/test_eval.py b/pandas/tests/computation/test_eval.py index b0ad2f69a75b9..8bc557d3646c0 100644 --- a/pandas/tests/computation/test_eval.py +++ b/pandas/tests/computation/test_eval.py @@ -1890,6 +1890,14 @@ def test_negate_lt_eq_le(engine, parser): tm.assert_frame_equal(result, expected) +def test_set_inplace(): + # GH 47449 + df = DataFrame({"A": range(1, 6), "B": range(10, 0, -2), "C": range(11, 16)}) + expected = Series([21, 20, 19, 18, 17], index=[0,1,2,3,4], name="A") + df.eval("A = B + C", inplace=True) + tm.assert_series_equal(df["A"], expected) + + class TestValidate: @pytest.mark.parametrize("value", [1, "True", [1, 2, 3], 5.0]) def test_validate_bool_args(self, value): From 39987cfdd0e6b1f3b61f4dfe78b43fa6cd8ef9d9 Mon Sep 17 00:00:00 2001 From: "tobias.pitters" Date: Wed, 29 Jun 2022 18:35:00 +0200 Subject: [PATCH 02/12] fix linting errors --- pandas/core/computation/eval.py | 9 +++++++-- pandas/tests/computation/test_eval.py | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/pandas/core/computation/eval.py b/pandas/core/computation/eval.py index 6a9d4330337d2..d6e2dd445f7d6 100644 --- a/pandas/core/computation/eval.py +++ b/pandas/core/computation/eval.py @@ -6,6 +6,8 @@ import tokenize import warnings +import numpy as np + from pandas._libs.lib import no_default from pandas.util._exceptions import find_stack_level from pandas.util._validators import validate_bool_kwarg @@ -20,7 +22,6 @@ from pandas.core.computation.scope import ensure_scope from pandas.io.formats.printing import pprint_thing -import numpy as np def _check_engine(engine: str | None) -> str: @@ -385,7 +386,11 @@ def eval( try: with warnings.catch_warnings(record=True): # TODO: Filter the warnings we actually care about here. - if inplace is True and hasattr(target, 'columns') and assigner in target.columns: + if ( + inplace is True + and hasattr(target, "columns") + and assigner in target.columns + ): loc = target.columns.get_loc(assigner) target._iset_item_mgr(loc, np.array(ret), inplace=inplace) else: diff --git a/pandas/tests/computation/test_eval.py b/pandas/tests/computation/test_eval.py index 8bc557d3646c0..30b1953b06e28 100644 --- a/pandas/tests/computation/test_eval.py +++ b/pandas/tests/computation/test_eval.py @@ -1893,7 +1893,7 @@ def test_negate_lt_eq_le(engine, parser): def test_set_inplace(): # GH 47449 df = DataFrame({"A": range(1, 6), "B": range(10, 0, -2), "C": range(11, 16)}) - expected = Series([21, 20, 19, 18, 17], index=[0,1,2,3,4], name="A") + expected = Series([21, 20, 19, 18, 17], index=[0, 1, 2, 3, 4], name="A") df.eval("A = B + C", inplace=True) tm.assert_series_equal(df["A"], expected) From 517c9289a99a49ab716e6958cef15c0d30cd3982 Mon Sep 17 00:00:00 2001 From: "tobias.pitters" Date: Wed, 29 Jun 2022 19:03:36 +0200 Subject: [PATCH 03/12] add inplace argument to isetitem and use in eval --- pandas/core/computation/eval.py | 2 +- pandas/core/frame.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pandas/core/computation/eval.py b/pandas/core/computation/eval.py index d6e2dd445f7d6..8545dff7ef2a6 100644 --- a/pandas/core/computation/eval.py +++ b/pandas/core/computation/eval.py @@ -392,7 +392,7 @@ def eval( and assigner in target.columns ): loc = target.columns.get_loc(assigner) - target._iset_item_mgr(loc, np.array(ret), inplace=inplace) + target.isetitem(loc, np.array(ret), inplace=inplace) else: target[assigner] = ret except (TypeError, IndexError) as err: diff --git a/pandas/core/frame.py b/pandas/core/frame.py index a8c33a144378e..a198bebdd2844 100644 --- a/pandas/core/frame.py +++ b/pandas/core/frame.py @@ -3781,7 +3781,7 @@ def _get_value(self, index, col, takeable: bool = False) -> Scalar: loc = engine.get_loc(index) return series._values[loc] - def isetitem(self, loc, value) -> None: + def isetitem(self, loc, value, inplace: bool = False) -> None: """ Set the given value in the column with position 'loc'. @@ -3802,7 +3802,7 @@ def isetitem(self, loc, value) -> None: `frame[frame.columns[i]] = value`. """ arraylike = self._sanitize_column(value) - self._iset_item_mgr(loc, arraylike, inplace=False) + self._iset_item_mgr(loc, arraylike, inplace=inplace) def __setitem__(self, key, value): key = com.apply_if_callable(key, self) From c0d8502f37102f69353265a5b1586925d1b450ac Mon Sep 17 00:00:00 2001 From: "tobias.pitters" Date: Wed, 29 Jun 2022 19:12:34 +0200 Subject: [PATCH 04/12] changes due to PR discussions --- pandas/core/computation/eval.py | 9 ++------- pandas/core/frame.py | 4 ++-- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/pandas/core/computation/eval.py b/pandas/core/computation/eval.py index 8545dff7ef2a6..ffa1462e1de90 100644 --- a/pandas/core/computation/eval.py +++ b/pandas/core/computation/eval.py @@ -386,13 +386,8 @@ def eval( try: with warnings.catch_warnings(record=True): # TODO: Filter the warnings we actually care about here. - if ( - inplace is True - and hasattr(target, "columns") - and assigner in target.columns - ): - loc = target.columns.get_loc(assigner) - target.isetitem(loc, np.array(ret), inplace=inplace) + if hasattr(target, 'loc'): + target.loc[:, assigner] = ret else: target[assigner] = ret except (TypeError, IndexError) as err: diff --git a/pandas/core/frame.py b/pandas/core/frame.py index a198bebdd2844..fb7aa0fd83b15 100644 --- a/pandas/core/frame.py +++ b/pandas/core/frame.py @@ -3781,7 +3781,7 @@ def _get_value(self, index, col, takeable: bool = False) -> Scalar: loc = engine.get_loc(index) return series._values[loc] - def isetitem(self, loc, value, inplace: bool = False) -> None: + def isetitem(self, loc, value) -> None: """ Set the given value in the column with position 'loc'. @@ -3802,7 +3802,7 @@ def isetitem(self, loc, value, inplace: bool = False) -> None: `frame[frame.columns[i]] = value`. """ arraylike = self._sanitize_column(value) - self._iset_item_mgr(loc, arraylike, inplace=inplace) + self._iset_item_mgr(loc, arraylike) def __setitem__(self, key, value): key = com.apply_if_callable(key, self) From 7d2c398e1d0a1870ce8b4e6b1cd35167373b80bf Mon Sep 17 00:00:00 2001 From: "tobias.pitters" Date: Wed, 29 Jun 2022 19:13:56 +0200 Subject: [PATCH 05/12] fix issues --- pandas/core/computation/eval.py | 2 -- pandas/core/frame.py | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/pandas/core/computation/eval.py b/pandas/core/computation/eval.py index ffa1462e1de90..058662e342d9e 100644 --- a/pandas/core/computation/eval.py +++ b/pandas/core/computation/eval.py @@ -6,8 +6,6 @@ import tokenize import warnings -import numpy as np - from pandas._libs.lib import no_default from pandas.util._exceptions import find_stack_level from pandas.util._validators import validate_bool_kwarg diff --git a/pandas/core/frame.py b/pandas/core/frame.py index fb7aa0fd83b15..a8c33a144378e 100644 --- a/pandas/core/frame.py +++ b/pandas/core/frame.py @@ -3802,7 +3802,7 @@ def isetitem(self, loc, value) -> None: `frame[frame.columns[i]] = value`. """ arraylike = self._sanitize_column(value) - self._iset_item_mgr(loc, arraylike) + self._iset_item_mgr(loc, arraylike, inplace=False) def __setitem__(self, key, value): key = com.apply_if_callable(key, self) From ba32bd5ca508c0d04fd7fc22b2399c9695e84354 Mon Sep 17 00:00:00 2001 From: "tobias.pitters" Date: Wed, 29 Jun 2022 21:17:19 +0200 Subject: [PATCH 06/12] update eval --- pandas/core/computation/eval.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pandas/core/computation/eval.py b/pandas/core/computation/eval.py index 058662e342d9e..8e850106079cd 100644 --- a/pandas/core/computation/eval.py +++ b/pandas/core/computation/eval.py @@ -384,7 +384,7 @@ def eval( try: with warnings.catch_warnings(record=True): # TODO: Filter the warnings we actually care about here. - if hasattr(target, 'loc'): + if hasattr(target, "loc"): target.loc[:, assigner] = ret else: target[assigner] = ret From cead80fae73e8241eb937030d8af89f9f2960222 Mon Sep 17 00:00:00 2001 From: "tobias.pitters" Date: Thu, 30 Jun 2022 07:45:47 +0200 Subject: [PATCH 07/12] update whatsnew --- doc/source/whatsnew/v1.4.4.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/doc/source/whatsnew/v1.4.4.rst b/doc/source/whatsnew/v1.4.4.rst index ef9537200bccd..156b959cf4140 100644 --- a/doc/source/whatsnew/v1.4.4.rst +++ b/doc/source/whatsnew/v1.4.4.rst @@ -14,8 +14,7 @@ including other versions of pandas. Fixed regressions ~~~~~~~~~~~~~~~~~ -- -- +- Fixed regression in :meth:`DataFrame.eval` called with ``inplace=True`` had no effect (:issue:`47449`) .. --------------------------------------------------------------------------- From e52895e6192d2c19752b343d4a435fe0a43cfe50 Mon Sep 17 00:00:00 2001 From: "tobias.pitters" Date: Mon, 11 Jul 2022 21:07:33 +0200 Subject: [PATCH 08/12] add PR suggestions --- doc/source/whatsnew/v1.4.4.rst | 2 +- pandas/core/computation/eval.py | 4 +++- pandas/tests/computation/test_eval.py | 2 ++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/doc/source/whatsnew/v1.4.4.rst b/doc/source/whatsnew/v1.4.4.rst index 156b959cf4140..7439a2ef4ab55 100644 --- a/doc/source/whatsnew/v1.4.4.rst +++ b/doc/source/whatsnew/v1.4.4.rst @@ -14,7 +14,7 @@ including other versions of pandas. Fixed regressions ~~~~~~~~~~~~~~~~~ -- Fixed regression in :meth:`DataFrame.eval` called with ``inplace=True`` had no effect (:issue:`47449`) +- Fixed regression in :meth:`DataFrame.eval` creating a copy when updating inplace (:issue:`47449`) .. --------------------------------------------------------------------------- diff --git a/pandas/core/computation/eval.py b/pandas/core/computation/eval.py index 8e850106079cd..1abaaf817bf40 100644 --- a/pandas/core/computation/eval.py +++ b/pandas/core/computation/eval.py @@ -21,6 +21,8 @@ from pandas.io.formats.printing import pprint_thing +from pandas.core.frame import DataFrame, Series + def _check_engine(engine: str | None) -> str: """ @@ -384,7 +386,7 @@ def eval( try: with warnings.catch_warnings(record=True): # TODO: Filter the warnings we actually care about here. - if hasattr(target, "loc"): + if isinstance(target, (DataFrame, Series)): target.loc[:, assigner] = ret else: target[assigner] = ret diff --git a/pandas/tests/computation/test_eval.py b/pandas/tests/computation/test_eval.py index 30b1953b06e28..b96a94c312382 100644 --- a/pandas/tests/computation/test_eval.py +++ b/pandas/tests/computation/test_eval.py @@ -1893,9 +1893,11 @@ def test_negate_lt_eq_le(engine, parser): def test_set_inplace(): # GH 47449 df = DataFrame({"A": range(1, 6), "B": range(10, 0, -2), "C": range(11, 16)}) + result_view = df[:] expected = Series([21, 20, 19, 18, 17], index=[0, 1, 2, 3, 4], name="A") df.eval("A = B + C", inplace=True) tm.assert_series_equal(df["A"], expected) + tm.assert_series_equal(result_view["A"], expected) class TestValidate: From e3e34cc5af8384488fc1064b5dc3cfbabd9ec5c2 Mon Sep 17 00:00:00 2001 From: "tobias.pitters" Date: Mon, 11 Jul 2022 21:56:40 +0200 Subject: [PATCH 09/12] update imports in eval.py --- pandas/core/computation/eval.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pandas/core/computation/eval.py b/pandas/core/computation/eval.py index 1abaaf817bf40..ab99c65d57ee5 100644 --- a/pandas/core/computation/eval.py +++ b/pandas/core/computation/eval.py @@ -18,11 +18,13 @@ from pandas.core.computation.ops import BinOp from pandas.core.computation.parsing import tokenize_string from pandas.core.computation.scope import ensure_scope +from pandas.core.frame import ( + DataFrame, + Series, +) from pandas.io.formats.printing import pprint_thing -from pandas.core.frame import DataFrame, Series - def _check_engine(engine: str | None) -> str: """ From 283c20aa99620d2e4997cd39e243de59a6b1a428 Mon Sep 17 00:00:00 2001 From: Joris Van den Bossche Date: Fri, 12 Aug 2022 12:02:23 +0200 Subject: [PATCH 10/12] check inplace and use NDFrame + small update to test --- pandas/core/computation/eval.py | 7 ++----- pandas/tests/computation/test_eval.py | 14 +++++++++----- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/pandas/core/computation/eval.py b/pandas/core/computation/eval.py index 09ad814cff6b6..f833b59a2484d 100644 --- a/pandas/core/computation/eval.py +++ b/pandas/core/computation/eval.py @@ -18,10 +18,7 @@ ) from pandas.core.computation.parsing import tokenize_string from pandas.core.computation.scope import ensure_scope -from pandas.core.frame import ( - DataFrame, - Series, -) +from pandas.core.generic import NDFrame from pandas.io.formats.printing import pprint_thing @@ -390,7 +387,7 @@ def eval( try: with warnings.catch_warnings(record=True): # TODO: Filter the warnings we actually care about here. - if isinstance(target, (DataFrame, Series)): + if inplace and isinstance(target, NDFrame): target.loc[:, assigner] = ret else: target[assigner] = ret diff --git a/pandas/tests/computation/test_eval.py b/pandas/tests/computation/test_eval.py index b96a94c312382..56c638b6dd136 100644 --- a/pandas/tests/computation/test_eval.py +++ b/pandas/tests/computation/test_eval.py @@ -1891,13 +1891,17 @@ def test_negate_lt_eq_le(engine, parser): def test_set_inplace(): - # GH 47449 - df = DataFrame({"A": range(1, 6), "B": range(10, 0, -2), "C": range(11, 16)}) + # https://github.com/pandas-dev/pandas/issues/47449 + # Ensure we don't only update the DataFrame inplace, but also the actual + # column values, such that references to this column also get updated + df = DataFrame({"A": [1, 2, 3], "B": [4, 5, 6], "C": [7, 8, 9]}) result_view = df[:] - expected = Series([21, 20, 19, 18, 17], index=[0, 1, 2, 3, 4], name="A") + ser = df["A"] df.eval("A = B + C", inplace=True) - tm.assert_series_equal(df["A"], expected) - tm.assert_series_equal(result_view["A"], expected) + expected = DataFrame({"A": [11, 13, 15], "B": [4, 5, 6], "C": [7, 8, 9]}) + tm.assert_frame_equal(df, expected) + tm.assert_series_equal(ser, expected["A"]) + tm.assert_series_equal(result_view["A"], expected["A"]) class TestValidate: From e74b049d5fcf0fcd878d0b227d4b24750e71c130 Mon Sep 17 00:00:00 2001 From: Joris Van den Bossche Date: Fri, 12 Aug 2022 12:11:07 +0200 Subject: [PATCH 11/12] update test to use using_copy_on_write --- pandas/tests/computation/test_eval.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pandas/tests/computation/test_eval.py b/pandas/tests/computation/test_eval.py index fd677d9cc3cb6..9519b48676198 100644 --- a/pandas/tests/computation/test_eval.py +++ b/pandas/tests/computation/test_eval.py @@ -1912,7 +1912,7 @@ def test_eval_no_support_column_name(request, column): tm.assert_frame_equal(result, expected) -def test_set_inplace(): +def test_set_inplace(using_copy_on_write): # https://github.com/pandas-dev/pandas/issues/47449 # Ensure we don't only update the DataFrame inplace, but also the actual # column values, such that references to this column also get updated @@ -1922,8 +1922,13 @@ def test_set_inplace(): df.eval("A = B + C", inplace=True) expected = DataFrame({"A": [11, 13, 15], "B": [4, 5, 6], "C": [7, 8, 9]}) tm.assert_frame_equal(df, expected) - tm.assert_series_equal(ser, expected["A"]) - tm.assert_series_equal(result_view["A"], expected["A"]) + if not using_copy_on_write: + tm.assert_series_equal(ser, expected["A"]) + tm.assert_series_equal(result_view["A"], expected["A"]) + else: + expected = Series([1, 2, 3], name="A") + tm.assert_series_equal(ser, expected) + tm.assert_series_equal(result_view["A"], expected) class TestValidate: From 9959800f1dffbdf8d2644cfef0da6cbbe61f1596 Mon Sep 17 00:00:00 2001 From: Joris Van den Bossche Date: Fri, 19 Aug 2022 10:08:58 +0200 Subject: [PATCH 12/12] skip test for array manager --- pandas/tests/computation/test_eval.py | 1 + 1 file changed, 1 insertion(+) diff --git a/pandas/tests/computation/test_eval.py b/pandas/tests/computation/test_eval.py index 9519b48676198..b3b80e8e5db65 100644 --- a/pandas/tests/computation/test_eval.py +++ b/pandas/tests/computation/test_eval.py @@ -1912,6 +1912,7 @@ def test_eval_no_support_column_name(request, column): tm.assert_frame_equal(result, expected) +@td.skip_array_manager_not_yet_implemented def test_set_inplace(using_copy_on_write): # https://github.com/pandas-dev/pandas/issues/47449 # Ensure we don't only update the DataFrame inplace, but also the actual