riptide_cpp icon indicating copy to clipboard operation
riptide_cpp copied to clipboard

Consolidate NaN/invalid handling in min/max/nanmin/nanmax.

Open jack-pappas opened this issue 3 years ago • 2 comments

Fixes rtosholdings/riptable#87 and rtosholdings/riptable#175.

This change fixes the max/nanmax/min/nanmin reduction kernels so they propgate NaNs (max, min) or ignore NaNs (nanmax, nanmin). The changes also consolidate + streamline some logic; for example, the vector register type is now derived from the element type provided in the template argument vs. being passed as a separate argument.

This change also implements part of the support needed for these functions to (selectively) handle integer invalids. That could be exposed in the future as a keyword argument to these ufuncs. Before the logic will work correctly, we'll need to implement invalid-recognizing versions of the MIN_OP, MAX_OP, etc. functions used in the AVX2-vectorized versions of these kernels; the logic for them should be fairly similar to the logic used by the floating-point MIN_OP, FMIN_OP, etc. after this change.

Before merging, I'll run some before/after benchmarks + try to put some proper tests together to run in this repo.

Testing / demonstration code:

Setup

>>> arr = rt.FA([np.nan, 0.0, 0.1, 0.2, np.nan, 0.3], dtype=np.float64).repeat(100)
>>> arr
FastArray([nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3,
           nan, 0. , 0.1, 0.2, nan, 0.3, nan, 0. , 0.1, 0.2, nan, 0.3])

Before

>>> rt.max(arr)
0.3
>>> rt.nanmax(arr)
0.3
>>> rt.min(arr)
0.3
>>> rt.nanmin(arr)
0.0
>>> arr = rt.arange(1000).astype(np.float64)
>>> rt.max(arr)
999.0
>>> rt.nanmax(arr)
999.0
>>> rt.min(arr)
0.0
>>> rt.nanmin(arr)
0.0

After

>>> rt.max(arr)
nan
>>> rt.nanmax(arr)
0.3
>>> rt.min(arr)
nan
>>> rt.nanmin(arr)
0.0
>>> arr = rt.arange(1000).astype(np.float64)
>>> arr[501] = np.nan
>>> rt.max(arr)
nan
>>> rt.nanmax(arr)
999.0
>>> rt.min(arr)
nan
>>> rt.nanmin(arr)
0.0
>>> ints = rt.FA([0, 2, 1, 4, 3, 5], dtype=np.int8)
>>> rt.max(arr)
nan
>>> rt.nanmax(arr)
999.0
>>> rt.max(ints)
5
>>> rt.nanmax(ints)
5
>>> rt.min(ints)
0
>>> rt.nanmin(ints)
0
>>> intsinv = ints.copy()
>>> intsinv[3] = intsinv.inv
>>> rt.max(intsinv)
5
>>> rt.nanmax(intsinv)
5
>>> rt.min(intsinv)
-128
>>> rt.nanmin(intsinv)
-128

jack-pappas avatar Mar 22 '22 18:03 jack-pappas

This work should maybe also take a look at the nan-propagation behavior in the group-based versions of these functions to ensure consistent behavior. (Either in this PR or in a follow-up PR.)

jack-pappas avatar Mar 22 '22 18:03 jack-pappas

The failing AVX2 specializations might be better off as constexpr's - not sure if that will work out, but could be worth trying.

staffantj avatar Mar 22 '22 18:03 staffantj