Skip to content

BUG: transpose not respecting CoW #51430

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 9 commits into from
Feb 22, 2023
Merged

BUG: transpose not respecting CoW #51430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
138df12
BUG: transpose not respecting CoW
phofl Feb 16, 2023
29c1bda
FIx
phofl Feb 16, 2023
001ddac
Merge branch 'main' into cow_transpose
phofl Feb 16, 2023
f0336da
Merge remote-tracking branch 'upstream/main' into cow_transpose
phofl Feb 17, 2023
ffcd4c0
Adjust
phofl Feb 17, 2023
d305e42
Merge remote-tracking branch 'upstream/main' into cow_transpose
phofl Feb 20, 2023
530f00c
Fix ci
phofl Feb 20, 2023
742e54c
Fix array manager
phofl Feb 20, 2023
bdbc286
Merge branch 'main' into cow_transpose
phofl Feb 21, 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
2 changes: 2 additions & 0 deletions doc/source/whatsnew/v2.0.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,8 @@ Copy-on-Write improvements
- :meth:`DataFrame.replace` will now respect the Copy-on-Write mechanism
when ``inplace=True``.

- :meth:`DataFrame.transpose` will now respect the Copy-on-Write mechanism.

- Arithmetic operations that can be inplace, e.g. ``ser *= 2`` will now respect the
Copy-on-Write mechanism.

Expand Down
8 changes: 6 additions & 2 deletions pandas/core/frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3557,10 +3557,14 @@ def transpose(self, *args, copy: bool = False) -> DataFrame:
if self._can_fast_transpose:
# Note: tests pass without this, but this improves perf quite a bit.
new_vals = self._values.T
if copy:
if copy and not using_copy_on_write():
new_vals = new_vals.copy()

result = self._constructor(new_vals, index=self.columns, columns=self.index)
result = self._constructor(
new_vals, index=self.columns, columns=self.index, copy=False
)
if using_copy_on_write() and len(self) > 0:
result._mgr.add_references(self._mgr) # type: ignore[arg-type]

elif (
self._is_homogeneous_type and dtypes and is_extension_array_dtype(dtypes[0])
Expand Down
3 changes: 3 additions & 0 deletions pandas/core/internals/managers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,9 @@ def add_references(self, mgr: BaseBlockManager) -> None:
Adds the references from one manager to another. We assume that both
managers have the same block structure.
"""
if len(self.blocks) != len(mgr.blocks):
Copy link
Member

Choose a reason for hiding this comment

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

I also don't understand how the case in transpose can ever run into this? Because we should only get here in case of self._can_fast_transpose, and then by definition there should only be 1 block?

Copy link
Member Author

@phofl phofl Feb 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

df = DataFrame({"a": [pd.Timedelta("1 days"), pd.NaT, pd.Timedelta("2 days")]}, dtype="object")

df.T

the constructor infers this to date time after transposing the array. We get 1 Timedelta block back and another NaT block We could consider this a bug though...

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yes, I seem to recall that we have had discussions about this corner case before (IMO we should indeed change that)

I tried locally with object dtype with ints to test, but so this corner case is only for time like, I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we are calling maybe_infer_datetimelike under the hood which causes this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we deprecate this first?

Copy link
Member

Choose a reason for hiding this comment

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

I seem to recall that we have had discussions about this corner case before (IMO we should indeed change that)

And note that this is not only about transpose, it's about constructors in general. So the question is also if we want to change this:

In [10]: df = DataFrame({"a": np.array([pd.Timedelta("1 days"), pd.NaT, pd.Timedelta("2 days")], dtype=object)})

In [11]: df
Out[11]: 
       a
0 1 days
1    NaT
2 2 days

In [12]: df.dtypes
Out[12]: 
a    timedelta64[ns]
dtype: object

(but that's for another issue)

Although of course we could fix this locally in transpose to specifically fix transpose to preserve the dtype.

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 think the constructor would have to be deprecated. Transpose is more of a bug fix imo because that we are using the inference rules of the constructor under the hood is more an implementation detail.

Copy link
Member

@jorisvandenbossche jorisvandenbossche Feb 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

One related issue: #39117

It's also not clear we necessarily want to deprecate this for the constructors. But for transpose() I would certainly consider it a bug / something to change. And for transpose() I am not sure a deprecation is needed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah constructors makes a bit of sense.

I'll try what's failing locally when changing this.

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 have tests covering exactly this behavior :(

FAILED pandas/tests/frame/methods/test_transpose.py::TestTranspose::test_transpose_tzaware_2col_mixed_tz - AssertionError: Attributes of DataFrame.iloc[:, 0] (column name="A") are different
FAILED pandas/tests/frame/methods/test_transpose.py::TestTranspose::test_transpose_object_to_tzaware_mixed_tz - assert False

#26285

So lets merge this without tackling this case

# If block structure changes, then we made a copy
return
for i, blk in enumerate(self.blocks):
blk.refs = mgr.blocks[i].refs
# Argument 1 to "add_reference" of "BlockValuesRefs" has incompatible type
Expand Down
34 changes: 34 additions & 0 deletions pandas/tests/copy_view/test_methods.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1593,3 +1593,37 @@ def test_inplace_arithmetic_series_with_reference(using_copy_on_write):
tm.assert_series_equal(ser_orig, view)
else:
assert np.shares_memory(get_array(ser), get_array(view))


@pytest.mark.parametrize("copy", [True, False])
def test_transpose(using_copy_on_write, copy, using_array_manager):
df = DataFrame({"a": [1, 2, 3], "b": 1})
df_orig = df.copy()
result = df.transpose(copy=copy)

if not copy and not using_array_manager or using_copy_on_write:
assert np.shares_memory(get_array(df, "a"), get_array(result, 0))
else:
assert not np.shares_memory(get_array(df, "a"), get_array(result, 0))

result.iloc[0, 0] = 100
if using_copy_on_write:
tm.assert_frame_equal(df, df_orig)


def test_transpose_different_dtypes(using_copy_on_write):
df = DataFrame({"a": [1, 2, 3], "b": 1.5})
df_orig = df.copy()
result = df.T

assert not np.shares_memory(get_array(df, "a"), get_array(result, 0))
result.iloc[0, 0] = 100
if using_copy_on_write:
tm.assert_frame_equal(df, df_orig)


def test_transpose_ea_single_column(using_copy_on_write):
df = DataFrame({"a": [1, 2, 3]}, dtype="Int64")
result = df.T

assert not np.shares_memory(get_array(df, "a"), get_array(result, 0))