-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
ENH: Add lazy copy to align #50432
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
ENH: Add lazy copy to align #50432
Conversation
|
Whatsnew entry? |
|
We did not add entries for any of those, but can discuss this maybe in the next dev call and then add all at once? |
|
Sounds good. IMO I think we should add an entry for these methods that get a CoW benefit |
|
Sounds good to me, we should also add a whatsnew in 1.5.0 explaining how to try it out. I’ll make prs for this in the coming days after compiling the prs that were already merged |
|
Thanks @phofl |
| left = self._reindex_indexer(join_index, lidx, copy) | ||
| elif lidx is None or join_index is None: | ||
| left = self.copy() if copy else self | ||
| left = self.copy(deep=copy) if copy or copy is None else self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean there are still cases where self is being returned? (if you manually specify copy=False)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In [1]: s1 = pd.Series([1, 2, 3])
In [2]: s2 = pd.Series([4, 5, 6])
In [3]: s11, s22 = s1.align(s2, copy=False)
In [4]: s11 is s1
Out[4]: True
In [5]: s11.iloc[0] = 10
In [6]: s1
Out[6]:
0 10
1 2
2 3
dtype: int64
Of course, this is also because copy is already an existing keyword, so we don't just do self = self.copy(deep=None) under the hood as in other methods that in the past always returned a copy.
But for other methods that already have a copy keyword, I think for now we used copy=False to also mean shallow/lazy copy like copy=None with CoW enabled.
(it's also something we still need to discuss in general what to do with those copy keywords)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But for other methods that already have a copy keyword, I think for now we used
copy=Falseto also mean shallow/lazy copy likecopy=Nonewith CoW enabled.
Actually, that's not fully correct. It seems this depends on case by case depending on how it is implemented, and we didn't consistently cover this case. Will open a dedicated issue for this topic: #50535
| return self.copy() | ||
| if copy or copy is None: | ||
| return self.copy(deep=copy) | ||
| return self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.