Skip to content
Open
Show file tree
Hide file tree
Changes from 9 commits
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
22 changes: 16 additions & 6 deletions pandas/tests/extension/base/missing.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@


class BaseMissingTests:
_supports_fillna_copy_false = True

def test_isna(self, data_missing):
expected = np.array([True, False])

Expand Down Expand Up @@ -123,18 +125,26 @@ def test_fillna_no_op_returns_copy(self, data):
tm.assert_extension_array_equal(result, data)

def test_fillna_readonly(self, data_missing):
fill_value = data_missing[1]
data = data_missing.copy()
data._readonly = True

expected = data_missing.fillna(fill_value, copy=True)

# by default fillna(copy=True), then this works fine
result = data.fillna(data_missing[1])
assert result[0] == data_missing[1]
res_copy = data.fillna(fill_value, copy=True)
tm.assert_extension_array_equal(res_copy, expected)
Copy link
Member

Choose a reason for hiding this comment

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

can you keep using data_missing instead of defining a new expected. i.e. minimize the diff

tm.assert_extension_array_equal(data, data_missing)

# but with copy=False, this raises for EAs that respect the copy keyword
with pytest.raises(ValueError, match="Cannot modify read-only array"):
data.fillna(data_missing[1], copy=False)
tm.assert_extension_array_equal(data, data_missing)
if self._supports_fillna_copy_false:
# but with copy=False, this raises for EAs that respect the copy keyword
Copy link
Member

Choose a reason for hiding this comment

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

i think this comment should be for EAs that dont respect the copy keyword?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment in the else block for the EAs that don't respect the copy keyword.

Copy link
Member

Choose a reason for hiding this comment

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

ok but isnt the comment here still wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i thought about removing that comment earlier but wasn’t fully sure since it was written specifically for the if case. You're right, so I’ll remove it.

with pytest.raises(ValueError, match="Cannot modify read-only array"):
data.fillna(fill_value, copy=False)
tm.assert_extension_array_equal(data, data_missing)
else:
res_no_copy = data.fillna(fill_value, copy=False)
tm.assert_extension_array_equal(res_no_copy, res_copy)
tm.assert_extension_array_equal(data, data_missing)

def test_fillna_series(self, data_missing):
fill_value = data_missing[1]
Expand Down
5 changes: 0 additions & 5 deletions pandas/tests/extension/test_numpy.py
Original file line number Diff line number Diff line change
Expand Up @@ -342,11 +342,6 @@ def test_fillna_frame(self, data_missing):
# Non-scalar "scalar" values.
super().test_fillna_frame(data_missing)

@skip_nested
def test_fillna_readonly(self, data_missing):
# Non-scalar "scalar" values.
super().test_fillna_readonly(data_missing)

@skip_nested
def test_setitem_invalid(self, data, invalid_scalar):
# object dtype can hold anything, so doesn't raise
Expand Down
17 changes: 2 additions & 15 deletions pandas/tests/extension/test_sparse.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ def data_for_compare(request):


class TestSparseArray(base.ExtensionTests):
_supports_fillna_copy_false = False

def _supports_reduction(self, obj, op_name: str) -> bool:
return True

Expand Down Expand Up @@ -237,21 +239,6 @@ def test_isna(self, data_missing):
def test_fillna_no_op_returns_copy(self, data, request):
super().test_fillna_no_op_returns_copy(data)

def test_fillna_readonly(self, data_missing):
Copy link
Member

Choose a reason for hiding this comment

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

getting rid of this is perfect. i think there are other subclasses that override this too that we also want to get rid of

# copy keyword is ignored by SparseArray.fillna
# -> copy=True vs False doesn't make a difference
data = data_missing.copy()
data._readonly = True

result = data.fillna(data_missing[1])
assert result[0] == data_missing[1]
tm.assert_extension_array_equal(data, data_missing)

# fillna(copy=False) is ignored -> so same result as above
result = data.fillna(data_missing[1], copy=False)
assert result[0] == data_missing[1]
tm.assert_extension_array_equal(data, data_missing)

@pytest.mark.xfail(reason="Unsupported")
def test_fillna_series(self, data_missing):
# this one looks doable.
Expand Down
Loading