-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
TST: add method/dtype coverage to str-accessor; precursor to #23167 #23582
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
Changes from 8 commits
91b720e
dcee05a
41cecb9
01a3c10
a94f569
f0ae1db
16fe71c
78cead0
0aaa04e
9b36a50
c0752eb
a53a28e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -602,82 +602,6 @@ def f(): | |
ordered=True)) | ||
tm.assert_series_equal(result, expected) | ||
|
||
def test_str_accessor_api_for_categorical(self): | ||
# https://github.com/pandas-dev/pandas/issues/10661 | ||
from pandas.core.strings import StringMethods | ||
s = Series(list('aabb')) | ||
s = s + " " + s | ||
c = s.astype('category') | ||
assert isinstance(c.str, StringMethods) | ||
|
||
# str functions, which need special arguments | ||
special_func_defs = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The full list of these functions & arg-combinations is reflected in the |
||
('cat', (list("zyxw"),), {"sep": ","}), | ||
('center', (10,), {}), | ||
('contains', ("a",), {}), | ||
('count', ("a",), {}), | ||
('decode', ("UTF-8",), {}), | ||
('encode', ("UTF-8",), {}), | ||
('endswith', ("a",), {}), | ||
('extract', ("([a-z]*) ",), {"expand": False}), | ||
('extract', ("([a-z]*) ",), {"expand": True}), | ||
('extractall', ("([a-z]*) ",), {}), | ||
('find', ("a",), {}), | ||
('findall', ("a",), {}), | ||
('index', (" ",), {}), | ||
('ljust', (10,), {}), | ||
('match', ("a"), {}), # deprecated... | ||
('normalize', ("NFC",), {}), | ||
('pad', (10,), {}), | ||
('partition', (" ",), {"expand": False}), # not default | ||
('partition', (" ",), {"expand": True}), # default | ||
('repeat', (3,), {}), | ||
('replace', ("a", "z"), {}), | ||
('rfind', ("a",), {}), | ||
('rindex', (" ",), {}), | ||
('rjust', (10,), {}), | ||
('rpartition', (" ",), {"expand": False}), # not default | ||
('rpartition', (" ",), {"expand": True}), # default | ||
('slice', (0, 1), {}), | ||
('slice_replace', (0, 1, "z"), {}), | ||
('split', (" ",), {"expand": False}), # default | ||
('split', (" ",), {"expand": True}), # not default | ||
('startswith', ("a",), {}), | ||
('wrap', (2,), {}), | ||
('zfill', (10,), {}) | ||
] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also added a handful more combinations to test |
||
_special_func_names = [f[0] for f in special_func_defs] | ||
|
||
# * get, join: they need a individual elements of type lists, but | ||
# we can't make a categorical with lists as individual categories. | ||
# -> `s.str.split(" ").astype("category")` will error! | ||
# * `translate` has different interfaces for py2 vs. py3 | ||
_ignore_names = ["get", "join", "translate"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I also got rid of these previously "ignored" methods - meaning they're being tested now as well. |
||
|
||
str_func_names = [f for f in dir(s.str) if not ( | ||
f.startswith("_") or | ||
f in _special_func_names or | ||
f in _ignore_names)] | ||
|
||
func_defs = [(f, (), {}) for f in str_func_names] | ||
func_defs.extend(special_func_defs) | ||
|
||
for func, args, kwargs in func_defs: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This loop is now explicitly parametrized. |
||
res = getattr(c.str, func)(*args, **kwargs) | ||
exp = getattr(s.str, func)(*args, **kwargs) | ||
|
||
if isinstance(res, DataFrame): | ||
tm.assert_frame_equal(res, exp) | ||
else: | ||
tm.assert_series_equal(res, exp) | ||
|
||
invalid = Series([1, 2, 3]).astype('category') | ||
msg = "Can only use .str accessor with string" | ||
|
||
with pytest.raises(AttributeError, match=msg): | ||
invalid.str | ||
assert not hasattr(invalid, 'str') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This part is fully tested ~100x as thoroughly by |
||
|
||
def test_dt_accessor_api_for_categorical(self): | ||
# https://github.com/pandas-dev/pandas/issues/10661 | ||
from pandas.core.indexes.accessors import Properties | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its nice that you added this, but you didn't remove any code. if you are not going to do that , then not much point of putting the fixture in conftest.py in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jreback
This is coming out of your review above:
And I don't get how adding this fixture is tied to code removal? I'm testing the
.str
-accessor on all the inferred dtypes to make sure it raises correctly, that's what I mainly need this fixture for.That I'm testing the validity of the fixture in
test_inference.py
is for consistency, because it belongs there thematically (but could otherwise test that directly in the fixture constructor).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jreback
This fixture would be a perfect candidate for splitting off a PR, but then, I'm afraid you're gonna say it doesn't do anything interesting (yet).
Do you want me to: