Uh oh!
There was an error while loading. Please reload this page.
Add Tensor.IndexOfMaxNumber - #129582
Conversation
Tagging subscribers to this area: @dotnet/area-system-numerics-tensors |
jeffhandley
left a comment
There was a problem hiding this comment.
Note
This review was generated by the holistic code review workflow being iterated on as part of #130339. Please treat its findings as assistive input for human review.
Holistic Review
Motivation: The linked API proposal covers a real parity gap in TensorPrimitives: the Number min/max variants existed, but their index-returning counterparts did not. The PR is appropriately scoped to the remaining approved APIs.
Approach: Reusing the existing IndexOfMinMaxCore shape is the right general direction, and the scalar comparisons match the intended NaN-ignoring and signed-zero semantics. However, the vectorized path appears to miss the existing Half-specific treatment used by the sibling *Number aggregation APIs.
Summary: api-approved, applied by @bartonjs, and the ref additions match the remaining approved IndexOf*Number(ReadOnlySpan<T> x) signatures and parameter names. The blocking concern is that Half can enter the vectorized index core, while the new vector comparisons only implement maximumNumber/minimumNumber NaN handling for float/double, which can return -1 or the wrong index when non-NaN Half values follow NaNs.
Detailed Findings
❌ Correctness — Half vectorized paths do not implement Number NaN-ignoring semantics
The new Number operators handle NaN-ignoring semantics in scalar code, but their vector Compare methods special-case only double and float before falling back to plain GreaterThan/LessThan (for example TensorPrimitives.IndexOfMaxNumber.cs lines 59-81, TensorPrimitives.IndexOfMinNumber.cs lines 60-82, and the magnitude variants at lines 71-116). IndexOfMinMaxCore dispatches to vectorized implementations solely from Vector128/256/512<T>.IsSupported (TensorPrimitives.IIndexOfMinMaxOperator.cs lines 24-51), with no Half-specific escape hatch.
That differs from the existing sibling aggregation APIs, which explicitly route Half through a float-based helper for these operations (MaxNumber.cs line 27, MinNumber.cs line 27, MaxMagnitudeNumber.cs line 30, MinMagnitudeNumber.cs line 30). If a Half input reaches the vectorized index path with an initial vector/lane containing NaN and a later non-NaN in the same lane, the fallback GreaterThan/LessThan comparison against NaN is false, so the non-NaN value does not replace the NaN; the final aggregate can remain NaN and the method returns -1 even though a valid number exists. Please either add equivalent Half handling for the index operators or keep Half on the scalar/fallback path, and include a regression case where the first vector is NaN-filled and the selected non-NaN appears later.
✅ API Approval — Approved surface matches the PR additions
New public API surface is present in src/libraries/System.Numerics.Tensors/ref/System.Numerics.Tensors.netcore.cs lines 992-998. I verified linked issue #98862 has the api-approved label; the timeline shows @bartonjs applied it immediately after posting the approved C# shape on 2024-04-11. The PR adds the remaining approved methods with matching names, return type, ReadOnlySpan<T> x parameter name, and where T : INumber<T> constraints; the other APIs from the approved shape were already present from prior PRs.
✅ Tests — Important semantics are covered, but add the Half regression with the fix
The new tests cover empty input, all-NaN input, NaNs being ignored, signed-zero/magnitude tie-breaking, duplicate ties choosing the first matching index, and very large indices (TensorPrimitives.Generic.cs lines 2864-3232). Once the Half vectorized behavior is fixed, please add/ensure a case specifically exercises NaNs in the first vector with the first non-NaN after that vector so this does not regress.
lilinus
commented
Jul 17, 2026
This should be disregarded.
This is alread covered in e.g. |
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "ba0db923c0b0164ae013f4e3787e1740602e9f2c",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "71d81c1705aa1f00e0b57073c97efdd5496f6b9f",
"last_reviewed_commit": "ba0db923c0b0164ae013f4e3787e1740602e9f2c",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "71d81c1705aa1f00e0b57073c97efdd5496f6b9f",
"last_recorded_worker_run_id": "29729479578",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "b5b4ee386b773b8653af85d469005483d9a0b4f1",
"review_id": 4730737620
},
{
"commit": "ba0db923c0b0164ae013f4e3787e1740602e9f2c",
"review_id": 4733540120
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. This adds the IndexOfMaxNumber/IndexOfMinNumber/IndexOfMaxMagnitudeNumber/IndexOfMinMagnitudeNumber overloads that slipped through when the Number (NaN-ignoring) variants of Min/Max were added, closing approved API issue #98862 (labeled api-approved, milestone 11.0.0). The ref surface matches the approved proposal exactly.
Approach: Sound and consistent with the codebase. Rather than duplicating the vectorized index search, the PR parameterizes the existing IIndexOfMinMaxOperator<T> with a new static abstract bool ShouldEarlyExitOnNan so the NaN-propagating operators keep their fast early-exit while the new Number operators skip it and instead fall through to the aggregate, returning -1 only when the whole span is NaN. The per-lane and per-vector Compare implementations mirror the established MaxNumber/MinNumberOperator semantics (NaN-ignoring, signed-zero tiebreak, overflow-aware magnitude handling).
Summary: ✅ LGTM. New public API is approved and matches the ref exactly; the refactor is minimal and reuses the existing search infrastructure; tests cover empty spans, all-NaN spans, NaN-not-returned, signed-zero/sign-magnitude tiebreaks, all tensor lengths, and the large-index (>int.MaxValue byte offset) path across the generic numeric type matrix, including the new IsUnsignedInteger/NFloat guards. One non-blocking nit noted inline about operator-precedence style consistency in dead-code branches.
Detailed Findings
✅ API surface — matches approved proposal
The four new int IndexOf*Number<T>(ReadOnlySpan<T>) where T : INumber<T> methods in ref/System.Numerics.Tensors.netcore.cs correspond exactly to the api-approved proposal in #98862, and all four new source files are registered in the csproj Compile list.
✅ Correctness — NaN and edge-case semantics
The ShouldEarlyExitOnNan => false operators correctly defer the NaN decision to the aggregate: IndexOfMinMaxFallback now returns -1 when the final result is NaN, and each vectorized SizeN tail checks T.IsNaN(aggResult) before computing the index, so an all-NaN span yields -1 while a mixed span ignores NaN lanes. The scalar and vector Compare overloads consistently implement NaN-ignoring comparison with the positive/negative-zero (and sign-magnitude) tiebreak, matching the documented maximumNumber/minimumNumber IEEE semantics and the existing non-index MaxNumber/MinMagnitudeNumber operators.
⚠️ Style — operator-precedence inconsistency (non-blocking, flagged inline)
Two of the migrated conditions (IndexOfMinMaxVectorized128Size2 and ...Size1) omit the parentheses around the typeof(T) == float || typeof(T) == double sub-expression that all other 22 call sites use. It is harmless because those methods only handle 1-/2-byte integer element types, making the float/double test dead, but the inconsistency is a copy/paste hazard. See the inline comment.
✅ Test quality — thorough
Tests are added per-operation and exercise: empty → -1, all-NaN → -1, NaN values never returned, signed-zero and negative-vs-positive magnitude tiebreaks, all Helpers.TensorLengths (covering scalar/128/256/512 paths and remainders), and the IndexOfSizeExceedingMaxValue large-index path. The new IsUnsignedInteger predicate (extended for UInt128/nuint) properly gates the sign-magnitude tests, and IsFloatingPoint is correctly extended to include NFloat. The AllLengths assertions tolerate ties by verifying the returned index's value equals the expected extreme, which is the right contract for an index-of over duplicate extremes.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 150.9 AIC · ⌖ 11.1 AIC · ⊞ 10K
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Holistic Review
Motivation: Justified and unchanged. This adds the IndexOfMaxNumber/IndexOfMinNumber/IndexOfMaxMagnitudeNumber/IndexOfMinMagnitudeNumber NaN-ignoring overloads that close approved API issue #98862 (api-approved, milestone 11.0.0). The ref surface matches the approved proposal exactly.
Approach: Sound and unchanged. The PR parameterizes the existing IIndexOfMinMaxOperator<T> with a new static abstract bool ShouldEarlyExitOnNan so NaN-propagating operators keep the fast early-exit while the new Number operators defer the NaN decision to the aggregate, returning -1 only for an all-NaN span. The new commit ba0db92 ("Add missing parenthesis") adds the parentheses around the typeof(T) == float || typeof(T) == double sub-expression at the three remaining call sites, so all ShouldEarlyExitOnNan && (float || double) conditions are now consistently grouped.
Summary: ✅ LGTM. The sole new commit resolves the operator-precedence style nit that the initial review flagged inline. No behavioral change (the affected branches were dead code for the 1-/2-byte integer element types those methods handle), and the fix removes a copy/paste hazard. No new actionable findings.
Assessment History
- review 4730737620 (commit
b5b4ee3): verdict ✅ LGTM. Current verdict: ✅ LGTM — unchanged. Motivation, approach, and risk are unchanged; the only new patch (ba0db92) simply applies the parenthesization suggested by that review's non-blocking inline nit, so the previously-noted style inconsistency is now resolved.
Detailed Findings
✅ Resolved — operator-precedence consistency
The incremental commit adds the missing parentheses at the three previously-inconsistent sites (IndexOfMinMaxVectorized128Size2, ...Size1, and the additional migrated condition), so every ShouldEarlyExitOnNan && (typeof(T) == typeof(float) || typeof(T) == typeof(double)) guard now reads uniformly. This addresses the non-blocking style finding from the prior review with no semantic change.
No new findings in the incremental scope.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 48.5 AIC · ⌖ 9.86 AIC · ⊞ 10K
Closes#98862