BUG: isin fails to correctly compare None values. #41145
Conversation
DriesSchaumont
commented
Apr 24, 2021
•
DriesSchaumont
commented
- closes different behaviour of df.isin() in 1.0.5/1.1.0, when df contains None #35565
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
| @@ -160,17 +160,19 @@ def vec_compare(ndarray[object] left, ndarray[object] right, object op) -> ndarr | |||
| for i in range(n): | |||
| x = left[i] | |||
| y = right[i] | |||
|
|
|||
| if checknull(x) or checknull(y): | |||
| if x is None and y is None: | |||
There was a problem hiding this comment.
this is superfluous as checknull already checks None
There was a problem hiding this comment.
Yep, this was incorrect. Changed.
There was a problem hiding this comment.
what was incorrect?
rule of thumb: let the question-asker mark the conversation as resolved
| result[i] = True | ||
| else: | ||
| result[i] = PyObject_RichCompareBool(x, y, flag) | ||
| else: | ||
| for i in range(n): | ||
| x = left[i] | ||
| y = right[i] | ||
|
|
||
| if checknull(x) or checknull(y): | ||
| if x is None and y is None: |
There was a problem hiding this comment.
this is contrary to what we do with nulls generally, does anything break when you make this change (only)?
There was a problem hiding this comment.
AFAIK, this does not break any tests. However, I am starting to think that #35565 is not actually a bug, but intended behavior. From the docs: "One has to be mindful that in Python (and NumPy), the nan's don’t compare equal, but None's do. Note that pandas/NumPy uses the fact that np.nan != np.nan, and treats None like np.nan."
I see the same behavior in scalar_compare:
Lines 74 to 86 in 1269bc6
I found this related issue: #34975
Should we close this PR and the issue, then?
jreback
added
Algos
simonjayhawkins
added
Bug
Regression
|
@DriesSchaumont can you respond to jreback comments |
|
Hi @jbrockmendel, I am in the middle of moving to a new place and I currently have limited internet access. I will try to come back to this PR as soon as possible. I hope this is not blocking anything? |
no, not to worry |
|
Seems I accidentally pushed a commit that was meant to be in another branch. Updating this PR now. I will add a comment when this is ready for review. |
|
if possible we should reuse libmissing.is_matching_na |
|
closing as stale, if you want to continue working, please ping. |
