Skip to content

Fix TensorPrimitives MinNumber/MaxNumber span reductions propagating NaN (#133346) #133628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jabrailkhalil wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix TensorPrimitives MinNumber/MaxNumber span reductions propagating NaN (#133346) #133628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ...nsors/src/System/Numerics/Tensors/netcore/Common/TensorPrimitives.IAggregationOperator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,15 @@ private interface IAggregationOperator<T> : IBinaryOperator<T>
static abstract T Invoke(Vector256<T> x);
static abstract T Invoke(Vector512<T> x);

/// <summary>
/// Whether the operator propagates NaN inputs to its output, as the IEEE 754:2019
/// <c>minimum</c>/<c>maximum</c> (and magnitude) functions do. Operators implementing the
/// <c>minimumNumber</c>/<c>maximumNumber</c> family return <see langword="false"/>, so the
/// reduction does not early-exit on a NaN and the lane-wise operator gets to ignore it
/// when a numeric operand is available.
/// </summary>
static virtual bool PropagatesNaNs => true;

static virtual T IdentityValue => throw new NotSupportedException();
}

Expand Down
22 changes: 11 additions & 11 deletions ...aries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorPrimitives.Max.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ private static T MinMaxCore<T, TMinMaxOperator>(ReadOnlySpan<T> x)
Vector512<T> current;

Vector512<T> nanMask;
if (typeof(T) == typeof(float) || typeof(T) == typeof(double))
if (TMinMaxOperator.PropagatesNaNs && (typeof(T) == typeof(float) || typeof(T) == typeof(double)))
{
// Check for NaNs
nanMask = Vector512.IsNaN(result);
Expand All @@ -166,7 +166,7 @@ private static T MinMaxCore<T, TMinMaxOperator>(ReadOnlySpan<T> x)
// Load the next vector, and early exit on NaN.
current = Vector512.LoadUnsafe(ref xRef, (uint)i);

if (typeof(T) == typeof(float) || typeof(T) == typeof(double))
if (TMinMaxOperator.PropagatesNaNs && (typeof(T) == typeof(float) || typeof(T) == typeof(double)))
{
// Check for NaNs
nanMask = ~Vector512.Equals(current, current);
Expand All @@ -185,7 +185,7 @@ private static T MinMaxCore<T, TMinMaxOperator>(ReadOnlySpan<T> x)
{
current = Vector512.LoadUnsafe(ref xRef, (uint)(x.Length - Vector512<T>.Count));

if (typeof(T) == typeof(float) || typeof(T) == typeof(double))
if (TMinMaxOperator.PropagatesNaNs && (typeof(T) == typeof(float) || typeof(T) == typeof(double)))
{
// Check for NaNs
nanMask = ~Vector512.Equals(current, current);
Expand All @@ -212,7 +212,7 @@ private static T MinMaxCore<T, TMinMaxOperator>(ReadOnlySpan<T> x)
Vector256<T> current;

Vector256<T> nanMask;
if (typeof(T) == typeof(float) || typeof(T) == typeof(double))
if (TMinMaxOperator.PropagatesNaNs && (typeof(T) == typeof(float) || typeof(T) == typeof(double)))
{
// Check for NaNs
nanMask = ~Vector256.Equals(result, result);
Expand All @@ -231,7 +231,7 @@ private static T MinMaxCore<T, TMinMaxOperator>(ReadOnlySpan<T> x)
// Load the next vector, and early exit on NaN.
current = Vector256.LoadUnsafe(ref xRef, (uint)i);

if (typeof(T) == typeof(float) || typeof(T) == typeof(double))
if (TMinMaxOperator.PropagatesNaNs && (typeof(T) == typeof(float) || typeof(T) == typeof(double)))
{
// Check for NaNs
nanMask = ~Vector256.Equals(current, current);
Expand All @@ -251,7 +251,7 @@ private static T MinMaxCore<T, TMinMaxOperator>(ReadOnlySpan<T> x)
current = Vector256.LoadUnsafe(ref xRef, (uint)(x.Length - Vector256<T>.Count));


if (typeof(T) == typeof(float) || typeof(T) == typeof(double))
if (TMinMaxOperator.PropagatesNaNs && (typeof(T) == typeof(float) || typeof(T) == typeof(double)))
{
// Check for NaNs
nanMask = ~Vector256.Equals(current, current);
Expand All @@ -278,7 +278,7 @@ private static T MinMaxCore<T, TMinMaxOperator>(ReadOnlySpan<T> x)
Vector128<T> current;

Vector128<T> nanMask;
if (typeof(T) == typeof(float) || typeof(T) == typeof(double))
if (TMinMaxOperator.PropagatesNaNs && (typeof(T) == typeof(float) || typeof(T) == typeof(double)))
{
// Check for NaNs
nanMask = Vector128.IsNaN(result);
Expand All @@ -297,7 +297,7 @@ private static T MinMaxCore<T, TMinMaxOperator>(ReadOnlySpan<T> x)
// Load the next vector, and early exit on NaN.
current = Vector128.LoadUnsafe(ref xRef, (uint)i);

if (typeof(T) == typeof(float) || typeof(T) == typeof(double))
if (TMinMaxOperator.PropagatesNaNs && (typeof(T) == typeof(float) || typeof(T) == typeof(double)))
{
// Check for NaNs
nanMask = Vector128.IsNaN(current);
Expand All @@ -316,7 +316,7 @@ private static T MinMaxCore<T, TMinMaxOperator>(ReadOnlySpan<T> x)
{
current = Vector128.LoadUnsafe(ref xRef, (uint)(x.Length - Vector128<T>.Count));

if (typeof(T) == typeof(float) || typeof(T) == typeof(double))
if (TMinMaxOperator.PropagatesNaNs && (typeof(T) == typeof(float) || typeof(T) == typeof(double)))
{
// Check for NaNs
nanMask = Vector128.IsNaN(current);
Expand All @@ -335,15 +335,15 @@ private static T MinMaxCore<T, TMinMaxOperator>(ReadOnlySpan<T> x)

// Scalar path used when either vectorization is not supported or the input is too small to vectorize.
T curResult = x[0];
if (T.IsNaN(curResult))
if (TMinMaxOperator.PropagatesNaNs && T.IsNaN(curResult))
{
return curResult;
}

for (int i = 1; i < x.Length; i++)
{
T current = x[i];
if (T.IsNaN(current))
if (TMinMaxOperator.PropagatesNaNs && T.IsNaN(current))
{
return current;
}
Expand Down
2 changes: 2 additions & 0 deletions ...merics.Tensors/src/System/Numerics/Tensors/netcore/TensorPrimitives.MaxMagnitudeNumber.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ public static void MaxMagnitudeNumber<T>(ReadOnlySpan<T> x, T y, Span<T> destina
{
public static bool Vectorizable => true;

public static bool PropagatesNaNs => false;

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static T Invoke(T x, T y) => T.MaxMagnitudeNumber(x, y);

Expand Down
2 changes: 2 additions & 0 deletions ...System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorPrimitives.MaxNumber.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ public static void MaxNumber<T>(ReadOnlySpan<T> x, T y, Span<T> destination)
{
public static bool Vectorizable => true;

public static bool PropagatesNaNs => false;

public static T Invoke(T x, T y) => T.MaxNumber(x, y);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand Down
2 changes: 2 additions & 0 deletions ...merics.Tensors/src/System/Numerics/Tensors/netcore/TensorPrimitives.MinMagnitudeNumber.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ public static void MinMagnitudeNumber<T>(ReadOnlySpan<T> x, T y, Span<T> destina
{
public static bool Vectorizable => true;

public static bool PropagatesNaNs => false;

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static T Invoke(T x, T y) => T.MinMagnitudeNumber(x, y);

Expand Down
2 changes: 2 additions & 0 deletions ...System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorPrimitives.MinNumber.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ public static void MinNumber<T>(ReadOnlySpan<T> x, T y, Span<T> destination)
{
public static bool Vectorizable => true;

public static bool PropagatesNaNs => false;

public static T Invoke(T x, T y) => T.MinNumber(x, y);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand Down
71 changes: 71 additions & 0 deletions src/libraries/System.Numerics.Tensors/tests/TensorPrimitives.Generic.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1772,6 +1772,77 @@ public void StdDev_AllLengths()
});
}
#endregion

#region Number aggregates ignore NaN
[Theory]
[InlineData(1)]
[InlineData(3)]
[InlineData(4)]
[InlineData(5)]
[InlineData(16)]
[InlineData(33)]
public void NumberAggregates_IgnoreNaN(int length)

@lilinus lilinus Sep 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like MinNumber(ROS) and MaxNumber(ROS) is currently missing any coverage (not only for NaN values, but overall). N.b. MinNumber(ROS, ROS) and MaxNumber(ROS, ROS) seems to be covered. Worth adding more coverage while at it?

I think we need to test more values of length if we want to cover all paths (for Vector128, Vector256, Vector512 etc...). Great opportunity to use Assert.All(Helpers.TensorLengths, ...?

Also, looks like Max/Min are already covered by tests Max_Tensor_NanReturned and Min_Tensor_NanReturned

{
// IEEE 754:2019 minimumNumber/maximumNumber ignore a NaN operand when a numeric one is
// available, while minimum/maximum propagate it. The span reductions must match.
T nan = T.CreateTruncating(float.NaN);
T one = T.One;
T two = one + one;

if (length == 1)
{
AssertEqualAggregate(nan, TensorPrimitives.MinNumber<T>([nan]));
AssertEqualAggregate(nan, TensorPrimitives.MaxNumber<T>([nan]));
AssertEqualAggregate(nan, TensorPrimitives.MinMagnitudeNumber<T>([nan]));
AssertEqualAggregate(nan, TensorPrimitives.MaxMagnitudeNumber<T>([nan]));
AssertEqualAggregate(nan, TensorPrimitives.Min<T>([nan]));
AssertEqualAggregate(nan, TensorPrimitives.Max<T>([nan]));
return;
}

T[] values = new T[length];

// NaN at the start, in the middle, and at the end of the span, so both the vectorized
// and the scalar tails of the reduction see it.
foreach (int nanIndex in new[] { 0, length / 2, length - 1 })
{
Array.Fill(values, two);
values[nanIndex] = nan;

// A distinct minimum so the reduction is not trivially the fill value.
values[(nanIndex + 1) % length] = one;

// Number variants ignore the NaN and pick the numeric extreme.
AssertEqualAggregate(one, TensorPrimitives.MinNumber<T>(values));
AssertEqualAggregate(two, TensorPrimitives.MaxNumber<T>(values));
AssertEqualAggregate(one, TensorPrimitives.MinMagnitudeNumber<T>(values));
AssertEqualAggregate(two, TensorPrimitives.MaxMagnitudeNumber<T>(values));

// Plain Min/Max still propagate NaN.
AssertEqualAggregate(nan, TensorPrimitives.Min<T>(values));
AssertEqualAggregate(nan, TensorPrimitives.Max<T>(values));
AssertEqualAggregate(nan, TensorPrimitives.MinMagnitude<T>(values));
AssertEqualAggregate(nan, TensorPrimitives.MaxMagnitude<T>(values));
}

// Signed zeros follow minimumNumber/maximumNumber: +0 is greater than -0.
T[] signedZeros = { -T.Zero, T.Zero };
Assert.True(T.IsNegative(TensorPrimitives.MinNumber<T>(signedZeros)));
Assert.False(T.IsNegative(TensorPrimitives.MaxNumber<T>(signedZeros)));

static void AssertEqualAggregate(T expected, T actual)

@lilinus lilinus Sep 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this helper is not needed, since it seems that Assert.Equal is enough for asserting NaNs in e.g. Max_Tensor_NanReturned

{
if (T.IsNaN(expected))
{
Assert.True(T.IsNaN(actual), $"expected NaN, got {actual}");
}
else
{
Assert.Equal(expected, actual);
}
}
}
#endregion
}

public unsafe abstract class GenericSignedIntegerTensorPrimitivesTests<T> : GenericIntegerTensorPrimitivesTests<T>
Expand Down
Loading