Skip to content

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

Open
jabrailkhalil wants to merge 2 commits into
dotnet:mainfrom
jabrailkhalil:fix-tensorprimitives-number-nan-reduction
Open

Fix TensorPrimitives MinNumber/MaxNumber span reductions propagating NaN (#133346)#133628
jabrailkhalil wants to merge 2 commits into
dotnet:mainfrom
jabrailkhalil:fix-tensorprimitives-number-nan-reduction

Conversation

@jabrailkhalil

Copy link
Copy Markdown

Summary

Fixes #133346

TensorPrimitives.MinNumber(ReadOnlySpan<T>) (and MaxNumber, MinMagnitudeNumber, MaxMagnitudeNumber) returned NaN as soon as the reduction encountered a NaN, while the scalar T.MinNumber/T.MaxNumber semantics ignore a NaN operand when a numeric one is available (IEEE 754:2019 minimumNumber/maximumNumber).

The span reductions share MinMaxCore<T, TMinMaxOperator> with the plain Min/Max/Magnitude reductions, and that core early-exits on the first NaN at every vector width and in its scalar tail. The early exit is correct for minimum/maximum (which propagate NaN), but wrong for the *Number family.

Changes

  • IAggregationOperator<T> gains static virtual bool PropagatesNaNs => true.
  • The four *NumberOperator<T> structs override it with false.
  • MinMaxCore gates its NaN early-exits on TMinMaxOperator.PropagatesNaNs (Vector512/256/128 paths plus the scalar tail), so *Number reductions proceed and the lane-wise *Number operator ignores the NaN, while plain Min/Max/Magnitude behavior is unchanged.
  • Regression test NumberAggregates_IgnoreNaN in TensorPrimitives.Generic.cs: NaN at the start/middle/end of spans of several lengths (scalar and vector paths) for all four *Number reductions, the all-NaN case, plus signed-zero ordering and the plain Min/Max NaN propagation control.

Validation

Reproduced on the released .NET 10 SDK (10.0.401): TensorPrimitives.MinNumber<float>([1f, NaN, 2f]) returned NaN, and the same for MaxNumber, MinMagnitudeNumber, MaxMagnitudeNumber.

The fixed sources were compiled with the SDK (all src/System.Numerics.Tensors netcore sources) and verified against a 25-case matrix:

  • Before the fix (same harness against the unmodified sources): MinNumber(float) [1, NaN, 2] FAILED with got NaN, expected 1.
  • After the fix: all 25 cases pass - MinNumber/MaxNumber/MinMagnitudeNumber/MaxMagnitudeNumber ignore NaN (float, double, Half; NaN first/middle/last; vector-sized and scalar inputs), all-NaN inputs still return NaN, -0 vs +0 ordering preserved, and plain Min/Max/MinMagnitude/MaxMagnitude still propagate NaN.

Not run locally: the repo's full library test suite (requires the arcade toolchain setup). The regression test follows existing conventions in TensorPrimitives.Generic.cs (GenericFloatingPointNumberTensorPrimitivesTests<T>) and runs on all four floating-point instantiations in CI.

Note: the .NET Foundation CLA must be signed for this PR to be mergeable (the account jabrailkhalil will need to sign at https://cla.dotnetfoundation.org).

Copilot AI lite review requested due to automatic review settings September 10, 2026 19:25

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Sep 10, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

@jabrailkhalil

Copy link
Copy Markdown
Author

@dotnet-policy-service agree

[InlineData(5)]
[InlineData(16)]
[InlineData(33)]
public void NumberAggregates_IgnoreNaN(int length)

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

Assert.True(T.IsNegative(TensorPrimitives.MinNumber<T>(signedZeros)));
Assert.False(T.IsNegative(TensorPrimitives.MaxNumber<T>(signedZeros)));

static void AssertEqualAggregate(T expected, T actual)

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

Copilot AI review requested due to automatic review settings September 12, 2026 18:02

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@jabrailkhalil

Copy link
Copy Markdown
Author

Thanks @lilinus! I have addressed the feedback in commit 8710e9a by expanding the test coverage for \MinNumber(ROS)\ and \MaxNumber(ROS)\ across tensor lengths and simplifying the NaN assertions using \Assert.Equal\ directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Numerics community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TensorPrimitives.MinNumber(ReadOnlySpan<T>) incorrectly propagates NaN

3 participants