Skip to content

Fix non-dense TensorSpan handling in SequenceEqual, Fill*, Resize*, and IndexOf* methods#124225

Open
Copilot wants to merge 5 commits intomainfrom
copilot/clarify-sequence-equal-behavior
Open

Fix non-dense TensorSpan handling in SequenceEqual, Fill*, Resize*, and IndexOf* methods#124225
Copilot wants to merge 5 commits intomainfrom
copilot/clarify-sequence-equal-behavior

Conversation

Copy link
Contributor

Copilot AI commented Feb 10, 2026

  • Fix SequenceEqual for non-dense tensor spans
  • Fix FillGaussianNormalDistribution, FillUniformDistribution, Resize, ResizeTo, IndexOfMax, IndexOfMin, IndexOfMaxMagnitude, IndexOfMinMagnitude for non-dense spans
  • Use FlattenedLength instead of _shape.LinearLength in dense fast-paths
  • Add braces to all one-line for/while/if/else bodies in changed code
  • Rewrite tests as comprehensive Theory-based tests with varied shapes/sizes via MemberData
  • All 5400 tests pass
Original prompt

This section details on the original issue you should resolve

<issue_title>Clarify expected behavior of Tensor.SequenceEqual with non-dense spans</issue_title>
<issue_description>Given a non-dense ReadOnlyTensorSpan<T>, SequenceEqual will be false if the unobserved elements are not the same.

For example:

using System;
using System.Numerics.Tensors;

int[] d1 = [10, 20, 99, 99, 30, 40, 99, 99];
int[] d2 = [10, 20, 77, 77, 30, 40, 77, 77];

var ts1 = new ReadOnlyTensorSpan<int>(d1, [2, 2], [4, 1]);
var ts2 = new ReadOnlyTensorSpan<int>(d2, [2, 2], [4, 1]);

DumpTensorSpan(ts1);
DumpTensorSpan(ts2);

bool result = ts1.SequenceEqual(ts2);
Console.WriteLine($"SequenceEqual: {result}");

static void DumpTensorSpan<T>(ReadOnlyTensorSpan<T> span)
{
    Console.WriteLine($"Dense: {span.IsDense}");
    
    foreach (T x in span)
    {
        Console.Write($"{x}, ");
    }
    
    Console.WriteLine();
}

This will print

Dense: False
10, 20, 30, 40, 
Dense: False
10, 20, 30, 40, 
SequenceEqual: False

I am not sure if this is "by design" or not. If it is, the documentation may want to make a note of how non-dense spans behavior.

The unit tests for this appear to be commented out:

//public static void TensorSequenceEqualTests()
</issue_description>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

…or spans

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 10, 2026
Copilot AI changed the title [WIP] Clarify expected behavior of Tensor.SequenceEqual Fix SequenceEqual for non-dense TensorSpan comparisons Feb 10, 2026
Copilot AI requested a review from stephentoub February 10, 2026 15:48
@stephentoub stephentoub marked this pull request as ready for review February 10, 2026 16:20
Copilot AI review requested due to automatic review settings February 10, 2026 16:20
@stephentoub stephentoub added area-System.Numerics.Tensors and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Feb 10, 2026
@dotnet-policy-service
Copy link
Contributor

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

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes Tensor.SequenceEqual / TensorSpan.SequenceEqual so that comparisons involving non-dense tensor spans compare only the logical elements (ignoring unobserved gap/padding in the underlying buffer), while preserving the dense fast-path.

Changes:

  • Updated ReadOnlyTensorSpan<T>.SequenceEqual to use a dense fast-path and a non-dense element-by-element slow-path.
  • Simplified the TensorSpan<T> overload to delegate to the ReadOnlyTensorSpan<T> implementation.
  • Added unit tests covering non-dense vs non-dense, dense vs non-dense (both directions), differing logical elements, and the TensorSpan<T> overload.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs Fixes SequenceEqual semantics for non-dense spans and centralizes logic via the ReadOnlyTensorSpan<T> overload.
src/libraries/System.Numerics.Tensors/tests/TensorTests.cs Adds regression tests validating correct SequenceEqual behavior across dense/non-dense scenarios.

@stephentoub
Copy link
Member

@copilot do other methods have the same or similar problem? e.g. IndexOf*, ResizeTo, Fill*, etc.? Please investigate and fix any other similar problems in the whole library.

…dd tests

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 10, 2026

@copilot do other methods have the same or similar problem? e.g. IndexOf*, ResizeTo, Fill*, etc.? Please investigate and fix any other similar problems in the whole library.

Investigated all methods in the library that use MemoryMarshal.CreateSpan/CreateReadOnlySpan with LinearLength without density checks. Found and fixed the following in commit 99ba364:

  • FillGaussianNormalDistribution / FillUniformDistribution — wrote to raw buffer including gaps; now iterates logical elements via enumerator for non-dense spans
  • Resize (Tensor overload) — read from raw buffer of non-dense sliced tensors; now enumerates logical elements
  • ResizeTo (ReadOnlyTensorSpan overload) — read/wrote raw buffer for both source and destination; now uses element-by-element copy for non-dense spans
  • IndexOfMax / IndexOfMin / IndexOfMaxMagnitude / IndexOfMinMagnitude — searched raw buffer including gaps; now flattens to temp array for non-dense spans

Methods using TensorOperation.Invoke (like Sum, Product, Average, Equals, element-wise ops) are already correct — they iterate via AdjustToNextIndex. Also noted ConcatenateOnDimension has a similar issue with non-dense destinations but that requires a larger rewrite; proposed as follow-up work.

Copilot AI changed the title Fix SequenceEqual for non-dense TensorSpan comparisons Fix non-dense TensorSpan handling in SequenceEqual, Fill*, Resize*, and IndexOf* methods Feb 10, 2026
Copilot AI requested a review from stephentoub February 10, 2026 17:13
@stephentoub stephentoub requested a review from Copilot February 10, 2026 18:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

…theories

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
@stephentoub
Copy link
Member

@copilot invoke the code-review skill and post your analysis/results as a comment on this PR

@stephentoub
Copy link
Member

@copilot invoke the code-review skill and post your analysis/results as a comment on this PR

Copy link
Contributor Author

Copilot AI commented Feb 11, 2026

@copilot invoke the code-review skill and post your analysis/results as a comment on this PR

🤖 Copilot Code Review — PR #123681

Holistic Assessment

Motivation: The PR fixes a real correctness bug. Multiple Tensor methods used MemoryMarshal.CreateSpan(ref _reference, (int)_shape.LinearLength) which includes gap/padding elements for non-dense (strided) tensor spans. This caused SequenceEqual to return false for logically-equal non-dense tensors, Fill* to write to gap positions, IndexOf* to search gap positions, and Resize* to copy gap data. The original tests for SequenceEqual were commented out, confirming this was a known gap.

Approach: The IsDense check with enumerator fallback is the right pattern — it's consistent with how TensorOperation.Invoke already handles non-dense iteration throughout the library. Dense fast paths are preserved. The IndexOf* methods use FlattenTo + temp array, which is correct and simple.

Summary: ⚠️ Needs Human Review. The core code changes are correct, but I have two concerns a human reviewer should evaluate: (1) a behavioral inconsistency in SequenceEqual between the dense and non-dense paths, and (2) whether ConcatenateOnDimension (line 240) should be addressed in this PR or is truly safe to defer.


Detailed Findings

⚠️ SequenceEqual — EqualityComparer<T>.Default vs IEquatable<T>.Equals behavioral difference

In SequenceEqual (line 1787), the non-dense path uses EqualityComparer<T>.Default.Equals(enumerator1.Current, enumerator2.Current), while the dense path delegates to Span<T>.SequenceEqual which calls IEquatable<T>.Equals directly. For T : IEquatable<T>?, these should behave the same for concrete types, but there's a subtle difference: EqualityComparer<T>.Default goes through a virtual dispatch that Span.SequenceEqual avoids. More importantly, if T is null (since the constraint allows IEquatable<T>?), the behavior may differ — EqualityComparer<T>.Default.Equals(null, null) returns true, while Span.SequenceEqual uses IEquatable<T>.Equals which would NRE on null.

This is likely not a practical issue since tensor element types won't be null in practice, but for consistency and to avoid the virtual dispatch overhead, consider using EqualityComparer<T>.Default.Equals in the non-dense path or document why the difference is acceptable. Low confidence concern — a human should evaluate whether this matters.

✅ Correctness — All 10 LinearLength sites fixed

Verified by grep: no remaining MemoryMarshal.Create*Span calls use _shape.LinearLength. All MemoryMarshal span creations in the changed methods now use FlattenedLength inside IsDense guards. The remaining LinearLength usages are in constructor calls (passing linear length as a size parameter to TensorSpan/ReadOnlyTensorSpan constructors), which is correct — those need the full buffer extent.

✅ Correctness — SequenceEqual logic is sound

The precondition check (FlattenedLength match + Lengths.SequenceEqual) correctly handles shape mismatches. The dense fast path via Span.SequenceEqual is correct and unchanged in behavior for dense tensors. The non-dense Enumerator path correctly walks only logical elements. The TensorSpan overload correctly delegates via cast.

✅ Correctness — Fill, Resize, IndexOf methods

  • FillGaussianNormalDistribution / FillUniformDistribution: Correctly iterate only logical elements for non-dense destinations. Gap positions remain untouched.
  • Resize: Non-dense Tensor inputs correctly flattened via enumerator. Output is always a fresh dense array, so no destination density issue.
  • ResizeTo: Both source and destination density checked. Non-dense path copies via dual enumerators.
  • IndexOf*: Non-dense fallback uses FlattenTo + TensorPrimitives.IndexOf*, returning a flattened logical index. Correct.

⚠️ ConcatenateOnDimension (line 240) — still uses totalLength with raw _reference

ConcatenateOnDimension creates MemoryMarshal.CreateSpan(ref destination._reference, (int)totalLength) where totalLength is the sum of source FlattenedLength values. If the destination is non-dense, this span length doesn't account for the destination's stride gaps, so writes via TensorOperation.CopyTo into dstSpan would write to incorrect positions. The PR description notes this as a follow-up, but since the inputs are Tensor<T> (always backed by a contiguous array), the sources are always dense. However, the destination TensorSpan<T> can be non-dense. A human should confirm whether this is exploitable in practice or safe to defer.

✅ Style — Braces added

All one-line for/while/if/else bodies have braces as requested.

💡 Tests — Reasonable coverage, minor duplication

The NonDenseTensorData() provider covers 4 configurations (2×2, 2×3, 3×2, 1D with varying strides/gaps). The tests verify core scenarios: same logical elements with different gaps, non-dense vs dense, differing elements, and the TensorSpan overload. The Fill* tests verify gap positions remain zero. The IndexOf* tests verify correct logical indices.

The offset-computation logic is duplicated across FillGaussianNormalDistributionNonDenseTests and FillUniformDistributionNonDenseTests (~15 lines each). This could be extracted to a helper, but it's a minor concern for test code.

💡 IndexOf* fallbacks — Allocation of T[FlattenedLength]

The IndexOf*Fallback methods allocate new T[x.FlattenedLength] which could be large for big tensors. ArrayPool<T>.Shared would be more appropriate for BCL code. Not blocking since correctness is the priority here and the pattern is simple, but worth noting for follow-up.

@lilinus
Copy link
Contributor

lilinus commented Feb 12, 2026

⚠️ ConcatenateOnDimension (line 240) — still uses totalLength with raw _reference

ConcatenateOnDimension creates MemoryMarshal.CreateSpan(ref destination._reference, (int)totalLength) where totalLength is the sum of source FlattenedLength values. If the destination is non-dense, this span length doesn't account for the destination's stride gaps, so writes via TensorOperation.CopyTo into dstSpan would write to incorrect positions. The PR description notes this as a follow-up, but since the inputs are Tensor<T> (always backed by a contiguous array), the sources are always dense. However, the destination TensorSpan<T> can be non-dense. A human should confirm whether this is exploitable in practice or safe to defer.

This is still an issue. ConcatenateOnDimension will write to "unobserved" elements in ud in demonstration:

var u0 = Tensor.CreateFromShape<int>([2, 3]);
var ud = u0.AsTensorSpan().Slice([0..1, 1..2]);
var u1 = Tensor.Create([1, 2]);
var u2 = Tensor.Create([3, 4]);
Tensor.ConcatenateOnDimension(-1, [u1, u2], ud);
Console.WriteLine($"[{u0[0, 0]} {u0[0, 1]} {u0[0, 2]}]");
Console.WriteLine($"[{u0[1, 0]} {u0[1, 1]} {u0[1, 2]}]");

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clarify expected behavior of Tensor.SequenceEqual with non-dense spans

3 participants