-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
BUG: Series.any() and .all() don't return bool values if dtype=object #30416
Conversation
f9037cf
to
876b763
Compare
Hello @MomIsBestFriend! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-01-12 16:37:00 UTC |
78da081
to
48f08c0
Compare
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.
lgtm. can you add a whatsnew note. i think this deserves a small subsection (even though its a bug fix), you can put in api-breaking changes, just show the before and after (model after some existing changes like this)
35391e9
to
650c906
Compare
The only implementation I can think of right now, is to create a function to iterate over the values and returns a bool if it the any/all the values elevated to to True. tl;dr |
650c906
to
00c9b50
Compare
pandas/core/nanops.py
Outdated
# GH #12863 | ||
# Checking if the `axis` is None because numpy | ||
# doesn't handle ``any`` and ``all`` on | ||
# object arrays correclty. see |
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.
correclty -> correctly. small preference for this to go inside the "if", not sure how widely shared that is
assert s1.all(skipna=True) | ||
assert np.isnan(s2.any(skipna=False)) # nan || False => nan | ||
assert s1.any(skipna=True) |
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.
pls add the GH ref here too
# Alternative types, with implicit 'object' dtype. | ||
s = Series(["abc", True]) | ||
assert "abc" == s.any() # 'abc' || True => 'abc' | ||
# GH 12863 |
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.
might as well make this a separate test test_all_any_object_dtype
Does this bugfix affect DataFrame at all? if so, test for that? optional: could test the affected nanops functions directly in test_nanops. |
what case is breaking that necessitates the brute force patch? |
e87ba0a
to
f774031
Compare
The original test case (from #12863) pd.Series(index=range(5), data=['a', 'b', 'c', 'd', 'e']) |
d6996ca
to
cf72a99
Compare
I can't figure it out, when the original case is working, every test is failing. |
if is_object_dtype(dtype) and axis is None: | ||
output = values.any() | ||
else: | ||
output = values.any(axis) |
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.
you can just return directly from here.
else: | ||
output = values.any(axis) | ||
|
||
if isinstance(output, bool): |
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.
when is this NOT true?
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.
That's exactly the bug, for example (from the original issue).
master:
>>> import pandas as pd
>>> s = pd.Series(index=range(5), data=['a', 'b', 'c', 'd', 'e'], dtype=object)
>>> s.any()
'a'
>>> s.all()
'e'
This branch (with brute force function):
>>> import pandas as pd
>>> s = pd.Series(index=range(5), data=['a', 'b', 'c', 'd', 'e'], dtype=object)
>>> s.any()
True
>>> s.all()
True
can you merge master and we'll take a look |
I have merged master @jreback |
@MomIsBestFriend so the tests are failing, ping when green. |
@jreback I'm really not sure what to do, I see pretty much everywhere the error message of: AttributeError: type object 'bool' has no attribute 'any' No idea on where to even start. should I change the or change about 30 - 50 tests, it's just seems wrong to do so. |
try using np.any |
This also doesn't work. |
@MomIsBestFriend now that #29847 has been merged, I'm planning to do a significant cleanup of DataFrame._reduce and core.nanops. So I think the path forward is for you and I to trade tasks. Up for something new? |
Yes just take it away from me 😆 |
7864171
to
2217be0
Compare
2217be0
to
3492806
Compare
Closing this, because this I don't know how to fix it without breaking everything else. |
Did the issue eventually got fixed? Can someone tldr the thread? Thanks ! |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff