Skip to content

Conversation

jorisvandenbossche
Copy link
Member

@jorisvandenbossche jorisvandenbossche commented Oct 29, 2020

Closes #36373

See discussion at #36498 (comment) for reverting the original fix (#30501 for #30484)

@jorisvandenbossche jorisvandenbossche added the Regression Functionality that used to work in a prior pandas version label Oct 29, 2020
@jorisvandenbossche jorisvandenbossche added this to the 1.1.4 milestone Oct 29, 2020
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm merge on green.

@jbrockmendel
Copy link
Member

@jorisvandenbossche i have no objection if you're just looking for the quickest available fix, but another option is to update NDFrame._inplace_method:

def _inplace_method(self, other, op):
    result = op(self, other)

+    if self.ndim == 1 and result._indexed_same(self) and result.dtype == self.dtype:
+        self._values[:] = result._values
+        return self

    self._reset_cacher()
    [...]

i havent run the full test suite on this yet, but it passes the new test this PR adds and one of the two that this PR xfails.

@jorisvandenbossche
Copy link
Member Author

i have no objection if you're just looking for the quickest available fix

That's indeed the case, as I want to have this fixed for 1.1.4, and I am almost off to bed ;-)
But certainly feel free to push that change here (or in another PR), if it passes the test we can merge that instead.

@jreback
Copy link
Contributor

jreback commented Oct 29, 2020

yeah let's just revert this as is to avoid blocking. @jbrockmendel feel free to push your change in a new PR

@simonjayhawkins
Copy link
Member

@jbrockmendel feel free to push your change in a new PR

or could add the change to _maybe_cache_changed from #36498. The blockers on that PR weren't related to that change.

assert all(sliced_series[:3] == [7, 8, 9])

@pytest.mark.xfail(reason="accidental fix reverted - GH37497")
def test_loc_copy_vs_view(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im inclined to think this test is wrong.

orig = DataFrame(zip(range(3), range(3)), columns=["a", "b"])
df = orig.copy()

ser = df.loc[:, "a"]
ser += 2

tm.assert_frame_equal(df, orig)

df.loc[:, "a"] is a view on df, and += is specifically an inplace op (as long as it doesnt result in a dtype change). I think df should be altered.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im inclined to think this test is wrong.

Yep, I had the same feeling: #36498 (comment)

@jorisvandenbossche
Copy link
Member Author

Closing in favor of #37508, which fixes it without reverting the fix in drop (thanks @jbrockmendel !)

@jorisvandenbossche jorisvandenbossche deleted the gh-36373-inplace-arith-regression branch October 30, 2020 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Regression Functionality that used to work in a prior pandas version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

4 participants