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

BUG/CoW: Series.rename not making a lazy copy when passed a scalar #53189

Merged
merged 4 commits into from
May 14, 2023

Conversation

lithomas1
Copy link
Member

I put this in for 2.0.2 since we claim that the lazy-copy was implemented for rename in 2.0

@lithomas1 lithomas1 requested a review from phofl May 12, 2023 15:44
@lithomas1 lithomas1 added this to the 2.0.2 milestone May 12, 2023
@@ -4667,7 +4671,9 @@ def rename(
errors=errors,
)
else:
return self._set_name(index, inplace=inplace)
return self._set_name(
index, inplace=inplace, deep=copy and not using_copy_on_write()
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we are doing this here and not in _set_name?

Copy link
Member

Choose a reason for hiding this comment

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

Simply passing copy through should be sufficient though

Copy link
Member Author

Choose a reason for hiding this comment

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

It's to ignore copy=True, when Copy on Write is activated. I just copy pasted from your code in #51464

Copy link
Member

Choose a reason for hiding this comment

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

Forgot about this, thx.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing else calls _set_name other than rename, so I just left the Copy on Write logic in rename itself. I can move it if you want me to.

Copy link
Member

Choose a reason for hiding this comment

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

Probably more consistent to push it down, but not a really strong opinion

@phofl phofl merged commit 455c880 into pandas-dev:main May 14, 2023
@phofl
Copy link
Member

phofl commented May 14, 2023

thx @lithomas1

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request May 14, 2023
@lithomas1 lithomas1 deleted the lazy-series-rename branch May 14, 2023 09:56
lithomas1 added a commit that referenced this pull request May 14, 2023
… a lazy copy when passed a scalar) (#53221)

* Backport PR #53189: BUG/CoW: Series.rename not making a lazy copy when passed a scalar

* Update test_setitem.py

---------

Co-authored-by: Thomas Li <[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.

BUG: Series.rename(scalar, copy=False) still copies data
2 participants