Skip to content

Fix duplicate _start offset in Tensor.Resize#128511

Open
prozolic wants to merge 4 commits into
dotnet:mainfrom
prozolic:tensorresize
Open

Fix duplicate _start offset in Tensor.Resize#128511
prozolic wants to merge 4 commits into
dotnet:mainfrom
prozolic:tensorresize

Conversation

@prozolic

@prozolic prozolic commented May 23, 2026

Copy link
Copy Markdown
Contributor

Closes #128512

This PR fix the offset used when creating a ReadOnlySpan<T> that starts at _start in Tensor.Resize. AsTensorSpan()._reference already includes the _start offset, so applying it again via Unsafe.Add caused the span to start at the wrong position.

Changes:

  • Remove Unsafe.Add when creating a ReadOnlySpan<T>
  • Change from tensor.AsTensorSpan to Tensor.AsReadOnlyTensorSpan to match Tensor.ResizeTo
  • Use (int)tensor.FlattenedLength instead of tensor._values.Length - tensor._start

Fix the offset used when creating a ReadOnlySpan<T> that starts
at _start in Tensor.Resize. AsTensorSpan()._reference already
includes the _start offset, so applying it again via Unsafe.Add
caused the span to start at the wrong position.
Copilot AI review requested due to automatic review settings May 23, 2026 00:32

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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes a bug in Tensor.Resize where the source span incorrectly included data before the tensor's _start offset, causing wrong values to be copied when resizing a tensor created with a non-zero start.

Changes:

  • Replaced manual _start-offset span creation with AsReadOnlyTensorSpan()._reference and used FlattenedLength as the span length.
  • Added a new test TensorResizeWithStartTests covering resize cases for a tensor created with a non-zero start.

Reviewed changes

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

File Description
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs Corrects source span reference and length for dense tensor resize.
src/libraries/System.Numerics.Tensors/tests/TensorTests.cs Adds test coverage for Resize on tensors with a non-zero start.

@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label May 23, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@prozolic
prozolic marked this pull request as ready for review May 23, 2026 00:35
Copilot AI review requested due to automatic review settings May 23, 2026 00:35

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.

Pull request overview

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

@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "184adf3f510d67e80eb95caa663d0fcad9412c8f",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "f43516f45fff5920eabd11753bd5f1803cfacd0c",
  "last_reviewed_commit": "184adf3f510d67e80eb95caa663d0fcad9412c8f",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "f43516f45fff5920eabd11753bd5f1803cfacd0c",
  "last_recorded_worker_run_id": "29707108762",
  "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": "5e6b704939438a033b81ce96f075b4694a635114",
      "review_id": 4730521324
    },
    {
      "commit": "184adf3f510d67e80eb95caa663d0fcad9412c8f",
      "review_id": 4731654495
    }
  ]
}

@github-actions github-actions Bot 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.

Holistic Review

Motivation: This PR fixes a real correctness bug (#128512) in Tensor.Resize for the dense code path. When the input tensor has a non-zero _start (i.e. it wraps a backing array with an offset, such as a slice), the source span was constructed with the _start offset applied twice, so Resize read from the wrong position and produced incorrect data.

Approach: The change replaces MemoryMarshal.CreateSpan(ref Unsafe.Add(ref tensor.AsTensorSpan()._reference, tensor._start), tensor._values.Length - tensor._start) with MemoryMarshal.CreateSpan(ref tensor.AsReadOnlyTensorSpan()._reference, (int)tensor.FlattenedLength). This is correct: AsReadOnlyTensorSpan()/AsTensorSpan() already offset _reference by _start (Tensor_1.cs lines 137/149 use Unsafe.Add(ref GetArrayDataReference(_values), _start)), so re-adding tensor._start was the double-offset bug. Switching the length to FlattenedLength is also an improvement over _values.Length - _start: it bounds the span to the logical element count rather than the remaining raw array capacity, which avoids reading past the logical data of an offset/sliced tensor, and it aligns with the sibling ResizeTo overloads that already use FlattenedLength. Moving to AsReadOnlyTensorSpan for a read-only source is consistent with ResizeTo.

Summary: This is a focused, correct bug fix with no behavioral risk to the previously-working _start == 0 case (both offset and length reduce to the prior values when _start is 0). The new TensorResizeWithStartTests test directly exercises the offset scenario across several target shapes (shrink, grow with zero-fill, and reshape), giving good regression coverage for the fixed path. The IsDense gate ensures the linear copy remains valid. I found no additional issues. LGTM.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 47 AIC · ⌖ 10.3 AIC · ⊞ 10K

Copilot AI review requested due to automatic review settings July 19, 2026 22:47

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.

Pull request overview

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

@github-actions github-actions Bot 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.

Holistic Review

Motivation: This PR fixes a correctness bug (#128512) in Tensor.Resize on the dense path. When the input tensor has a non-zero _start (e.g. a slice over an offset backing array), the source span was constructed with the _start offset applied twice, so Resize read from the wrong position and produced incorrect data.

Approach: It replaces MemoryMarshal.CreateSpan(ref Unsafe.Add(ref tensor.AsTensorSpan()._reference, tensor._start), tensor._values.Length - tensor._start) with MemoryMarshal.CreateSpan(ref tensor.AsReadOnlyTensorSpan()._reference, (int)tensor.FlattenedLength). AsReadOnlyTensorSpan()/AsTensorSpan() already offset _reference by _start, so re-adding tensor._start was the double-offset bug. Using FlattenedLength bounds the span to the logical element count instead of remaining raw array capacity and matches the sibling ResizeTo overloads. Both offset and length reduce to the previous values when _start == 0, so the previously-working case is unchanged.

Summary: The PR patch has not changed since the prior review — the previous and current cumulative patch IDs are identical, so this re-review adds no new actionable findings. The cumulative assessment is unchanged: this remains a focused, correct bug fix with good regression coverage from TensorResizeWithStartTests. LGTM.

Assessment History:

  • review 4730521324 reviewed commit 5e6b704 with verdict LGTM. Current verdict is LGTM; assessment unchanged (motivation, approach, and risk are all identical, and the PR patch is unchanged).

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 29.7 AIC · ⌖ 9.63 AIC · ⊞ 10K

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

Labels

area-System.Numerics.Tensors 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.

Tensor.Resize returns unexpected values when start is greater than 0

2 participants