Skip to content

CoW warning mode: clean-up some TODOs #56080

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.

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 3 commits into from
Nov 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
3 changes: 2 additions & 1 deletion pandas/tests/copy_view/test_core_functionalities.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ def test_setitem_with_view_invalidated_does_not_copy(
df["b"] = 100
arr = get_array(df, "a")
view = None # noqa: F841
# TODO(CoW-warn) false positive?
# TODO(CoW-warn) false positive? -> block gets split because of `df["b"] = 100`
# which introduces additional refs, even when those of `view` go out of scopes
with tm.assert_cow_warning(warn_copy_on_write):
df.iloc[0, 0] = 100
if using_copy_on_write:
Expand Down
23 changes: 17 additions & 6 deletions pandas/tests/copy_view/test_indexing.py
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,6 @@ def test_subset_column_slice(
subset.iloc[0, 0] = 0
assert not np.shares_memory(get_array(subset, "b"), get_array(df, "b"))
elif warn_copy_on_write:
# TODO(CoW-warn) should warn
with tm.assert_cow_warning(single_block):
subset.iloc[0, 0] = 0
else:
Expand Down Expand Up @@ -334,7 +333,6 @@ def test_subset_set_with_row_indexer(
):
pytest.skip("setitem with labels selects on columns")

# TODO(CoW-warn) should warn
if using_copy_on_write:
indexer_si(subset)[indexer] = 0
elif warn_copy_on_write:
Expand Down Expand Up @@ -369,7 +367,8 @@ def test_subset_set_with_mask(backend, using_copy_on_write, warn_copy_on_write):

mask = subset > 3

# TODO(CoW-warn) should warn
# TODO(CoW-warn) should warn -> mask is a DataFrame, which ends up going through
# DataFrame._where(..., inplace=True)
if using_copy_on_write or warn_copy_on_write:
subset[mask] = 0
else:
Expand Down Expand Up @@ -403,7 +402,6 @@ def test_subset_set_column(backend, using_copy_on_write, warn_copy_on_write):
else:
arr = pd.array([10, 11], dtype="Int64")

# TODO(CoW-warn) should warn
if using_copy_on_write or warn_copy_on_write:
subset["a"] = arr
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 replacing a full column (so never updating the original), so while this gives a SettingWithCopyWarning right now, we don't have to warn anything here for CoW

else:
Expand Down Expand Up @@ -512,7 +510,6 @@ def test_subset_set_columns(backend, using_copy_on_write, warn_copy_on_write, dt
df_orig = df.copy()
subset = df[1:3]

# TODO(CoW-warn) should warn
if using_copy_on_write or warn_copy_on_write:
subset[["a", "c"]] = 0
Comment on lines -515 to 514
Copy link
Member Author

Choose a reason for hiding this comment

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

Similarly here, replacing full columns, so don't have to warn

else:
Expand Down Expand Up @@ -877,6 +874,8 @@ def test_series_subset_set_with_indexer(
)
if warn_copy_on_write:
# TODO(CoW-warn) should also warn for setting with mask
# -> Series.__setitem__ with boolean mask ends up using Series._set_values
# or Series._where depending on value being set
with tm.assert_cow_warning(
not is_mask, raise_on_extra_warnings=warn is not None
):
Expand Down Expand Up @@ -1006,6 +1005,7 @@ def test_column_as_series_set_with_upcast(
s[0] = "foo"
expected = Series([1, 2, 3], name="a")
elif using_copy_on_write or warn_copy_on_write or using_array_manager:
# TODO(CoW-warn) assert the FutureWarning for CoW is also raised
with tm.assert_produces_warning(FutureWarning, match="incompatible dtype"):
s[0] = "foo"
expected = Series(["foo", 2, 3], dtype=object, name="a")
Expand Down Expand Up @@ -1130,6 +1130,7 @@ def test_set_value_copy_only_necessary_column(
view = df[:]

if val == "a" and indexer[0] != slice(None):
# TODO(CoW-warn) assert the FutureWarning for CoW is also raised
with tm.assert_produces_warning(
FutureWarning, match="Setting an item of incompatible dtype is deprecated"
):
Expand All @@ -1154,6 +1155,8 @@ def test_series_midx_slice(using_copy_on_write):
ser = Series([1, 2, 3], index=pd.MultiIndex.from_arrays([[1, 1, 2], [3, 4, 5]]))
result = ser[1]
assert np.shares_memory(get_array(ser), get_array(result))
# TODO(CoW-warn) should warn -> reference is only tracked in CoW mode, so
# warning is not triggered
result.iloc[0] = 100
if using_copy_on_write:
expected = Series(
Expand All @@ -1162,7 +1165,9 @@ def test_series_midx_slice(using_copy_on_write):
tm.assert_series_equal(ser, expected)


def test_getitem_midx_slice(using_copy_on_write, using_array_manager):
def test_getitem_midx_slice(
using_copy_on_write, warn_copy_on_write, using_array_manager
):
df = DataFrame({("a", "x"): [1, 2], ("a", "y"): 1, ("b", "x"): 2})
df_orig = df.copy()
new_df = df[("a",)]
Expand All @@ -1175,6 +1180,10 @@ def test_getitem_midx_slice(using_copy_on_write, using_array_manager):
if using_copy_on_write:
new_df.iloc[0, 0] = 100
tm.assert_frame_equal(df_orig, df)
else:
with tm.assert_cow_warning(warn_copy_on_write):
new_df.iloc[0, 0] = 100
assert df.iloc[0, 0] == 100


def test_series_midx_tuples_slice(using_copy_on_write):
Expand All @@ -1184,6 +1193,8 @@ def test_series_midx_tuples_slice(using_copy_on_write):
)
result = ser[(1, 2)]
assert np.shares_memory(get_array(ser), get_array(result))
# TODO(CoW-warn) should warn -> reference is only tracked in CoW mode, so
# warning is not triggered
result.iloc[0] = 100
if using_copy_on_write:
expected = Series(
Expand Down
3 changes: 0 additions & 3 deletions pandas/tests/copy_view/test_setitem.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import numpy as np
import pytest

from pandas import (
DataFrame,
Expand Down Expand Up @@ -67,8 +66,6 @@ def test_set_column_with_index(using_copy_on_write):
assert not np.shares_memory(get_array(df, "d"), arr)


# TODO(CoW-warn) this should NOT warn
@pytest.mark.filterwarnings("ignore:Setting a value on a view:FutureWarning")
def test_set_columns_with_dataframe(using_copy_on_write):
# Case: setting a DataFrame as new columns copies that data
# (with delayed copy with CoW)
Expand Down
2 changes: 0 additions & 2 deletions pandas/tests/frame/indexing/test_indexing.py
Original file line number Diff line number Diff line change
Expand Up @@ -1466,8 +1466,6 @@ def test_loc_named_tuple_for_midx(self):
)
tm.assert_frame_equal(result, expected)

# TODO(CoW-warn) shouldn't warn, but does because of item cache
@pytest.mark.filterwarnings("ignore:Setting a value on a view:FutureWarning")
@pytest.mark.parametrize("indexer", [["a"], "a"])
@pytest.mark.parametrize("col", [{}, {"b": 1}])
def test_set_2d_casting_date_to_int(self, col, indexer):
Expand Down
4 changes: 2 additions & 2 deletions pandas/tests/groupby/test_groupby.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ def test_repr():
assert result == expected


# TODO(CoW-warn) this should NOT warn
@pytest.mark.filterwarnings("ignore:Setting a value on a view:FutureWarning")
# TODO(CoW-warn) this should NOT warn -> inplace operator triggers it
# @pytest.mark.filterwarnings("ignore:Setting a value on a view:FutureWarning")
def test_groupby_std_datetimelike(warn_copy_on_write):
# GH#48481
tdi = pd.timedelta_range("1 Day", periods=10000)
Expand Down
2 changes: 1 addition & 1 deletion pandas/tests/indexing/test_iloc.py
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ def test_iloc_getitem_slice_dups(self):
tm.assert_frame_equal(df.iloc[10:, :2], df2)
tm.assert_frame_equal(df.iloc[10:, 2:], df1)

# TODO(CoW-warn) this should NOT warn
# TODO(CoW-warn) this should NOT warn -> Series inplace operator
@pytest.mark.filterwarnings("ignore:Setting a value on a view:FutureWarning")
def test_iloc_setitem(self):
df = DataFrame(
Expand Down