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: Ignore copy=True when copy_on_write is enabled #51464

Merged
merged 12 commits into from
Mar 8, 2023

Conversation

phofl
Copy link
Member

@phofl phofl commented Feb 17, 2023

@phofl phofl marked this pull request as draft February 17, 2023 19:40
@phofl phofl marked this pull request as ready for review February 18, 2023 11:32
@jorisvandenbossche
Copy link
Member

This basically closes #50535? (although maybe not every method listed there is already covered)

@phofl
Copy link
Member Author

phofl commented Mar 3, 2023

I actually forgot about this issue. I double checked, we covered every case except transpose (added a test for this). So should be good to close when this is merged

if not using_copy_on_write or axis in [0, "index"]:
assert comb.index is not ser.index
else:
assert comb.index is ser.index
Copy link
Member

Choose a reason for hiding this comment

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

Not for this PR since we already do it for the copy=False case as well (and I suppose it is a general issue). But I am not sure the index should be identical here. An index' attributes are still mutable (eg setting the name), so we should maybe start creating new Index objects for the same data in such cases.

Sidenote: we should maybe discuss in general how Index objects fit into CoW, since those don't use blocks (and thus don't participate in the refs?)

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 good point

I took a shot at index data in #51803, it’s only a draft for now till we agree that we would want to do this

@phofl phofl added this to the 2.0 milestone Mar 7, 2023
@mroeschke mroeschke merged commit 6d6dc1a into pandas-dev:main Mar 8, 2023
@mroeschke
Copy link
Member

Thanks @phofl

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Mar 8, 2023
@phofl phofl deleted the cow_cop branch March 8, 2023 23:45
phofl added a commit that referenced this pull request Mar 8, 2023
…n_write is enabled) (#51849)

Backport PR #51464: CoW: Ignore copy=True when copy_on_write is enabled

Co-authored-by: Patrick Hoefler <[email protected]>
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.

API: CoW and explicit copy keyword in DataFrame/Series methods
4 participants