API / CoW: Copy arrays by default in Series constructor #52022
Conversation
pandas/core/series.py
Outdated
| @@ -394,6 +401,11 @@ def __init__( | |||
| self.name = name | |||
| return | |||
|
|
|||
| if isinstance(data, (ExtensionArray, np.ndarray)): | |||
| if default_cow_copy and not copy and using_copy_on_write(): | |||
There was a problem hiding this comment.
i find this difficult to reason about (... in part bc im distracted by being on a call). if a user explicitly passes copy=False, we should never ignore that. Does this (or any of the other CoW-constructor PRs that im having trouble keeping track of) respect that?
There was a problem hiding this comment.
If you pass an array then we respect copy=False with the drawback that your series gets modified when you modify the array, if you pass a Series/DataFrame we make a lazy copy to set up references
There was a problem hiding this comment.
This is actually the reason why I changed the default to None, so that we can respect a user actually passing False
There was a problem hiding this comment.
I also don't find it easy to read, but I think it can be simplified?
Wouldn't it be enough to check for if copy was originally None? Because if copy=True, I would expect the copy to happen anyway later on (assuming it currently honored that keyword)
There was a problem hiding this comment.
I wanted to avoid using_copy_on_write before hitting the fastpath, but only using copy is certainly more readable, changed
jorisvandenbossche
changed the title
| "arr", [np.array([1, 2, 3], dtype="int64"), pd.array([1, 2, 3], dtype="Int64")] | ||
| ) | ||
| def test_series_from_array(using_copy_on_write, idx, dtype, fastpath, arr): | ||
| ser = Series(arr, dtype=dtype) |
There was a problem hiding this comment.
Should idx and fastpath be passed here as well??
# Conflicts: # pandas/tests/copy_view/test_astype.py # pandas/tests/copy_view/test_constructors.py
# Conflicts: # pandas/tests/copy_view/test_astype.py # pandas/tests/copy_view/test_constructors.py
pandas/core/series.py
Outdated
| if copy is None: | ||
| if using_copy_on_write(): | ||
| copy = True | ||
| else: | ||
| copy = False |
There was a problem hiding this comment.
You could easily move this below the first fastpath if block, when updating that to check for copy is None instead of copy is False
# Conflicts: # doc/source/whatsnew/v2.0.0.rst
|
merging this to be consistent with the DataFrame constructor |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.will update the whatsnew from the other copy=True pr