It seems currently NaT is completely ignored for min/max and argmin/argmax. I wonder if we should push to change that to be identical NaN (NaN is always returned). I guess if it that bites someone, on the up-side it probably will be loud since it points to NaT.

There is also an independend bug for argmin when it starts with NaT and has only one not NaT item at the end:

--- a/numpy/core/src/multiarray/arraytypes.c.src
+++ b/numpy/core/src/multiarray/arraytypes.c.src
@@ -3198,7 +3198,7 @@ static int
         mp = ip[i];
         i++;
     }
-    if (i == n) {
+    if (i - 1 == n) {
         /* All NaTs: return 0 */
         *min_ind = 0;
         return 0;

EDIT: Or probably nicer, just init it to -1 or so. The loop is not called for empty arrays.

It is LT, so I guess to keep sort order equal should evaluate to false

Agree. If the two quantities are the same, NaT, NaN or not, then one cannot be < the other. That is for sorting only and independent of their "normal" arithmetic.

Needs a release note.

Added release note, versionchanged directive, and adopted the first ternary operator that @mhvk suggested (second one didn't pass my unit tests). Good to verify that adjustment--there was some back & forth there between reviewers.

Perhaps we can get away with the adjustment to other functions in separate PRs?

@tylerjereddy, I think separate PRs should be fine. Just need to open an issue with 1.17.0 milestone, because I would hate to not do it all in the same release.

Ok, I restored logic closer to my original instead of one of the reviewer-suggested ternary operators. It is just easier for me to read it. I did apply some of the suggested simplifications to those conditionals.

I added a test for argsort stabillity with all NaT values in an array. This seems ok as long as I force mergesort. It looks like the other stability tests also force mergesort. That does make sense of course, but I wonder about the other sorting code paths.

I did notice that some of the sorting algorithms will segfault with NaT if LT implementation is not correct (i.e., some of them would segfault with one of the ternary operators suggested above). That means we don't test the NaT argsort codepaths with other sort algorithms. Do we want something crude parametrized over the sorting kinds just to make sure there's no major issue (i.e., all-NaT array in sorts to all-NaT array out)?

The other sorts will change the relative position of even identical elements, the tests for them is simply that the result be sorted. We will probably want parametrized tests at some point, especially with type specific sorts like the upcoming radix sort. Both timsort and radixsort should also be stable, so we will need to expand the tests for those. A new test_sorting.py module is looking like the way to go, that whole library has grown over the years.

@tylerjereddy do you want to put this in and you/I can do the rest in a followup, or should we/I continue here? Got to resolve the conflicts, but I can do that.

Are we concerned about deprecation? I guess opening a tracking issue for the other things we'd need to do before 1.17 related to this may be useful.

Yeah, you are right. It is a bit annoying because it will create FutureWarnings with no good way of silencing them (except manually removing the NaTs, or, I suppose, view(np.int64) is a hackish way to do it in a more or less future proof way). For min/max it is a bit "nicer" because at least the result is pretty special.
It all feels painful to deprecate, but I suppose few people will actually hit it.

I'm happy if we want to delay this to 1.18. I think 1.17 is getting quite a lot of stuff already?

Sounds OK to me, I do not remember that there was something specific that made it super pressing. Only feel it should keep in sync with the argmin/argmax issue. Moved both milestones.

We need to fix up the release notes. We just opened a fixup for the (somewhat, but maybe not exactly) related fixes to min/max and argmin/argmax. So I would like to put this into 1.18.

Rebased and reformated release note for 1.18

Let's put this in and see what feedback we get.

Thanks Tyler.

It seems this only has been done for datetime64, but not for timedelta64 (introducing another inconsistency). That might be something to still consider for 1.8 ?