Maintain inplace operation on series #36498

samilAyoub · 2020-09-20T12:47:33Z

closes REGR: inplace arithmetic operation on Series no longer updating parent DataFrame #36373
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

…rame

pep8speaks · 2020-09-20T12:47:36Z

Hello @samilAyoub! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-09-25 19:30:33 UTC

….com/samilAyoub/pandas into maintain_inplace_operation_on_series

dsaxton · 2020-09-20T13:42:48Z

pandas/core/ops/methods.py

@@ -93,8 +93,6 @@ def _wrap_inplace_method(method):

        def f(self, other):
            result = method(self, other)
-            # Delete cacher
-            self._reset_cacher()


This was added to fix an existing bug (see #30501) so removing it would likely cause a regression

could this have been the incorrect location for the original fix? it appears there were a couple of iterations in #30501.

jbrockmendel · 2020-09-24T17:13:38Z

pandas/tests/arithmetic/test_numeric.py

@@ -1297,3 +1297,10 @@ def test_dataframe_div_silenced():
    )
    with tm.assert_produces_warning(None):
        pdf1.div(pdf2, fill_value=0)
+
+
+def test_inplace_arithmetic_operation_on_series_updating_parent_dataframe():


this test passes for me on master. is there another case that fails?

yes I think need to test df["A"] is s which is also True on 1.1.2

>>> pd.__version__ '1.1.2' >>> >>> import pandas.testing as tm >>> >>> df = pd.DataFrame({"A": [1, 2, 3]}) >>> s = df["A"] >>> s += 1 >>> tm.assert_series_equal(df["A"], s) >>> >>> s 0 2 1 3 2 4 Name: A, dtype: int64 >>> >>> df["A"] 0 2 1 3 2 4 Name: A, dtype: int64 >>> >>> df A 0 1 1 2 2 3 >>> df["A"] is s True >>>

but the output of __repr__ and also other ops is incorrect

so should also compare using assert_frame_equal

>>> tm.assert_frame_equal(df, pd.DataFrame({"A": [2, 3, 4]})) Traceback (most recent call last): ... AssertionError: DataFrame.iloc[:, 0] (column name="A") are different DataFrame.iloc[:, 0] (column name="A") values are different (100.0 %) [index]: [0, 1, 2] [left]: [1, 2, 3] [right]: [2, 3, 4] >>>

but this should pass with the fix here

jbrockmendel · 2020-09-24T17:17:11Z

It looks like the underlying problem is

ser = pd.Series([1, 2, 3])
vals = ser._values
ser += 1
assert ser._values is vals   # <-- Fails!

simonjayhawkins · 2020-09-25T15:06:59Z

It looks like the underlying problem is

but that also occurs in 1.0.5 (and on 0.25.3)

>>> pd.__version__
'1.0.5'
>>>
>>> ser = pd.Series([1, 2, 3])
>>> vals = ser._values
>>>
>>> ser += 1
>>> assert ser._values is vals  # <-- Fails!
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AssertionError

so maybe not directly related to regression?

…operation_on_series

simonjayhawkins · 2020-09-25T18:09:24Z

pandas/tests/indexing/test_loc.py

@@ -888,22 +888,6 @@ def test_identity_slice_returns_new_object(self):
        original_series[:3] = [7, 8, 9]
        assert all(sliced_series[:3] == [7, 8, 9])

-    def test_loc_copy_vs_view(self):


see #15631 (comment)

umm pls revert this test.

simonjayhawkins · 2020-09-25T19:31:51Z

@jbrockmendel I think ready if you can have a look. Thanks.

jbrockmendel · 2020-09-25T21:46:47Z

It looks like the underlying problem is

but that also occurs in 1.0.5 (and on 0.25.3)

So fixing this would fix two bugs instead of one. Have you taken a look at what it would take to fix this? I suspect it may be simpler than the implementation here.

jreback · 2020-09-26T01:34:36Z

pandas/tests/arithmetic/test_numeric.py

+    df = pd.DataFrame({"A": [1, 2, 3]})
+    s = df["A"]
+    s += 1
+    assert s is df["A"]


can we also assert df['A'].to_numpy().base is s.to_numpy().base

I guess if we do this we no longer need the cacher machinery. At first glance his appears a big change so will open a separate PR as an alternative to this one.

jreback · 2020-09-26T01:35:07Z

pandas/tests/indexing/test_loc.py

@@ -888,22 +888,6 @@ def test_identity_slice_returns_new_object(self):
        original_series[:3] = [7, 8, 9]
        assert all(sliced_series[:3] == [7, 8, 9])

-    def test_loc_copy_vs_view(self):


umm pls revert this test.

jreback · 2020-09-26T01:35:41Z

@simonjayhawkins this may need some more work, don't block on for 1.1.3

simonjayhawkins · 2020-09-26T09:11:52Z

It looks like the underlying problem is

but that also occurs in 1.0.5 (and on 0.25.3)

So fixing this would fix two bugs instead of one. Have you taken a look at what it would take to fix this?

It appears to me that the issue is that an inplace operation on the Series deletes the weakref to the dataframe, i.e. the fix in #30501 was incorrect

I suspect it may be simpler than the implementation here.

This is simple. revert the incorrect fix. remove the test that has since been added for a issue that was not actually fixed. and finally ensure that dropped columns are not added back to original dataframe (i.e. what imo should have been done in #30501 originally).

Including fixes for the deleted test, i.e. issue #15631 would only make the change more invasive?

I'm not sure I understand how a more invasive change would be a backport candidate.

jbrockmendel · 2020-09-26T17:28:27Z

@simonjayhawkins your appraisal seems reasonable for backporting purposes. i'll take a look a the ser.values issue and see how that affects options for longer-term solutions.

jreback · 2020-10-14T18:22:05Z

@samilAyoub

can we just revert the original. (and pls merge master as well)

jreback · 2020-10-26T17:51:13Z

@simonjayhawkins this PR is stale, but I think if we revert the original commit is best here.

simonjayhawkins · 2020-10-26T18:06:34Z

@simonjayhawkins this PR is stale, but I think if we revert the original commit is best here.

if we do that, I think we would still need to disable or remove test_loc_copy_vs_view see #15631 (comment)

jreback · 2020-10-26T18:35:34Z

@simonjayhawkins this PR is stale, but I think if we revert the original commit is best here.

if we do that, I think we would still need to disable or remove test_loc_copy_vs_view see #15631 (comment)

right that's what i mean revert that patch

jorisvandenbossche · 2020-10-29T21:21:10Z

I opened a PR with a revert of the original patch: #37497

simonjayhawkins · 2020-10-29T21:24:07Z

I opened a PR with a revert of the original patch: #37497

test_loc_copy_vs_view will fail.

jorisvandenbossche · 2020-10-29T21:44:20Z

Ah, yes, thanks for the note, added an xfail
(I am personally not really sure the test is actually correct, though, but that's another discussion ;-))

jorisvandenbossche · 2020-10-30T09:42:02Z

This PR can probably be claused now with #37508 merged?

@samilAyoub sorry about sidetracking this PR, but wanted to go fast yesterday to be able to get a fix into 1.1.4 release today

simonjayhawkins · 2020-10-30T10:25:44Z

Thanks @samilAyoub for working on this.

samilAyoub added 2 commits September 20, 2020 13:26

Not deleting cacher

1497988

add unit test to check if + operation on Series updating parent DataF…

b2b9bd6

…rame

samilAyoub added 5 commits September 20, 2020 13:51

solve PEP 8 issues

f7b0893

more PEP 8 issues

816425c

PEP 8 issues ...

8494202

Merge branch 'maintain_inplace_operation_on_series' of https://github…

e9d6447

….com/samilAyoub/pandas into maintain_inplace_operation_on_series

PEP8 ...

7277d38

dsaxton added Copy / view semantics Regression Functionality that used to work in a prior pandas version labels Sep 20, 2020

dsaxton added this to the 1.1.3 milestone Sep 20, 2020

dsaxton reviewed Sep 20, 2020

View reviewed changes

simonjayhawkins requested a review from jbrockmendel September 24, 2020 15:52

jbrockmendel reviewed Sep 24, 2020

View reviewed changes

simonjayhawkins added 5 commits September 25, 2020 16:26

Merge remote-tracking branch 'upstream/master' into maintain_inplace_…

5cbe60f

…operation_on_series

update test

dc5e8a1

lint and comment

acb3c02

fix failing tests

ffc051f

remove failing test

8640237

simonjayhawkins reviewed Sep 25, 2020

View reviewed changes

add release note

56a4c88

jreback requested changes Sep 26, 2020

View reviewed changes

jreback modified the milestones: 1.1.3, 1.1.4 Sep 26, 2020

simonjayhawkins mentioned this pull request Oct 5, 2020

REGR: inplace arithmetic operation on Series no longer updating parent DataFrame #36373

Closed

3 tasks

jreback modified the milestones: 1.1.4, 1.2 Oct 27, 2020

jorisvandenbossche mentioned this pull request Oct 29, 2020

REGR: fix inplace arithmetic operation on Series no longer updating parent DataFrame #37497

Closed

simonjayhawkins closed this Oct 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintain inplace operation on series #36498

Maintain inplace operation on series #36498

samilAyoub commented Sep 20, 2020 •

edited

Loading

pep8speaks commented Sep 20, 2020 •

edited

Loading

dsaxton Sep 20, 2020

simonjayhawkins Sep 23, 2020

jbrockmendel Sep 24, 2020

simonjayhawkins Sep 25, 2020

jbrockmendel commented Sep 24, 2020

simonjayhawkins commented Sep 25, 2020

simonjayhawkins Sep 25, 2020

jreback Sep 26, 2020

simonjayhawkins commented Sep 25, 2020

jbrockmendel commented Sep 25, 2020

jreback Sep 26, 2020

simonjayhawkins Sep 26, 2020

jreback Sep 26, 2020

jreback commented Sep 26, 2020

simonjayhawkins commented Sep 26, 2020

jbrockmendel commented Sep 26, 2020

jreback commented Oct 14, 2020

jreback commented Oct 26, 2020

simonjayhawkins commented Oct 26, 2020

jreback commented Oct 26, 2020

jorisvandenbossche commented Oct 29, 2020

simonjayhawkins commented Oct 29, 2020

jorisvandenbossche commented Oct 29, 2020

jorisvandenbossche commented Oct 30, 2020

simonjayhawkins commented Oct 30, 2020

Maintain inplace operation on series #36498

Maintain inplace operation on series #36498

Conversation

samilAyoub commented Sep 20, 2020 • edited Loading

pep8speaks commented Sep 20, 2020 • edited Loading

Comment last updated at 2020-09-25 19:30:33 UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Sep 24, 2020

simonjayhawkins commented Sep 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjayhawkins commented Sep 25, 2020

jbrockmendel commented Sep 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Sep 26, 2020

simonjayhawkins commented Sep 26, 2020

jbrockmendel commented Sep 26, 2020

jreback commented Oct 14, 2020

jreback commented Oct 26, 2020

simonjayhawkins commented Oct 26, 2020

jreback commented Oct 26, 2020

jorisvandenbossche commented Oct 29, 2020

simonjayhawkins commented Oct 29, 2020

jorisvandenbossche commented Oct 29, 2020

jorisvandenbossche commented Oct 30, 2020

simonjayhawkins commented Oct 30, 2020

samilAyoub commented Sep 20, 2020 •

edited

Loading

pep8speaks commented Sep 20, 2020 •

edited

Loading