BUG/DEPR: deprecate Categorical take default behaviour + fix Series[categorical].take

doc/source/whatsnew/v0.23.0.txt

@@ -856,6 +856,7 @@ Other API Changes
 - Constructing a Series from a list of length 1 no longer broadcasts this list when a longer index is specified (:issue:`19714`, :issue:`20391`).
 - :func:`DataFrame.to_dict` with ``orient='index'`` no longer casts int columns to float for a DataFrame with only int and float columns (:issue:`18580`)
 - A user-defined-function that is passed to :func:`Series.rolling().aggregate() <pandas.core.window.Rolling.aggregate>`, :func:`DataFrame.rolling().aggregate() <pandas.core.window.Rolling.aggregate>`, or its expanding cousins, will now *always* be passed a ``Series``, rather than a ``np.array``; ``.apply()`` only has the ``raw`` keyword, see :ref:`here <whatsnew_0230.enhancements.window_raw>`. This is consistent with the signatures of ``.aggregate()`` across pandas (:issue:`20584`)
+- The previous default behavior of negative indices in ``Categorical.take`` is deprecated. In a future version it will change from meaning missing values to meaning positional indicies from the right (:issue:`20664`)
 
 .. _whatsnew_0230.deprecations:
 

pandas/core/arrays/categorical.py

@@ -2,6 +2,7 @@
 
 import numpy as np
 from warnings import warn
+import textwrap
 import types
 
 from pandas import compat
@@ -29,7 +30,7 @@
     is_scalar,
     is_dict_like)
 
-from pandas.core.algorithms import factorize, take_1d, unique1d
+from pandas.core.algorithms import factorize, take_1d, unique1d, take
 from pandas.core.accessor import PandasDelegate
 from pandas.core.base import (PandasObject,
                               NoNewAttributesMixin, _shared_docs)
@@ -48,6 +49,17 @@
 from .base import ExtensionArray
 
 
+_take_msg = textwrap.dedent("""\
+    Interpreting negative values in 'indexer' as missing values.
+    In the future, this will change to meaning positional indicies
+    from the right.
+
+    Use 'allow_fill=True' to retain the previous behavior and silence this
+    warning.
+
+    Use 'allow_fill=False' to accept the new behavior.""")
+
+
 def _cat_compare_op(op):
     def f(self, other):
         # On python2, you can usually compare any type to any type, and
@@ -1732,17 +1744,25 @@ def fillna(self, value=None, method=None, limit=None):
         return self._constructor(values, categories=self.categories,
                                  ordered=self.ordered, fastpath=True)
 
-    def take_nd(self, indexer, allow_fill=True, fill_value=None):
-        """ Take the codes by the indexer, fill with the fill_value.
+    def take_nd(self, indexer, allow_fill=None, fill_value=None):
+        """
+        Return the indices
 
         For internal compatibility with numpy arrays.
         """
+        indexer = np.asarray(indexer, dtype=np.intp)
+        if allow_fill is None:
+            if (indexer < 0).any():
+                warn(_take_msg, FutureWarning, stacklevel=2)
+                allow_fill = True
 
-        # filling must always be None/nan here
-        # but is passed thru internally
-        assert isna(fill_value)
+        if fill_value is None or isna(fill_value):
+            # For backwards compatability, we have to override
+            # any na values for `fill_value`
+            fill_value = -1
 
-        codes = take_1d(self._codes, indexer, allow_fill=True, fill_value=-1)
+        codes = take(self._codes, indexer, allow_fill=allow_fill,
+                     fill_value=fill_value)
         result = self._constructor(codes, categories=self.categories,
                                    ordered=self.ordered, fastpath=True)
         return result

pandas/core/series.py

@@ -3499,7 +3499,14 @@ def _take(self, indices, axis=0, convert=True, is_copy=False):
 
         indices = _ensure_platform_int(indices)
         new_index = self.index.take(indices)
-        new_values = self._values.take(indices)
+
+        if is_categorical_dtype(self):
+            # https://github.com/pandas-dev/pandas/issues/20664
+            # TODO: remove when the default Categorical.take behavior changes
+            kwargs = {'allow_fill': False}
+        else:
+            kwargs = {}
+        new_values = self._values.take(indices, **kwargs)
 
         result = (self._constructor(new_values, index=new_index,
                                     fastpath=True).__finalize__(self))

pandas/tests/categorical/test_algos.py

@@ -5,6 +5,18 @@
 import pandas.util.testing as tm
 
 
+@pytest.fixture(params=[True, False])
+def allow_fill(request):
+    """Boolean 'allow_fill' parameter for Categorical.take"""
+    return request.param
+
+
+@pytest.fixture(params=[True, False])
+def ordered(request):
+    """Boolean 'ordered' parameter for Categorical."""
+    return request.param
+
+
 @pytest.mark.parametrize('ordered', [True, False])
 @pytest.mark.parametrize('categories', [
     ['b', 'a', 'c'],
@@ -69,3 +81,45 @@ def test_isin_empty(empty):
 
     result = s.isin(empty)
     tm.assert_numpy_array_equal(expected, result)
+
+
+class TestTake(object):
+    # https://github.com/pandas-dev/pandas/issues/20664
+
+    def test_take_warns(self):
+        cat = pd.Categorical(['a', 'b'])
+        with tm.assert_produces_warning(FutureWarning):
+            cat.take([0, -1])
+
+    def test_take_positive_no_warning(self):
+        cat = pd.Categorical(['a', 'b'])
+        with tm.assert_produces_warning(None):
+            cat.take([0, 0])
+
+    def test_take_bounds(self, allow_fill):
+        # https://github.com/pandas-dev/pandas/issues/20664
+        cat = pd.Categorical(['a', 'b', 'a'])
+        with pytest.raises(IndexError):
+            cat.take([4, 5], allow_fill=allow_fill)
+
+    def test_take_empty(self, allow_fill):
+        # https://github.com/pandas-dev/pandas/issues/20664
+        cat = pd.Categorical([], categories=['a', 'b'])
+        with pytest.raises(IndexError):
+            cat.take([0], allow_fill=allow_fill)
+
+    def test_positional_take(self, ordered):
+        cat = pd.Categorical(['a', 'a', 'b', 'b'], categories=['b', 'a'],
+                             ordered=ordered)
+        result = cat.take([0, 1, 2], allow_fill=False)
+        expected = pd.Categorical(['a', 'a', 'b'], categories=cat.categories,
+                                  ordered=ordered)
+        tm.assert_categorical_equal(result, expected)
+
+    def test_positional_take_unobserved(self, ordered):
+        cat = pd.Categorical(['a', 'b'], categories=['a', 'b', 'c'],
+                             ordered=ordered)
+        result = cat.take([1, 0], allow_fill=False)
+        expected = pd.Categorical(['b', 'a'], categories=cat.categories,
+                                  ordered=ordered)
+        tm.assert_categorical_equal(result, expected)

pandas/tests/series/indexing/test_indexing.py

@@ -753,6 +753,16 @@ def test_take():
         s.take([-1, 3, 4], convert=False)
 
 
+def test_take_categorical():
+    # https://github.com/pandas-dev/pandas/issues/20664
+    s = Series(pd.Categorical(['a', 'b', 'c']))
+    result = s.take([-2, -2, 0])
+    expected = Series(pd.Categorical(['b', 'b', 'a'],
+                      categories=['a', 'b', 'c']),
+                      index=[1, 1, 0])
+    assert_series_equal(result, expected)
+
+
 def test_head_tail(test_data):
     assert_series_equal(test_data.series.head(), test_data.series[:5])
     assert_series_equal(test_data.series.head(0), test_data.series[0:0])