8000 API / CoW: Copy NumPy arrays by default in DataFrame constructor by phofl · Pull Request #51731 · pandas-dev/pandas · GitHub
[go: up one dir, main page]
Skip to content

API / CoW: Copy NumPy arrays by default in DataFrame constructor #51731

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.

Sign up for GitHub

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

Merged
merged 29 commits into from
Mar 17, 2023
Merged

API / CoW: Copy NumPy arrays by default in DataFrame constructor #51731

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
563257e
Implement copy by default for numpy array
phofl Mar 1, 2023
f3161a3
Fix tests
phofl Mar 1, 2023
3a95311
Simplify
phofl Mar 2, 2023
17cf5ae
Fix
phofl Mar 2, 2023
07aa26d
Remove
phofl Mar 2, 2023
8e84d85
Merge branch 'main' into cow_copy_numpy_array
phofl Mar 2, 2023
f3ccf0f
Add comment
phofl Mar 3, 2023
5cdc6ad
Fix
phofl Mar 3, 2023
3e384ea
Change logic
phofl Mar 3, 2023
49ee53f
Merge remote-tracking branch 'upstream/main' into cow_copy_numpy_array
phofl Mar 3, 2023
fcc7be2
Fix tests
phofl Mar 3, 2023
9223836
Fix test
phofl Mar 3, 2023
a474bf5
Fix test
phofl Mar 3, 2023
d5a0268
Merge branch 'main' into cow_copy_numpy_array
phofl Mar 4, 2023
0be7fc6
Merge remote-tracking branch 'upstream/main' into cow_copy_numpy_array
phofl Mar 7, 2023
293f8a5
Remove order changes
phofl Mar 14, 2023
3376d06
Merge remote-tracking branch 'upstream/main' into cow_copy_numpy_array
phofl Mar 14, 2023
265d9e3
Update
phofl Mar 14, 2023
9ac1bae
Address review
phofl Mar 14, 2023
db92ce4
Add test
phofl Mar 14, 2023
be9cb04
Fix test
phofl Mar 15, 2023
4bf3ee8
Merge branch 'main' into cow_copy_numpy_array
phofl Mar 15, 2023
e2eceec
Fix array manager
phofl Mar 15, 2023
65965c6
Remove elif
phofl Mar 15, 2023
ecb756c
Update pandas/tests/copy_view/test_constructors.py
phofl Mar 15, 2023
8e837d9
Merge remote-tracking branch 'upstream/main' into cow_copy_numpy_array
phofl Mar 15, 2023
177fbbc
Add whatsnew
phofl Mar 15, 2023
2bbff3b
Merge branch 'main' into cow_copy_numpy_array
phofl Mar 16, 2023
5bef4ba
Add note
phofl Mar 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Implement copy by default for numpy array
  • Loading branch information
@phofl
phofl committed Mar 1, 2023
commit 563257ed285af46e156fef8e86c23e936a8918af
7 changes: 6 additions & 1 deletion pandas/core/construction.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import numpy as np
from numpy import ma

from pandas._config import using_copy_on_write

from pandas._libs import lib
from pandas._libs.tslibs.period import Period
from pandas._typing import (
Expand Down Expand Up @@ -762,6 +764,9 @@ def _try_cast(

subarr = maybe_cast_to_integer_array(arr, dtype)
else:
subarr = np.array(arr, dtype=dtype, copy=copy)
if using_copy_on_write():
subarr = np.array(arr, dtype=dtype, copy=copy, order="F")
Copy link
Member

Choose a reason for hiding this comment

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

Why is the order="F" specifically needed for CoW? (and can you add a comment about it)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is about #50756

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I understand that it's related to that, but I don't understand why we would only do it for CoW? The default is not to copy arrays (without CoW) at the moment, which creates this inefficient layout; but so if the user manually specifies copy=True in the constructor, why not directly copy it to the desired layout in all cases without the if/else check for CoW?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah did not think about this, will add

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on @jbrockmendel s comment above I think we should leave it out for now

Copy link
Member

Choose a reason for hiding this comment

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

Based on @jbrockmendel s comment above I think we should leave it out for now

Leave it out in general (from this PR), or you mean not do it for the default mode for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely default mode and maybe also split the copy and the layout change into two PRs?

Copy link
Member

Choose a reason for hiding this comment

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

reiterating preference for this to be separate. The two of you are much more familiar with the CoW logic than most of the rest of us; i get antsy when i see using_copy_on_write checks appearing in new places

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed all order relevant changes, is more straightforward now

else:
subarr = np.array(arr, dtype=dtype, copy=copy)

return subarr
12 changes: 10 additions & 2 deletions pandas/core/dtypes/cast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

import numpy as np

from pandas._config import using_copy_on_write

from pandas._libs import lib
from pandas._libs.missing import (
NA,
Expand Down Expand Up @@ -1594,11 +1596,17 @@ def maybe_cast_to_integer_array(arr: list | np.ndarray, dtype: np.dtype) -> np.n
"NumPy will stop allowing conversion of out-of-bound Python int",
DeprecationWarning,
)
casted = np.array(arr, dtype=dtype, copy=False)
if using_copy_on_write():
casted = np.array(arr, dtype=dtype, copy=False, order="F")
else:
casted = np.array(arr, dtype=dtype, copy=False)
else:
with warnings.catch_warnings():
warnings.filterwarnings("ignore", category=RuntimeWarning)
casted = arr.astype(dtype, copy=False)
if using_copy_on_write():
casted = arr.astype(dtype, copy=False, order="F")
else:
casted = arr.astype(dtype, copy=False)
except OverflowError as err:
raise OverflowError(
"The elements provided in the data cannot all be "
Expand Down
10 changes: 10 additions & 0 deletions pandas/core/internals/construction.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
import numpy as np
from numpy import ma

from pandas._config import using_copy_on_write

from pandas._libs import lib
from pandas._typing import (
ArrayLike,
Expand Down Expand Up @@ -289,6 +291,14 @@ def ndarray_to_mgr(
if values.ndim == 1:
values = values.reshape(-1, 1)

elif (
using_copy_on_write()
and isinstance(values, np.ndarray)
and (dtype is None or is_dtype_equal(values.dtype, dtype))
):
values = np.array(values, order="F", copy=copy_on_sanitize)
Copy link
Member

Choose a reason for hiding this comment

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

is the order="F" thing something we should be doing in general in cases with copy=True?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are operating on the transposed array when copying a DataFrame object, so not needed in that case

Copy link
Member

Choose a reason for hiding this comment

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

xref #44871 took a look at preserving order when doing copy. there are some tradeoffs here

values = _ensure_2d(values)

elif isinstance(values, (np.ndarray, ExtensionArray, ABCSeries, Index)):
# drop subclass info
values = np.array(values, copy=copy_on_sanitize)
Expand Down
2 changes: 2 additions & 0 deletions pandas/tests/frame/methods/test_set_index.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import numpy as np
import pytest

import pandas as pd
from pandas import (
Categorical,
DataFrame,
Expand Down Expand Up @@ -401,6 +402,7 @@ def test_set_index_preserve_categorical_dtype(self):
tm.assert_frame_equal(result, df)

def test_set_index_datetime(self):
pd.options.mode.copy_on_write = True
# GH#3950
df = DataFrame(
{
Expand Down
0