Repository: pandas-dev/pandas
Pull request: BUG: Fixed strange behaviour of pd.DataFrame.drop() with inplace argu…
closes  #30484

- [x] closes #xxxx
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

This is my first PR, so pardon me if I did something wrong.

To fix this issue I started by creating a failing test based on the example given by the reporter. After, I fixed the test by removing the "inplace" special methods from add_special_arithmetic_methods, following what was suggested as a comment in the code.

Problem statement:
Strange behaviour of pd.DataFrame.drop() with inplace argument
#### Code Sample, a copy-pastable example if possible

```python
# input - 1 
df = pd.DataFrame({})
df["x1"] = [1, 2, 3, 4, 5]
df["x2"] = [0, 0, 0, 1, 1]
df["target"] = [10, 20, 30, 40, 50]
y = df["target"]
df.drop("target", axis=1, inplace=True)
y = y + np.min(y)

# output df (without removed target column)
	x1	x2
0	1	0
1	2	0
2	3	0
3	4	1
4	5	1

# input - 2 
df = pd.DataFrame({})
df["x1"] = [1, 2, 3, 4, 5]
df["x2"] = [0, 0, 0, 1, 1]
df["target"] = [10, 20, 30, 40, 50]
y = df["target"]
df.drop("target", axis=1, inplace=True)
y += np.min(y)

# output df (with the removed target column)

  | x1 | x2 | target
-- | -- | -- | --
0 | 1 | 0 | 20
1 | 2 | 0 | 30
2 | 3 | 0 | 40
3 | 4 | 1 | 50
4 | 5 | 1 | 60
```
#### Problem description

Pandas return DataFrame with removed column after doing in-place operation with a column. Is it a bug or intentional behavior? 

#### Output of ``pd.show_versions()``

<details>

INSTALLED VERSIONS
------------------
commit           : None

pandas           : 0.25.1
numpy            : 1.16.2
pytz             : 2018.9
dateutil         : 2.7.5
pip              : 18.1
setuptools       : 42.0.2
Cython           : 0.29.2
pytest           : 4.1.0
hypothesis       : None
sphinx           : 1.8.3
blosc            : None
feather          : None
xlsxwriter       : 1.1.2
lxml.etree       : 4.3.0
html5lib         : 1.0.1
pymysql          : None
psycopg2         : None
jinja2           : 2.10
IPython          : 7.2.0
pandas_datareader: None
bs4              : 4.7.1
bottleneck       : 1.2.1
fastparquet      : None
gcsfs            : None
lxml.etree       : 4.3.0
matplotlib       : 3.0.2
numexpr          : 2.6.9
odfpy            : None
openpyxl         : 2.5.12
pandas_gbq       : None
pyarrow          : None
pytables         : None
s3fs             : None
scipy            : 1.2.1
sqlalchemy       : 1.2.15
tables           : 3.4.4
xarray           : 0.14.0
xlrd             : 1.2.0
xlwt             : 1.3.0
xlsxwriter       : 1.1.2

</details>

Hints from issue discussion:
The use of the `inplace` keyword is strongly discouraged. If you replace that with the more idiomatic `df = df.drop("target", axis=1)` then things look OK

If you'd like to take a look internally would welcome a patch
```df = df.drop("target", axis=1)```
Does it require more memory than `inplace`?

Commit message: Update v1.0.0.rst

Patch to review:
diff --git a/pandas/core/ops/methods.py b/pandas/core/ops/methods.py
index c04658565f235..0cf1ac4d107f6 100644
--- a/pandas/core/ops/methods.py
+++ b/pandas/core/ops/methods.py
@@ -93,7 +93,8 @@ def _wrap_inplace_method(method):
 
         def f(self, other):
             result = method(self, other)
-
+            # Delete cacher
+            self._reset_cacher()
             # this makes sure that we are aligned like the input
             # we are updating inplace so we want to ignore is_copy
             self._update_inplace(
diff --git a/pandas/tests/frame/test_axis_select_reindex.py b/pandas/tests/frame/test_axis_select_reindex.py
index 7effa98fd8213..0bc67e9e41ee7 100644
--- a/pandas/tests/frame/test_axis_select_reindex.py
+++ b/pandas/tests/frame/test_axis_select_reindex.py
@@ -1152,3 +1152,22 @@ def test_drop_non_empty_list(self, index, drop_labels):
         # GH 21494
         with pytest.raises(KeyError, match="not found in axis"):
             pd.DataFrame(index=index).drop(drop_labels)
+
+    @pytest.mark.parametrize(
+        "operation", ["__iadd__", "__isub__", "__imul__", "__ipow__"]
+    )
+    @pytest.mark.parametrize("inplace", [False, True])
+    def test_inplace_drop_and_operation(self, operation, inplace):
+        # GH 30484
+        data_dict = {"x": [1, 2, 3, 4, 5], "y": [10, 20, 30, 40, 50]}
+        df = pd.DataFrame(data_dict)
+        y = df["y"]
+
+        if inplace:
+            df.drop("y", axis=1, inplace=inplace)
+        else:
+            df = df.drop("y", axis=1, inplace=inplace)
+        # Perform operation and ensure that df is not changed
+        expected = df.copy()
+        getattr(y, operation)(1)
+        tm.assert_frame_equal(df, expected)


Code review comments:
diff_hunk: @@ -1152,3 +1152,22 @@ def test_drop_non_empty_list(self, index, drop_labels):
         # GH 21494
         with pytest.raises(KeyError, match="not found in axis"):
             pd.DataFrame(index=index).drop(drop_labels)
+
+    @pytest.mark.parametrize(
+        "operation", ["__iadd__", "__isub__", "__imul__", "__ipow__"]
+    )
+    @pytest.mark.parametrize("inplace", [False, True])
+    def test_inplace_drop_and_operation(self, operation, inplace):
+        # GH 30484
+        data_dict = {"x": [1, 2, 3, 4, 5], "y": [10, 20, 30, 40, 50]}
+        df = pd.DataFrame(data_dict)
+        y = df["y"]
+
+        if inplace:
+            df.drop("y", axis=1, inplace=inplace)
+        else:
+            df = df.drop("y", axis=1, inplace=inplace)
+        # Perform operation and ensure that df is not changed
+        expected = df.copy()
path: pandas/tests/frame/test_axis_select_reindex.py
text: @user1:
can you put more in the inplace and else clauses, e.g. you should copy the expected first.

also can you assert that no SettingWithCopyWarning is show here.

@author:
Hi @user1,

Can you explain the first sentence please? In my opinion, since I want the test to make sure that an inplace operation in "y" doesn't change the origin "df", then the copy is done in the right place, that is right before this operation.

As for the warning, is it ok if I ensure that no warning of any type is produced? 

@author:
Hi @user1, did you have time to read my comment? Thanks in advance

@user1:
you need to move the 

expeted = df.copy() before running the function, otherwise you can't tell if you modified the original or not.

also need to do

tm.assert_produces_warning(None):
   ......

to make sure not outputting any SettingWithCopyWarnings

@author:
I still think that the copy is being done in the right place. It's the operation in "y" that is changing the "df" when it shouldn't. And the copy is done before this operation. If I do the copy before dropping the "y" from "df" then it's normal that I'll get a different "df" at the end in a bug-free code.

Anyway, I'll try to write the test in a different way that makes sense for you and me.

Thanks!