BUG: fix categorical comparison with missing values (#26504 )
State: closed

- [x] closes #26504
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

This PR fixes issues comparison of ordered categorical with missing values evaluates to True(#26504 ).  Now making comparison with missing values always gives False. The missing values could be introduced when constructing a Categorical with values that don't fall in with the categories provided. 

Labels:
name: Bug
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: Categorical
description: Categorical Data Type

Comments:
Hello @yanglinlee! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:





There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-06-01 00:32:57 UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=h1) Report
> Merging [#26514](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/b730ab35e8fe1540d121cf7b336f454df59f7d4d?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/26514/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #26514      +/-   ##
==========================================
- Coverage   91.76%   91.75%   -0.01%     
==========================================
  Files         174      174              
  Lines       50679    50684       +5     
==========================================
  Hits        46504    46504              
- Misses       4175     4180       +5
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.26% <100%> (ø)` | :arrow_up: |
| #single | `41.71% <100%> (-0.1%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/26514/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.93% <100%> (+0.02%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/26514/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `78.94% <0%> (-10.53%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/26514/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97% <0%> (-0.12%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/26514/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `90.6% <0%> (-0.11%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=footer). Last update [b730ab3...cf156cf](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=h1) Report
> Merging [#26514](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/c6a7cc1e08f9203caf57599244cd1c51f6347875?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/26514/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #26514      +/-   ##
==========================================
- Coverage   91.84%   91.83%   -0.01%     
==========================================
  Files         174      174              
  Lines       50644    50647       +3     
==========================================
- Hits        46515    46514       -1     
- Misses       4129     4133       +4
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.37% <100%> (ø)` | :arrow_up: |
| #single | `41.69% <57.14%> (-0.09%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/26514/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.92% <100%> (+0.01%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/26514/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `78.94% <0%> (-10.53%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/26514/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97% <0%> (-0.12%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=footer). Last update [c6a7cc1...d83c4f4](https://codecov.io/gh/pandas-dev/pandas/pull/26514?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@yanglinlee : Thanks for submitting this!  Can you also add a `whatsnew` entry to `v0.25.0.rst?`
> @yanglinlee : Thanks for submitting this! Can you also add a `whatsnew` entry to `v0.25.0.rst?`

Sure! Done~
> I am puzzled of the purpose of this PR. We don't' allow _any_ comparison of categorical except for eq / ne for non-ordered. I would be ok with simply raising the same TypeError message that we already do. But what is the purpose of this expansion?

Just want to get some clarification: 
Scenario 1: Missing values. 
```
In [50]:
cat = pd.Categorical([1, 2, 3, None], categories=[1, 2, 3], ordered=True) 
cat
Out[50]:
[1, 2, 3, NaN]
Categories (3, int64): [1 < 2 < 3]
In [51]: 
cat <=2
Out[51]:
array([ True,  True, False,  True])
```
If we allow missing values in the array, then the result should not be raising errors.
People suggest the result in Out[51] should be `array([ True,  True, False,  False])`

Scenario 2: Values that don't fall into a category as WillAyd suggested.
```
In [54]:
cat2 = pd.Categorical([1, 2, 3, 999], categories=[1, 2, 3], ordered=True) 
cat2
Out[54]:
[1, 2, 3, NaN]
Categories (3, int64): [1 < 2 < 3]
In [55]:
cat2<=2
Out[55]:
array([ True,  True, False,  True])
```
Currently it doesn't raise TypeError. 

If you want to raise TypeError for both scenarios, I can change the behavior of the comparison with scalar. Let me know your suggestions. Thanks.

if the above scenarios compare with a listlike they raise i believe
a scalar should not be different
> if the above scenarios compare with a listlike they raise i believe
> a scalar should not be different

I tested comparison with listlike, and it didn't raise TypeError. It only takes comparison with None as False. 
```
cat1 = pd.Categorical([1,2,3,999], categories=[1,2,3], ordered=True)
cat2 = pd.Categorical([2,2,2,2], categories=[1,2,3], ordered=True)
cat1
Out[]:
[1, 2, 3, NaN]
Categories (3, int64): [1 < 2 < 3]
In[]:
cat1 >= cat2
Out[]:
array([False,  True,  True, False])
```
In this case, do we need to change the behavior of comparison to listlike? So in both cases, the comparison to None will raise Error.

> I am puzzled of the purpose of this PR.

The purpose is quite clear to me: fixing a bug. Which is clearly described in the issue: https://github.com/pandas-dev/pandas/issues/26504

The bug is that when doing a comparison (which is allowed for ordered categoricals), missing values always evaluate to True or False (depending on which operations, because they are encoded as -1 and we use that value in the comparison). 
While comparisons to NaN should always be False.

To show this in another way:

```
In [49]: cat = pd.Categorical([1, 2, 3, None], categories=[1, 2, 3], ordered=True)

In [50]: pd.Series(cat)           
Out[50]: 
0      1
1      2
2      3
3    NaN
dtype: category
Categories (3, int64): [1 < 2 < 3]

In [51]: pd.Series(cat) < 2    
Out[51]: 
0     True
1    False
2    False
3     True
dtype: bool

In [52]: pd.Series(cat, dtype=object) < 2                     
Out[52]: 
0     True
1    False
2    False
3    False
dtype: bool
```

The object dtype has the correct behaviour: a missing value gives False in the result.


> So to be clear this has nothing to do with the None value right?

This is not about "None", or also not about values that don't fall within the category, but is is about *missing values* (but both None as non-existent categories when constructing the categorical lead to missing values in the actual categorical) 
this needs to raise for non ordered categoricals

that is not addressed in this PR as there are no tests for this

furthermore the behavior for listlikes must also be the same (raise for non ordered)
if u r making work for scalars then it needs to work for listlikes as well (for ordered)

> this needs to raise for non ordered categoricals

> that is not addressed in this PR as there are no tests for this

I think the issue was about ordered Categoricals. unordered does indeed raise

```pytb
In [3]: import pandas as pd
   ...: pd.Categorical(["1", "2", "3", None], categories=["1", "2", "3"]) <= "2"
   ...:
   ...:
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-1804d8ae703c> in <module>
      1 import pandas as pd
----> 2 pd.Categorical(["1", "2", "3", None], categories=["1", "2", "3"]) <= "2"
      3

~/sandbox/pandas/pandas/core/arrays/categorical.py in f(self, other)
     64         if not self.ordered:
     65             if op in ['__lt__', '__gt__', '__le__', '__ge__']:
---> 66                 raise TypeError("Unordered Categoricals can only compare "
     67                                 "equality or not")
     68         if isinstance(other, Categorical):

TypeError: Unordered Categoricals can only compare equality or not
```
> this needs to raise for non ordered categoricals

This does already raise, that is not the issue. 

> furthermore the behavior for listlikes must also be the same. if u r making work for scalars then it needs to work for listlikes as well (for ordered)

This does already work for listlikes. Actually this PR basically copied the code from the code branch for list-likes to the code branch of scalars.
> This is not about "None", or also not about values that don't fall within the category, but is is about _missing values_ (but both None as non-existent categories when constructing the categorical lead to missing values in the actual categorical)

Gotcha - so in both in the original issue and here in the whatsnew we keep referring to `None` as being of importance but that is distracting from the actual issue. 

@yanglinlee looks like you might have some other changes to the whatsnew but can you account for that on next edit? Ping back if you are stuck on wording

> Gotcha - so in both in the original issue and here in the whatsnew we keep referring to None as being of importance but that is distracting from the actual issue.

Yes, it was by using "None" instead of "missing values" that caused some confusion I think. I already changed the title of this PR.
Thank everyone for the helpful discussions! This is great suggestion! I will update the PR after work tonight.
@yanglinlee CI is being fixed in https://github.com/pandas-dev/pandas/pull/26586. Merge master & repush once that's in.
thanks @yanglinlee