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

CoW: Remove todos that aren't necessary #56169

Merged
merged 7 commits into from
Nov 27, 2023
Merged

Conversation

phofl
Copy link
Member

@phofl phofl commented Nov 25, 2023

xref #56019

This should warn, row is a Series and the setitem is inplace then

The Index case is a bug at the moment, creating the df is zero copy, updating the df afterwards will propagate to the index, but will be fixed with CoW

test_del_frame the parent is a single block, we are viewing the same block in df2 even though we deleted column "b"

@phofl phofl changed the title CoW: Warn for cases that go through putmask CoW: Remove todos that aren't necessary Nov 25, 2023
msg = "'float' object has no attribute 'startswith'"
with pytest.raises(AttributeError, match=msg):
data.apply(transform, axis=1)
with tm.assert_cow_warning(warn_copy_on_write):
data.apply(transform, axis=1)
Copy link
Member

Choose a reason for hiding this comment

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

I think the reason I thought this didn't need to warn is that row should not be a view, given the dataframe has mixed dtypes.
So after CoW is enabled, nothing will change here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah sorry, you are correct

The implementation is dirty... I'll mess around with it later today

@jorisvandenbossche
Copy link
Member

The Index case is a bug at the moment, creating the df is zero copy, updating the df afterwards will propagate to the index, but will be fixed with CoW

Ah, yes, that ugly bug .. (I was assuming we didn't track some ref correctly in case of Index because of it being immutable in the non-CoW mode)

test_del_frame the parent is a single block, we are viewing the same block in df2 even though we deleted column "b"

Yes, but it's still a "false positive" warning for the user. Just one we decide we can't solve? (or is not worth our time to fix)

@phofl
Copy link
Member Author

phofl commented Nov 27, 2023

Yes, but it's still a "false positive" warning for the user. Just one we decide we can't solve? (or is not worth our time to fix)

yeah sorry should have been more precise. This is a known limitation. I have no hope in coming up with a fix before 2.2 is out if ever

@phofl phofl added this to the 2.2 milestone Nov 27, 2023
@phofl phofl merged commit 4c520e3 into pandas-dev:main Nov 27, 2023
42 checks passed
@phofl phofl deleted the cow_apply branch November 27, 2023 22:45
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