ENH: Improve replace lazy copy handling #51658
Conversation
|
Can you consolidate the inplace replace with no refs and inplace replace with refs tests by parametrizing? Would make it a little easier to see implemented so far/missing. |
|
done |
| nb = self.astype(np.dtype(object), copy=False) | ||
| if nb is self and not inplace: | ||
| has_ref = self.refs.has_reference() | ||
| nb = self.astype(np.dtype(object), copy=False, using_cow=using_cow) |
There was a problem hiding this comment.
copy=False doesn't do anything here right since astyping (to object) will always make a copy?
pandas/pandas/core/dtypes/astype.py
Lines 136 to 138 in 29140d4
There was a problem hiding this comment.
No this can create a view, would have to step through the code paths but discovered a bug when this is a view
There was a problem hiding this comment.
Haven't looked into this too closely, but I guess this would be a view when going from object -> object.
Happy to let optimizing this be a follow up.
There was a problem hiding this comment.
Yeah this is the case where I was able to produce the bug. Run time wise this is more or less optimal right now, the actual checks are performed within astype anyway
There was a problem hiding this comment.
I was referring to the nb.copy() below. I'm a little worried that that might double copy if astype already copied.
There was a problem hiding this comment.
Yeah this can still happen, but Astype should take care of returning a block without references if a copy already happened (check is not perfect yet though)
| assert not np.shares_memory(get_array(df, "a"), arr_a) | ||
| assert df._mgr._has_no_reference(0) | ||
| assert view._mgr._has_no_reference(0) | ||
| if isinstance(to_replace, list) and len(to_replace) == 0: |
There was a problem hiding this comment.
Might be better to parametrize test_replace_emptylist for inplace=True/False
There was a problem hiding this comment.
added it to another test where it added less complexity
| nb = self.astype(np.dtype(object), copy=False) | ||
| if nb is self and not inplace: | ||
| has_ref = self.refs.has_reference() | ||
| nb = self.astype(np.dtype(object), copy=False, using_cow=using_cow) |
…ndling) (#51679) Backport PR #51658: ENH: Improve replace lazy copy handling Co-authored-by: Patrick Hoefler <[email protected]>
| if nb is self and not inplace: | ||
| has_ref = self.refs.has_reference() | ||
| nb = self.astype(np.dtype(object), copy=False, using_cow=using_cow) | ||
| if (nb is self or using_cow) and not inplace: |
There was a problem hiding this comment.
is the nb is self here ever possible?
| nb = self.astype(np.dtype(object), copy=False, using_cow=using_cow) | ||
| if (nb is self or using_cow) and not inplace: | ||
| nb = nb.copy() | ||
| elif inplace and has_ref and nb.refs.has_reference(): |
There was a problem hiding this comment.
IIUC nb.refs.has_reference should only be True if we are already object dtype?
| @@ -811,17 +821,24 @@ def _replace_coerce( | |||
| if value is None: | |||
| # gh-45601, gh-45836, gh-46634 | |||
| if mask.any(): | |||
| nb = self.astype(np.dtype(object), copy=False) | |||
| if nb is self and not inplace: | |||
| has_ref = self.refs.has_reference() | |||
There was a problem hiding this comment.
IIUC checking self.refs.has_reference here vs a few lines down can have different results in the case where self.dtype == object? does that make a difference?
if it doesnt make a difference, i think we can refactor this to share some code with some other CoW-handling
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.