Repository: numpy/numpy
Pull request: BUG: Fix undefined behavior when converting NaN float16 to datetime or timedelta
Resolves #21762.

In this change to the casting of HALF types to DATETIME or TIMEDELTA, the case in which the input is NaN is handled appropriately by casting to NaT.

I am not sure where to add additional unit tests, if any; any feedback would be appreciated.

Thank you!

Problem statement:
BUG: float16 to datetime/timedelta goes via undefined behavior for NaNs
### Describe the issue:

When converting a float array to datetime/timedelta, NaNs should be converted to NaT (minimum integer).  On many platforms, that is just what happens, however, this is undefined behavior.

This was fixed by gh-14669.  But that PR is missing a fix for `float16 -> datetime/timedelta` casts.  Most platforms are fine, since the weirder behavior typically only gets triggered by SIMD instructions, and we won't use those when half is involved

(xref gh-13917 for the old issue about this)

### Reproduce the code example:

```python
import numpy as np

# the following invokes undefined behavior and should fail to return
# an all NaT array on some platforms:
arr = np.array([np.nan] * 5, dtype=np.float16).astype("m8[s]")
assert np.isnat(arr).all()
```

EDIT: This **can** actually be tested on NumPy main now on most platforms, because it will give an "invalid value" warning.  The correct code would not give such warning.

Hints from issue discussion:
Also, to make matters a bit insane, the `isnan` function still returns `True`
```py
>>> import numpy as np
>>> arr = np.array([np.nan] * 5, dtype='float16').astype("m8[s]")
>>> np.isnat(arr).all()
True
>>> np.isnan(arr).all()
True
```
I don't think it is possible to have both `NaN` and `NaT` types simultaneously.

EDIT: Turns out the above is also true for `float32` and `float64` datatypes.
That is a feature, not a bug.  We decided that `np.isnan` should also work for NaT since it makes live much easier for things like `np.nanmean` and has the same semantics.
> That is a feature, not a bug.

That was exactly my initial thought but the `00-bug` tag made me feel I was wrong but on the positive note, it means this wasn't even a problem to begin with.
@Siddhesh-Agarwal the bug is platform dependent.  The *first* line is the problem.  But unless you use the very latest `main` branch, you won't notice the warning I mentioned (which should not happen).  And unless you are on very specific hardware, the `assert` will not fail.

Commit message: BUG: Fix undefined behavior when converting NaN float16 to datetime or timedelta

Patch to review:
diff --git a/numpy/_core/src/multiarray/arraytypes.c.src b/numpy/_core/src/multiarray/arraytypes.c.src
index 9cfd9f59cd51..caa496006f4c 100644
--- a/numpy/_core/src/multiarray/arraytypes.c.src
+++ b/numpy/_core/src/multiarray/arraytypes.c.src
@@ -1399,7 +1399,15 @@ HALF_to_@TYPE@(void *input, void *output, npy_intp n,
     @type@ *op = output;
 
     while (n--) {
-        *op++ = (@type@)npy_half_to_float(*ip++);
+        @type@ t;
+        if (npy_half_isnan(*ip)) {
+            t = (@type@)NPY_DATETIME_NAT;
+        }
+        else {
+            t = (@type@)npy_half_to_float(*ip++);
+        }
+
+        *op++ = t;
     }
 }
 
diff --git a/numpy/_core/tests/test_datetime.py b/numpy/_core/tests/test_datetime.py
index 4a2ee96d3f08..299d2d8de8c3 100644
--- a/numpy/_core/tests/test_datetime.py
+++ b/numpy/_core/tests/test_datetime.py
@@ -585,6 +585,7 @@ def test_datetime_nat_casting(self):
         cnan = nan.astype('D')
         cfnan = nan.astype('F')
         clnan = nan.astype('G')
+        hnan = nan.astype(np.half)
 
         nat = np.array([np.datetime64('NaT')] * 8)
         assert_equal(nan.astype('M8[ns]'), nat)
@@ -593,6 +594,7 @@ def test_datetime_nat_casting(self):
         assert_equal(cnan.astype('M8[ns]'), nat)
         assert_equal(cfnan.astype('M8[ns]'), nat)
         assert_equal(clnan.astype('M8[ns]'), nat)
+        assert_equal(hnan.astype('M8[ns]'), nat)
 
         nat = np.array([np.timedelta64('NaT')] * 8)
         assert_equal(nan.astype('timedelta64[ns]'), nat)
@@ -601,6 +603,7 @@ def test_datetime_nat_casting(self):
         assert_equal(cnan.astype('timedelta64[ns]'), nat)
         assert_equal(cfnan.astype('timedelta64[ns]'), nat)
         assert_equal(clnan.astype('timedelta64[ns]'), nat)
+        assert_equal(hnan.astype('timedelta64[ns]'), nat)
 
     def test_days_creation(self):
         assert_equal(np.array('1599', dtype='M8[D]').astype('i8'),


Code review comments:
diff_hunk: @@ -1399,7 +1399,15 @@ HALF_to_@TYPE@(void *input, void *output, npy_intp n,
     @type@ *op = output;
 
     while (n--) {
-        *op++ = (@type@)npy_half_to_float(*ip++);
+        @type@ t;
+        if (npy_half_isnan(*ip)) {
+            t = (@type@)NPY_DATETIME_NAT;
+        }
+        else {
+            t = (@type@)npy_half_to_float(*ip++);
path: numpy/_core/src/multiarray/arraytypes.c.src
text: @user1:
Sorry, missed this:  This is actually incorrect.  It would be good to also modify the tests to end with a 0 value in the NaN array to catch this.

The `ip` pointer needs to be advanced no matter the branch.

@author:
Oops, thanks! Sorry to miss that. I fixed this and modified the tests to catch this behaviour. 

@user1:
Would you mind just moving the `ip++` into at the end together with the `*op++ = t`?  I don't think it is very nice to advance within the branches.

@author:
I agree, just did that!