Skip to content

CLN/TST: delegate StringArray.fillna() to parent class + add tests #37987

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 19 commits into from
Nov 26, 2020
Merged

CLN/TST: delegate StringArray.fillna() to parent class + add tests #37987

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
e6a3f40
ENH: add dtype validation for value in StringArray.fillna
arw2019 Nov 21, 2020
0a85704
TST: test fillna args
arw2019 Nov 21, 2020
4814614
ENH: add dtype validation for value in StringArray.fillna
arw2019 Nov 21, 2020
7051e97
TST: test fillna args
arw2019 Nov 21, 2020
df8e185
feedback: broaden validation to np.str_ + allow NA fill value
arw2019 Nov 21, 2020
43a6646
TST: add tests with np._str, missing value
arw2019 Nov 21, 2020
616a6c7
skip check if no value provided
arw2019 Nov 21, 2020
a43de61
feedback: remove np.str_ check
arw2019 Nov 21, 2020
f6eff15
Merge branch 'master' of https://github.com/pandas-dev/pandas into st…
arw2019 Nov 21, 2020
24cb928
add is_string_dtype check
arw2019 Nov 21, 2020
9b2f918
merge master
arw2019 Nov 23, 2020
a014784
merge origin
arw2019 Nov 23, 2020
02ff905
Merge branch 'master' of https://github.com/pandas-dev/pandas into st…
arw2019 Nov 24, 2020
b658791
rewrite using _validate_setitem_value
arw2019 Nov 24, 2020
135f55a
revert changes to base class
arw2019 Nov 24, 2020
ef1f800
Merge branch 'master' of https://github.com/pandas-dev/pandas into st…
arw2019 Nov 24, 2020
719d3cd
feedback: revert changes to string_.py
arw2019 Nov 25, 2020
f1489c7
Merge branch 'master' of https://github.com/pandas-dev/pandas into st…
arw2019 Nov 25, 2020
0e84df8
remove fillna override from StringArray
arw2019 Nov 25, 2020
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
5 changes: 4 additions & 1 deletion pandas/core/arrays/string_.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,10 @@ def __setitem__(self, key, value):
super().__setitem__(key, value)

def fillna(self, value=None, method=None, limit=None):
# TODO: validate dtype
if value is not None and not (
Copy link
Contributor

Choose a reason for hiding this comment

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

you should just be able to call _validate_setitem_value(value), maybe we can it it in the baseclass.

Copy link
Member

Choose a reason for hiding this comment

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

i would like this since it would allow us to implement a correct ExtensionBlock._can_hold_element, xref #36226

Copy link
Member Author

Choose a reason for hiding this comment

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

you should just be able to call _validate_setitem_value(value), maybe we can it it in the baseclass.

Gave that a go - is the last commit what you meant? @jreback @jbrockmendel

Copy link
Member

@jorisvandenbossche jorisvandenbossche Nov 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I am -1 adding it in this PR (or at least adding it on the base class). If we add it to the base class, it needs to be part of the EA interface, documented that way, have a base extension test, have a fall back implementation, .. (and first discuss if we actually want it).

Also, we actually don't need to explicitly call the setitem validation here, since fillna is already raising that exception because it uses the setitem implementation under the hood.

So we could also simply add the test to ensure fillna raises the proper error.

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 am -1 adding it in this PR (or at least adding it on the base class). If we add it to the base class, it needs to be part of the EA interface, documented that way, have a base extension test, have a fall back implementation, .. (and first discuss if we actually want it).

Ok! I'll open an issue

Also, we actually don't need to explicitly call the setitem validation here, since fillna is already raising that exception because it uses the setitem implementation under the hood.

So we could also simply add the test to ensure fillna raises the proper error.

There was an existing test and I added more testcases here

isinstance(value, str) or is_string_dtype(value) or isna(value)
):
raise TypeError(f"{value} is not a valid fill value; must be a string")
return super().fillna(value, method, limit)
Copy link
Member

Choose a reason for hiding this comment

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

does this method need to be overriden here at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

it doesn't

Copy link
Member Author

Choose a reason for hiding this comment

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

pushed that now


def astype(self, dtype, copy=True):
Expand Down
18 changes: 18 additions & 0 deletions pandas/tests/arrays/string_/test_string.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,24 @@ def test_reduce_missing(skipna, dtype):
assert pd.isna(result)


def test_fillna_args():
# GH 37987

arr = pd.array(["a", pd.NA], dtype="string")

res = arr.fillna(value="b")
expected = pd.array(["a", "b"], dtype="string")
tm.assert_extension_array_equal(res, expected)

res = arr.fillna(value=np.str_("b"))
expected = pd.array(["a", "b"], dtype="string")
tm.assert_extension_array_equal(res, expected)

msg = r"1 is not a valid fill value; must be a string"
with pytest.raises(TypeError, match=msg):
arr.fillna(value=1)


@td.skip_if_no("pyarrow", min_version="0.15.0")
def test_arrow_array(dtype):
# protocol added in 0.15.0
Expand Down