ENH: Make shallow copy for align nocopy with CoW #50917
Conversation
| if uses_cow: | ||
| left = self.copy(deep=copy) | ||
| else: | ||
| left = self.copy(deep=copy) if copy or copy is None else self |
There was a problem hiding this comment.
I think we should consider just doing left = self.copy(deep=copy) in both cases. In general, we always return new objects from methods even with copy=False (with swapaxes being another exception).
There was a problem hiding this comment.
Yeah I agree, but would prefer to keep things separate. We have a couple of other methods where we return self in case of len(index)==0 for example, would like to change them all together if we want to.
Can put up a pr for this first before merging here if you prefer
| if (copy or copy is None) and new_data is self._mgr: | ||
| new_data = new_data.copy(deep=copy) | ||
| elif using_copy_on_write() and new_data is self._mgr: | ||
| new_data = new_data.copy(deep=copy) |
There was a problem hiding this comment.
| if (copy or copy is None) and new_data is self._mgr: | |
| new_data = new_data.copy(deep=copy) | |
| elif using_copy_on_write() and new_data is self._mgr: | |
| new_data = new_data.copy(deep=copy) | |
| if (copy or copy is None or using_copy_on_write()) and new_data is self._mgr: | |
| new_data = new_data.copy(deep=copy) |
There was a problem hiding this comment.
Could you also add an explicit test that covers this?
There was a problem hiding this comment.
I prefer my version, this is easier to read imo. The conditions are nicely aligned.
There was a problem hiding this comment.
The align copy false test covers this
There was a problem hiding this comment.
Could you also add an explicit test that covers this?
Sorry, that wasn't clear ;) This change is in _reindex_with_indexers, which is used by align, but is also used through reindex? (I assumed that align would use it through reindex, but that's actually not the case I see now, it is using it directly) And so I was wondering if this change is also needed for correct behaviour of reindex (and so if we missed something there), and if so, it would be good to also have a test for reindex that covers this.
There was a problem hiding this comment.
Ah got you.
We can't get there through reindex, because we have early exits preventing us from reaching _reindex_with_indexers if both axes are equal to the object that is reindexed.
I also looked into avoiding this function call when using align, but there is nothing to be gained there. It makes more sense covering this here.
| @@ -1137,3 +1137,38 @@ def test_isetitem(using_copy_on_write): | |||
| assert np.shares_memory(get_array(df, "c"), get_array(df2, "c")) | |||
| else: | |||
| assert not np.shares_memory(get_array(df, "c"), get_array(df2, "c")) | |||
|
|
|||
|
|
|||
| def test_align_copy_false(using_copy_on_write): | |||
There was a problem hiding this comment.
There are already tests for align in this file, can you move them together?
|
Merging, will open a separate pr for |
xref #49473
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.