CoW: Add warning for replace with inplace #56060

phofl · 2023-11-19T15:00:53Z

jorisvandenbossche · 2023-11-20T08:14:12Z

pandas/core/generic.py

+                if isinstance(self, ABCSeries) and hasattr(self, "_cacher"):
+                    ref_count += 1


The problem might be that in the warning mode, this is not the case? (so that might need to add a not warn_copy_on_write() to this if block updating the ref_count

Although the tests are not failing ..

I don't understand your comment, why would this not be the case?

I double checked and it seems to work in warning mode

Related to #55838 (comment), I thought I had disabled the item cache for the warning mode, which I would think to affect the count.

I double checked this locally, the cache is still populated

OK, will take a closer look

Ah, no, the _cacher points to the DataFrame (in a case like s = df["col"]), so it doesn't increase the ref count for s (which is what is relevant for chained setitem detection). And also it uses a weakref, so shouldn't actually increase the ref count?

The hasattr(self, "_cacher") is kind of a check whether the Series is derived from a DataFrame?
So I think that the reason we need to increase the ref count is still because of _item_cache, not actually _cacher. The presence of _cacher just turns out to be an equivalent check for checking that we are a Series in non-CoW mode (only in that mode _item_cache would be populated). So maybe a more "correct" check (although they will give the same result) would be:

Suggested change

if isinstance(self, ABCSeries) and hasattr(self, "_cacher"):

ref_count += 1

if isinstance(self, ABCSeries) and not (using_copy_on_write() or warn_copy_on_write()):

ref_count += 1

Hmm, that suggestion gives a bunch of failures (false positives) in pandas/tests/series/methods/test_replace.py

Which is logical, because that's the whole point with _item_cache: you don't know when it is populated or when not in the non-CoW case. So you can't use a single fixed REF_COUNT value to check, this actually depends on the circumstances.

And so what you have (checking _cacher) is a way to check if the Series object is derived from a DataFrame with simple indexing (and has populated _item_cache). So that's probably fine? Or are there ways to get a Series that does not populate the cacher?

Or are there ways to get a Series that does not populate the cacher?

Getting a single column with ["col"], .loc[:, "col"], .iloc[:, 0], .get("col"), .xs("col", axis=1) all populate the cacher.

In theory you can get a Series as a result of a calculation that doesn't do this (df.mean().replace(..), but of course that never could have done something useful so we don't need to care about that. We only need to care about getting a Series that is view.

So to summarize: your fix is probably fully correct, I only didn't understand why ;) You might want to add a comment about it.

Yes your conclusion is correct as far as I can tell. That was the reason why I added the hasattr check. Sorry for omitting this information.

Added a comment

jorisvandenbossche · 2023-11-20T08:16:27Z

pandas/tests/copy_view/test_replace.py

        tm.assert_frame_equal(df, df_orig)
+    else:
+        with tm.assert_produces_warning(FutureWarning, match="inplace method"):
+            with option_context("mode.chained_assignment", None):


Does it otherwise also raise a SettingWithCopyWarning?

yes, [["a"]] currently copies

jorisvandenbossche · 2023-11-20T14:06:03Z

pandas/core/generic.py

+            elif not PYPY and not using_copy_on_write():
+                ctr = sys.getrefcount(self)
+                ref_count = REF_COUNT
+                if isinstance(self, ABCSeries) and hasattr(self, "_cacher"):


Suggested change

if isinstance(self, ABCSeries) and hasattr(self, "_cacher"):

# in non-CoW mode, chained Series access will populate the `_item_cache` which results in an increased ref count not below the threshold, while we still need to warn. We detect this case of a Series derived from a DataFrame through the presence of `_cacher`

if isinstance(self, ABCSeries) and hasattr(self, "_cacher"):

(maybe a bit long to put in every place that has this check (after the other PRs), but a comment like this would have explained it to me)

I am fine with adding this here. Maybe adding a link to this comment for the other prs?

phofl · 2023-11-20T21:32:26Z

Are you ok with merging this so that I can adjust the others?

jorisvandenbossche · 2023-11-20T21:57:07Z

Yes, merge away Op ma 20 nov. 2023 22:32 schreef Patrick Hoefler ***@***.***>:

…

Are you ok with merging this so that I can adjust the others? — Reply to this email directly, view it on GitHub <#56060 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHZEUBOTYPH4XHTM4MRQLDYFPD7LAVCNFSM6AAAAAA7RZMGT2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJZHAZTKNJWG4> . You are receiving this because your review was requested.Message ID: ***@***.***>

CoW: Add warning for replace with inplace

dff1fef

phofl requested a review from jorisvandenbossche November 19, 2023 15:01

phofl added Warnings Warnings that appear or should be added to pandas Copy / view semantics labels Nov 19, 2023

phofl added 3 commits November 19, 2023 17:19

Create baseline commit for warning message

89185cd

Merge branch 'cow_warn_baseline' into warn_cow_mode_replace

7562ca0

Merge branch 'main' into warn_cow_mode_replace

d74ad2b

jorisvandenbossche mentioned this pull request Nov 20, 2023

CoW warning mode for cases that will change behaviour #56019

Closed

14 tasks

jorisvandenbossche reviewed Nov 20, 2023

View reviewed changes

Add comment

97bf95f

phofl added this to the 2.2 milestone Nov 20, 2023

phofl merged commit 5625236 into pandas-dev:main Nov 20, 2023

phofl deleted the warn_cow_mode_replace branch November 20, 2023 22:23

phofl added a commit to phofl/pandas that referenced this pull request Nov 21, 2023

CoW: Add warning for replace with inplace (pandas-dev#56060)

e4a710f

		if isinstance(self, ABCSeries) and hasattr(self, "_cacher"):
		ref_count += 1

	if isinstance(self, ABCSeries) and hasattr(self, "_cacher"):
	# in non-CoW mode, chained Series access will populate the `_item_cache` which results in an increased ref count not below the threshold, while we still need to warn. We detect this case of a Series derived from a DataFrame through the presence of `_cacher`
	if isinstance(self, ABCSeries) and hasattr(self, "_cacher"):

Uh oh!

CoW: Add warning for replace with inplace #56060

CoW: Add warning for replace with inplace #56060

Conversation

phofl commented Nov 19, 2023 • edited by jorisvandenbossche Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jorisvandenbossche Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phofl commented Nov 20, 2023

Uh oh!

jorisvandenbossche commented Nov 20, 2023 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

phofl commented Nov 19, 2023 •

edited by jorisvandenbossche

Loading

jorisvandenbossche Nov 20, 2023 •

edited

Loading

jorisvandenbossche Nov 20, 2023 •

edited

Loading