Repository: pandas-dev/pandas
Pull request: BUG: Fix __ne__ comparison for Categorical
- [x] closes #32276
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Problem statement:
Categorical NaN behaviour different from a str
#### Code Sample

### Series as category
df = pd.Series(['a','a','b','c']).astype('category')
print(df.shift(1))
print(df)
print(df.shift(1) != df)

OUTPUT:

0    NaN
1      a
2      a
3      b
dtype: category
Categories (3, object): [a, b, c]
0    a
1    a
2    b
3    c
dtype: category
Categories (3, object): [a, b, c]
0    False
1    False
2     True
3     True
dtype: bool

### Series as str
df = pd.Series(['a','a','b','c']).astype('str')
print(df.shift(1))
print(df)
print(df.shift(1) != df)

OUTPUT:

0    NaN
1      a
2      a
3      b
dtype: object
0    a
1    a
2    b
3    c
dtype: object
0     True
1    False
2     True
3     True
dtype: bool


```
#### Problem description

The behaviour of NaN in comparison operators is different for type category and str. See example code - the first element is NaN in both instances, but the second instance equates to false, and the first equates to true for a != operation. For a == operation for a category, the behavior is as expected.

#### Expected Output

I would expect both to have the same output.

#### Output of ``pd.show_versions()``

<details>

INSTALLED VERSIONS
------------------
commit           : None
python           : 3.8.0.final.0
python-bits      : 64
OS               : Linux
OS-release       : 3.10.0-1062.12.1.el7.x86_64
machine          : x86_64
processor        : x86_64
byteorder        : little
LC_ALL           : None
LANG             : en_GB.UTF-8
LOCALE           : en_GB.UTF-8

pandas           : 1.0.1
numpy            : 1.18.1
pytz             : 2019.3
dateutil         : 2.8.1
pip              : 20.0.2
setuptools       : 41.4.0
Cython           : 0.29.15
pytest           : 5.3.5
hypothesis       : None
sphinx           : None
blosc            : None
feather          : None
xlsxwriter       : None
lxml.etree       : None
html5lib         : None
pymysql          : None
psycopg2         : None
jinja2           : 2.11.1
IPython          : 7.12.0
pandas_datareader: None
bs4              : None
bottleneck       : None
fastparquet      : None
gcsfs            : None
lxml.etree       : None
matplotlib       : 3.1.3
numexpr          : 2.7.1
odfpy            : None
openpyxl         : 3.0.3
pandas_gbq       : None
pyarrow          : None
pytables         : None
pytest           : 5.3.5
pyxlsb           : None
s3fs             : None
scipy            : 1.4.1
sqlalchemy       : 1.3.13
tables           : 3.6.1
tabulate         : None
xarray           : None
xlrd             : None
xlwt             : None
xlsxwriter       : None
numba            : None

</details>

Hints from issue discussion:
Thanks, this is a bug in the Categorical comparison operator

Commit message: Reference self

Patch to review:
diff --git a/doc/source/whatsnew/v1.1.0.rst b/doc/source/whatsnew/v1.1.0.rst
index 2b64b85863def..4ac4ff6c748b5 100644
--- a/doc/source/whatsnew/v1.1.0.rst
+++ b/doc/source/whatsnew/v1.1.0.rst
@@ -127,7 +127,7 @@ Bug fixes
 Categorical
 ^^^^^^^^^^^
 - Bug when passing categorical data to :class:`Index` constructor along with ``dtype=object`` incorrectly returning a :class:`CategoricalIndex` instead of object-dtype :class:`Index` (:issue:`32167`)
--
+- Bug where :class:`Categorical` comparison operator ``__ne__`` would incorrectly evaluate to ``False`` when either element was missing (:issue:`32276`)
 -
 
 Datetimelike
diff --git a/pandas/core/arrays/categorical.py b/pandas/core/arrays/categorical.py
index a5048e3aae899..a87ade10c1254 100644
--- a/pandas/core/arrays/categorical.py
+++ b/pandas/core/arrays/categorical.py
@@ -103,7 +103,10 @@ def func(self, other):
             mask = (self._codes == -1) | (other_codes == -1)
             if mask.any():
                 # In other series, the leads to False, so do that here too
-                ret[mask] = False
+                if opname == "__ne__":
+                    ret[mask & (self._codes == other_codes)] = True
+                else:
+                    ret[mask] = False
             return ret
 
         if is_scalar(other):
diff --git a/pandas/tests/extension/test_categorical.py b/pandas/tests/extension/test_categorical.py
index 69a97f5c9fe02..47e97cc3c70e7 100644
--- a/pandas/tests/extension/test_categorical.py
+++ b/pandas/tests/extension/test_categorical.py
@@ -282,6 +282,15 @@ def _compare_other(self, s, data, op_name, other):
             with pytest.raises(TypeError, match=msg):
                 op(data, other)
 
+    def test_not_equal_with_na(self):
+        # https://github.com/pandas-dev/pandas/issues/32276
+        categories = ["a", "b"]
+        c1 = Categorical([None, "a"], categories=categories)
+        c2 = Categorical(["a", "b"], categories=categories)
+
+        result = c1 != c2
+
+        assert result.all()
 
 class TestParsing(base.BaseParsingTests):
     pass


Code review comments:
diff_hunk: @@ -103,7 +103,10 @@ def func(self, other):
             mask = (self._codes == -1) | (other_codes == -1)
             if mask.any():
                 # In other series, the leads to False, so do that here too
-                ret[mask] = False
+                if opname == "__ne__":
+                    ret[mask & (self._codes == other_codes)] = True
path: pandas/core/arrays/categorical.py
text: @author:
This redundancy was intentional and an attempt to "reuse" the `mask` calculation above since I think it may short-circuit when `mask` is `False`, but could just make it `(self._codes == -1) & (other_codes == -1)` as well. I also think `__ne__` is the only situation that'd have to be special-cased here out of the comparison operators. Also worth pointing out that this is assuming we're dealing with `NaN` rather than `NA`.

@user1:
> `(self._codes == -1) & (other_codes == -1)`

I think this would be more clear.  Also there might be a cached `_isnan` attribute for this

@user1:
@author im working on cleaning up these comparisons, am confused by this line.  can we make this use the more common pattern

```
fill_value = True if op is operator.ne else False
[...]

ret = op(self._codes, other_codes)
mask = (self._codes == -1) | (other_codes == -1)
ret[mask] = fill_value
```

@author:
@user1 That is much simpler and seems correct to me