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

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

Conversation

@jabrailkhalil

@jabrailkhalil jabrailkhalil commented Sep 10, 2026 •

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 tests NumberAggregates_AllLengths and NumberAggregates_IgnoreNaN_AllLengths in TensorPrimitives.Generic.cs: all four *Number reductions run across every configured Helpers.TensorLengths value; the NaN suite covers all-NaN spans plus NaN at the start/middle/end, and retains signed-zero checks.

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.

The full upstream runtime matrix ran on the current head. Its remaining red statuses are unrelated to this change: one macOS job exceeded the 120-minute limit and Helix reported three System.Net.Sockets.Tests work-item failures; the other displayed runtime matrix jobs passed. The regression tests follow existing conventions in TensorPrimitives.Generic.cs and run on all four floating-point instantiations.

The .NET Foundation CLA check passes for jabrailkhalil.

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

Comment thread src/libraries/System.Numerics.Tensors/tests/TensorPrimitives.Generic.cs Outdated
Comment thread src/libraries/System.Numerics.Tensors/tests/TensorPrimitives.Generic.cs Outdated
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.

@jabrailkhalil

Copy link
Copy Markdown
Author

The remaining failures on current head 8710e9a appear unrelated to this TensorPrimitives change:

  • Azure build 1594281 canceled osx-arm64 Debug Libraries_CheckedCoreCLR after it exceeded the 120-minute job limit.
  • The Helix monitor completed 3,954 work items and reports its three failed work items only under System.Net.Sockets.Tests (Windows x86 CoreCLR, Linux x64 Mono MiniJIT, and Windows x64 CoreCLR).
  • The other runtime matrix jobs shown on the PR, including the Linux ARM64 checked-libraries lane, passed.

Could a maintainer please rerun the failed runtime pipeline? The current head also incorporates the inline review feedback by covering all configured tensor lengths and all four Number reductions.

This branch has not been deployed

No deployments
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