Repository: pandas-dev/pandas
Pull request: BUG: Make DTI/TDI/PI argsort match their underlying arrays
- [x] closes #37863
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

The fix to keep resample working is a kludge I'd like to make unnecessary before merging; cc @mroeschke

Problem statement:
API/BUG: DatetimeIndex.argsort does not match DatetimeArray.argsort
The DatetimeIndex returns `self.asi8.argsort(*args, **kwargs)`, while the DatetimeArray uses the M8[ns] values.  This puts NaTs at the front for DTI and the end for DTA.

Changing the DTI method (actually the DTI/TDI/PI methods) to match their array counterparts breaks 12 tests, 11 of them resample, 1 test_grouping.

Hints from issue discussion:
@jreback @mroeschke tracking this down im finding that changing it causes test failures largely in resample tests.

For an example, if we disable the needs_i8_conversion check in Index.argsort, then the following:

```
index = pd.to_timedelta(["0s", pd.NaT, "2s"])
df = pd.DataFrame({"value": [2, 3, 5]}, index)
rs = df.resample("1s")

>>> result = rs.mean()
pandas/core/resample.py:968: in g
    return self._downsample(_method)
pandas/core/resample.py:1080: in _downsample
    result = obj.groupby(self.grouper, axis=self.axis).aggregate(how, **kwargs)
pandas/core/groupby/generic.py:948: in aggregate
    result, how = aggregate(self, func, *args, **kwargs)
pandas/core/aggregation.py:563: in aggregate
    return obj._try_aggregate_string_function(arg, *args, **kwargs), None
pandas/core/base.py:313: in _try_aggregate_string_function
    return f(*args, **kwargs)
pandas/core/groupby/groupby.py:1491: in mean
    return self._cython_agg_general(
pandas/core/groupby/generic.py:1018: in _cython_agg_general
    agg_mgr = self._cython_agg_blocks(
pandas/core/groupby/generic.py:1116: in _cython_agg_blocks
    new_mgr = data.apply(blk_func, ignore_failures=True)
pandas/core/internals/managers.py:425: in apply
    applied = b.apply(f, **kwargs)
pandas/core/internals/blocks.py:369: in apply
    result = func(self.values, **kwargs)
pandas/core/groupby/generic.py:1067: in blk_func
    result, _ = self.grouper.aggregate(
pandas/core/groupby/ops.py:595: in aggregate
    return self._cython_operation(
pandas/core/groupby/ops.py:548: in _cython_operation
    result = self._aggregate(result, counts, values, codes, func, min_count)
pandas/core/groupby/ops.py:609: in _aggregate
    agg_func(result, counts, values, comp_ids, min_count)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

>   raise ValueError("len(index) != len(labels)")
E   ValueError: len(index) != len(labels)
```

If we breakpoint just before the call to ` result = self._aggregate(result, counts, values, codes, func, min_count)` we can see that in master (with the .asi8 argsort) `codes` is `array([0, 0, 2])` but with the TDA implementation codes is `array([0, 2])`.

The only relevant-looking argsort call is in `Grouper._set_grouper`, which looks like:

```
indexer = self.indexer = ax.argsort(kind="mergesort")

>>> indexer
array([0, 2, 1])   # <-- using the TDA.argsort implementation
>>> indexer
array([1, 0, 2])   # <-- using the .asi8 implementation in master
```

Any thoughts?
see this PR which I think will fix: https://github.com/pandas-dev/pandas/pull/37905, this PR: https://github.com/pandas-dev/pandas/pull/36198 (which going to close), but has a list of the issues

#27343
#33548
#35275

Commit message: BUG: Make DTI/TDI/PI argsort match their underlying arrays

Patch to review:
diff --git a/pandas/core/groupby/grouper.py b/pandas/core/groupby/grouper.py
index e8af9da30a298..38725a3c16b11 100644
--- a/pandas/core/groupby/grouper.py
+++ b/pandas/core/groupby/grouper.py
@@ -374,7 +374,8 @@ def _set_grouper(self, obj: FrameOrSeries, sort: bool = False):
         # possibly sort
         if (self.sort or sort) and not ax.is_monotonic:
             # use stable sort to support first, last, nth
-            indexer = self.indexer = ax.argsort(kind="mergesort")
+            # TODO: why does putting na_position="first" fix datetimelike cases?
+            indexer = self.indexer = ax.argsort(kind="mergesort", na_position="first")
             ax = ax.take(indexer)
             obj = obj.take(indexer, axis=self.axis)
 
diff --git a/pandas/core/indexes/base.py b/pandas/core/indexes/base.py
index 5209d83ade309..fb18b32aa4a73 100644
--- a/pandas/core/indexes/base.py
+++ b/pandas/core/indexes/base.py
@@ -4743,10 +4743,6 @@ def argsort(self, *args, **kwargs) -> np.ndarray:
         >>> idx[order]
         Index(['a', 'b', 'c', 'd'], dtype='object')
         """
-        if needs_i8_conversion(self.dtype):
-            # TODO: these do not match the underlying EA argsort methods GH#37863
-            return self.asi8.argsort(*args, **kwargs)
-
         # This works for either ndarray or EA, is overriden
         #  by RangeIndex, MultIIndex
         return self._data.argsort(*args, **kwargs)
diff --git a/pandas/tests/indexes/datetimelike.py b/pandas/tests/indexes/datetimelike.py
index 6f078237e3a97..a5a496e2a6ca2 100644
--- a/pandas/tests/indexes/datetimelike.py
+++ b/pandas/tests/indexes/datetimelike.py
@@ -9,6 +9,14 @@
 
 
 class DatetimeLike(Base):
+    def test_argsort_matches_array(self):
+        rng = self.create_index()
+        rng = rng.insert(1, pd.NaT)
+
+        result = rng.argsort()
+        expected = rng._data.argsort()
+        tm.assert_numpy_array_equal(result, expected)
+
     def test_argmax_axis_invalid(self):
         # GH#23081
         msg = r"`axis` must be fewer than the number of dimensions \(1\)"


Code review comments:
diff_hunk: @@ -374,7 +374,8 @@ def _set_grouper(self, obj: FrameOrSeries, sort: bool = False):
         # possibly sort
         if (self.sort or sort) and not ax.is_monotonic:
             # use stable sort to support first, last, nth
-            indexer = self.indexer = ax.argsort(kind="mergesort")
+            # TODO: why does putting na_position="first" fix datetimelike cases?
+            indexer = self.indexer = ax.argsort(kind="mergesort", na_position="first")
path: pandas/core/groupby/grouper.py
text: @user1:
```
   def argsort(
        self,
        ascending: bool = True,
        kind: str = "quicksort",
        na_position: str = "last",
 ```
in arrays/base.py so this is strange

@author:
i think it must involve the fact that on older numpys, dt64.argsort used to put NaT at the beginning instead of the end