Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TST/CoW: expand test for chained inplace methods #56402

Conversation

jorisvandenbossche
Copy link
Member

Another follow-up on #56400

getattr(df["a"], func)(*args, inplace=True)


def test_methods_iloc_getitem_item_cache_fillna(
Copy link
Member Author

Choose a reason for hiding this comment

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

I added a second version of this test with all hardcoded fillna() calls, so this isn't using *args, and we also have one version without this (in theory we could do this for each of the methods, but given the code is reused in each of the methods, I think just one is also fine)

# TODO(CoW-warn) because of the usage of *args, this doesn't warn on Py3.11+
if using_copy_on_write:
with tm.raises_chained_assignment_error(not PY311):
getattr(df["a"], func)(*args, inplace=True)
Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the first test to also use getattr(..)(*args) for this part of the test, which means we are for now asserting this incorrectly doesn't raise a warning for 3.11+ because of the different ref count when using *args

Comment on lines +132 to +135
# TODO(CoW-warn) ideally also warns on the default mode, but the ser' _cacher
# messes up the refcount
with tm.assert_cow_warning(warn_copy_on_write, match="A value"):
df["a"].fillna(1, inplace=True)
Copy link
Member Author

Choose a reason for hiding this comment

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

I added one more test case to both tests where we populate the item cache but also keep that series object alive. In this case, because that increases the ref count in the non-CoW or non-warn mode (the df['a'] is cached), this doesn't give a warning.

Not sure we can do something about this, though, given the fact we cache the getitem return value and reuse that.
But at least, the good part, is that it does correctly warn on the warning mode. So for users that do the migration in steps, they should see the warning then.

Copy link
Member

@phofl phofl left a comment

Choose a reason for hiding this comment

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

generally looks good, ci is red though

@mroeschke mroeschke merged commit 8cb60d0 into pandas-dev:main Jan 8, 2024
39 checks passed
@mroeschke
Copy link
Member

Thanks @jorisvandenbossche

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Jan 8, 2024
mroeschke pushed a commit that referenced this pull request Jan 9, 2024
…inplace methods) (#56790)

Backport PR #56402: TST/CoW: expand test for chained inplace methods

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
pmhatre1 pushed a commit to pmhatre1/pandas-pmhatre1 that referenced this pull request May 7, 2024
* TST/CoW: expand test for chained inplace methods

* fix test for older python

---------

Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants