Repository: pandas-dev/pandas
Pull request: BUG: df.sort_values() not respecting na_position with categoricals #22556
- [x] closes #22556 
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Problem statement:
df.sort_values() not respecting na_position with categoricals
#### Problem description
`DataFrame.sort_values()` appears not to respect the `na_position` parameter when sorting by a categorical series:

```python
>>> import pandas as pd
>>> c = pd.Categorical(['A', np.nan, 'B'], categories=['A','B'], ordered=True)
>>> df = pd.DataFrame({'c': c})
>>> df.sort_values(by='c', na_position='first')
     c
1  NaN
0    A
2    B
>>> df.sort_values(by='c', na_position='last')
     c
1  NaN
0    A
2    B
```
Unexpectedly, the NaNs always come first regardless of `na_position`.

#### Additional information
`Series.sort_values()` works as expected:
```python
>>> c.sort_values(na_position='first')
[NaN, A, B]
Categories (2, object): [A < B]
>>> c.sort_values(na_position='last')
[A, B, NaN]
Categories (2, object): [A < B]
```

Strangely, `df.sort_values()` does seem to respect `na_position` if you sort by more than one column (even the same column):
```python
>>> df.sort_values(by=['c','c'], na_position='first')
     c
1  NaN
0    A
2    B
>>> df.sort_values(by=['c','c'], na_position='last')
     c
0    A
2    B
1  NaN
```

#### Output of ``pd.show_versions()``

<details>

```
INSTALLED VERSIONS
------------------
commit: None
python: 3.7.0.final.0
python-bits: 64
OS: Windows
OS-release: 7
machine: AMD64
processor: Intel64 Family 6 Model 94 Stepping 3, GenuineIntel
byteorder: little
LC_ALL: None
LANG: None
LOCALE: None.None

pandas: 0.23.4
pytest: None
pip: 10.0.1
setuptools: 40.0.0
Cython: None
numpy: 1.15.0
scipy: None
pyarrow: None
xarray: None
IPython: 6.5.0
sphinx: None
patsy: None
dateutil: 2.7.3
pytz: 2018.5
blosc: None
bottleneck: None
tables: None
numexpr: None
feather: None
matplotlib: None
openpyxl: None
xlrd: None
xlwt: None
xlsxwriter: None
lxml: None
bs4: None
html5lib: None
sqlalchemy: None
pymysql: None
psycopg2: None
jinja2: None
s3fs: None
fastparquet: None
pandas_gbq: None
pandas_datareader: None
```
</details>

Hints from issue discussion:
How odd!  Investigation and PR are welcome!
c = pd.Series(['A', np.nan, 'B'])
using Series instead of Categorical, this problem can be tackled.
I think the problem is in pandas/core/sorting.py

```
def nargsort(items, kind='quicksort', ascending=True, na_position='last'):
    """
    This is intended to be a drop-in replacement for np.argsort which
    handles NaNs. It adds ascending and na_position parameters.
    GH #6399, #5231
    """


    # specially handle Categorical
    if is_categorical_dtype(items):
        return items.argsort(ascending=ascending, kind=kind)
```

For categorically, we don't pass `na_position`

My suggestion: if `na_position` is first, put nan first, else put it last (Of course one has to check if nan is in etc).  Any other suggestions? I'd like to work on this

Btw: Series does not use this sorting function, which I find a bit odd. Additionally, the problem does not appear when using `by=['c', 'c']`in zapnat's example because a lex sorter is used then.


i think this is a duplicate issue - pls do a search

Commit message: flake8 linter fix

Patch to review:
diff --git a/doc/source/whatsnew/v0.24.0.txt b/doc/source/whatsnew/v0.24.0.txt
index fb7af00f61534..f02bd1557ef34 100644
--- a/doc/source/whatsnew/v0.24.0.txt
+++ b/doc/source/whatsnew/v0.24.0.txt
@@ -608,6 +608,7 @@ Categorical
 ^^^^^^^^^^^
 
 - Bug in :meth:`Categorical.from_codes` where ``NaN`` values in `codes` were silently converted to ``0`` (:issue:`21767`). In the future this will raise a ``ValueError``. Also changes the behavior of `.from_codes([1.1, 2.0])`.
+- Bug in :meth:`Categorical.sort_values` where ``NaN`` values were always positioned in front regardless of ```na_position`` value. (:issue:`22556`). `test_stata.py` was incorrectly passing using default ``na_position='last'``.
 
 Datetimelike
 ^^^^^^^^^^^^
diff --git a/pandas/core/sorting.py b/pandas/core/sorting.py
index 5aa9ea658482b..49552788ed930 100644
--- a/pandas/core/sorting.py
+++ b/pandas/core/sorting.py
@@ -241,7 +241,19 @@ def nargsort(items, kind='quicksort', ascending=True, na_position='last'):
 
     # specially handle Categorical
     if is_categorical_dtype(items):
-        return items.argsort(ascending=ascending, kind=kind)
+        if na_position not in ['first', 'last']:
+            raise ValueError('invalid na_position: {!r}'.format(na_position))
+        mask = isna(items)
+        cnt_null = mask.sum()
+        sorted_idx = items.argsort(ascending=ascending, kind=kind)
+        if ascending:
+            # NaN is coded as -1 and is listed in front after sorting
+            return sorted_idx if na_position == 'first' \
+                else np.roll(sorted_idx, -cnt_null)
+        else:
+            # NaN is coded as -1 and is listed in the end after sorting
+            return sorted_idx if na_position == 'last' else \
+                np.roll(sorted_idx, cnt_null)
 
     items = np.asanyarray(items)
     idx = np.arange(len(items))
diff --git a/pandas/tests/frame/test_sorting.py b/pandas/tests/frame/test_sorting.py
index 599ae683f914b..434a74b6109bd 100644
--- a/pandas/tests/frame/test_sorting.py
+++ b/pandas/tests/frame/test_sorting.py
@@ -10,7 +10,7 @@
 from pandas.compat import lrange
 from pandas.api.types import CategoricalDtype
 from pandas import (DataFrame, Series, MultiIndex, Timestamp,
-                    date_range, NaT, IntervalIndex)
+                    date_range, NaT, IntervalIndex, Categorical)
 
 from pandas.util.testing import assert_series_equal, assert_frame_equal
 
@@ -598,3 +598,55 @@ def test_sort_index_intervalindex(self):
             closed='right')
         result = result.columns.levels[1].categories
         tm.assert_index_equal(result, expected)
+
+    def test_sort_index_na_position_with_categories(self):
+        # GH 22556
+        # Positioning missing value properly when column is Categorical.
+        df_category_with_nan = pd.DataFrame({
+            'c': pd.Categorical(['A', np.nan, 'B', np.nan, 'C'],
+                                categories=['A', 'B', 'C'],
+                                ordered=True)})
+        result = df_category_with_nan.sort_values(by='c',
+                                                  ascending=True,
+                                                  na_position='first')
+        expected = DataFrame({
+            'c': Categorical([np.nan, np.nan, 'A', 'B', 'C'],
+                             categories=['A', 'B', 'C'],
+                             ordered=True)}, index=[1, 3, 0, 2, 4])
+
+        assert_frame_equal(result, expected)
+
+        result = df_category_with_nan.sort_values(by='c',
+                                                  ascending=True,
+                                                  na_position='last')
+        expected = DataFrame({
+            'c': Categorical(['A', 'B', 'C', np.nan, np.nan],
+                             categories=['A', 'B', 'C'],
+                             ordered=True)}, index=[0, 2, 4, 1, 3])
+
+        assert_frame_equal(result, expected)
+
+        result = df_category_with_nan.sort_values(by='c',
+                                                  ascending=False,
+                                                  na_position='first')
+        expected = DataFrame({
+            'c': Categorical([np.nan, np.nan, 'C', 'B', 'A'],
+                             categories=['A', 'B', 'C'],
+                             ordered=True)}, index=[3, 1, 4, 2, 0])
+
+        assert_frame_equal(result, expected)
+
+        result = df_category_with_nan.sort_values(by='c',
+                                                  ascending=False,
+                                                  na_position='last')
+        expected = DataFrame({
+            'c': Categorical(['C', 'B', 'A', np.nan, np.nan],
+                             categories=['A', 'B', 'C'],
+                             ordered=True)}, index=[4, 2, 0, 3, 1])
+
+        assert_frame_equal(result, expected)
+
+        with pytest.raises(ValueError):
+            df_category_with_nan.sort_values(by='c',
+                                             ascending=False,
+                                             na_position='bad_position')
diff --git a/pandas/tests/io/test_stata.py b/pandas/tests/io/test_stata.py
index cfe47cae7e5e1..83b5290d82890 100644
--- a/pandas/tests/io/test_stata.py
+++ b/pandas/tests/io/test_stata.py
@@ -997,7 +997,7 @@ def test_categorical_sorting(self, file):
         parsed = read_stata(getattr(self, file))
 
         # Sort based on codes, not strings
-        parsed = parsed.sort_values("srh")
+        parsed = parsed.sort_values("srh", na_position='first')
 
         # Don't sort index
         parsed.index = np.arange(parsed.shape[0])


Code review comments:
diff_hunk: @@ -608,6 +608,7 @@ Categorical
 ^^^^^^^^^^^
 
 - Bug in :meth:`Categorical.from_codes` where ``NaN`` values in `codes` were silently converted to ``0`` (:issue:`21767`). In the future this will raise a ``ValueError``. Also changes the behavior of `.from_codes([1.1, 2.0])`.
+- Bug in :meth:`Categorical.sort_values` where ``NaN`` values were always positioned in front regardless of ```na_position`` value. (:issue:`22556`). `test_stata.py` was incorrectly passing using default ``na_position='last'``.
path: doc/source/whatsnew/v0.24.0.txt
text: @user1:
Extra backtick before `na_position`

diff_hunk: @@ -598,3 +598,55 @@ def test_sort_index_intervalindex(self):
             closed='right')
         result = result.columns.levels[1].categories
         tm.assert_index_equal(result, expected)
+
+    def test_sort_index_na_position_with_categories(self):
+        # GH 22556
+        # Positioning missing value properly when column is Categorical.
+        df_category_with_nan = pd.DataFrame({
path: pandas/tests/frame/test_sorting.py
text: @user1:
Can you parametrize these various cases?

diff_hunk: @@ -241,7 +241,19 @@ def nargsort(items, kind='quicksort', ascending=True, na_position='last'):
 
     # specially handle Categorical
     if is_categorical_dtype(items):
-        return items.argsort(ascending=ascending, kind=kind)
+        if na_position not in ['first', 'last']:
+            raise ValueError('invalid na_position: {!r}'.format(na_position))
+        mask = isna(items)
+        cnt_null = mask.sum()
+        sorted_idx = items.argsort(ascending=ascending, kind=kind)
+        if ascending:
+            # NaN is coded as -1 and is listed in front after sorting
+            return sorted_idx if na_position == 'first' \
path: pandas/core/sorting.py
text: @user1:
We typically use implicit line continuation where possible. If still needed after refactor of above condition would prefer that style

diff_hunk: @@ -241,7 +241,19 @@ def nargsort(items, kind='quicksort', ascending=True, na_position='last'):
 
     # specially handle Categorical
     if is_categorical_dtype(items):
-        return items.argsort(ascending=ascending, kind=kind)
+        if na_position not in ['first', 'last']:
+            raise ValueError('invalid na_position: {!r}'.format(na_position))
+        mask = isna(items)
+        cnt_null = mask.sum()
+        sorted_idx = items.argsort(ascending=ascending, kind=kind)
+        if ascending:
path: pandas/core/sorting.py
text: @user1:
Can you simplify the condition here to just negate `cnt_null` where required? Return can happen outside of condition

@author:
good point. not sure how to do condition check and negate `cnt_null` only once. updated to keep the `np.roll` only when needed and took return outside of condition.