Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Fast TensorAccessor#1396

Open
haytham2597 wants to merge 25 commits intodotnet:main from
haytham2597:fast_tensor_accesor
Open

Fast TensorAccessor #1396
haytham2597 wants to merge 25 commits intodotnet:main from
haytham2597:fast_tensor_accesor

Conversation

@haytham2597
Copy link

@haytham2597 haytham2597 commented Oct 25, 2024

No description provided.

Copy link
Author

@haytham2597 haytham2597 left a comment

Choose a reason for hiding this comment

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

Fixed some error on my FastTensorAccessor and rollback SLN file to Original of this Repo

Copy link
Member

alinpahontu2912 commented Mar 26, 2025 •
edited
Loading

Hey @haytham2597, are you still intending to work on this PR?

Copy link
Author

Hey @haytham2597, are you still intending to work on this PR?

This PR is already Finish. At least i know is worked and is fast.
alinpahontu2912 reacted with thumbs up emoji

Copy link
Member

Hey @haytham2597, can you add a releasenotes comment, rebase and solve the conflict to get this merged ?

Copy link
Author

@haytham2597 haytham2597 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@alinpahontu2912 Finished implement of Fast TensorAccesor with current update main i think i need wait for approving of @NiklasGustafsson for merge.

Copy link
Member

Hey @haytham2597, Niklas is no longer working on the project. I have taken over. This seems like it was reviewed by him before, so if tests pass, I can merge it.

Copy link
Author

haytham2597 commented Jul 16, 2025 •
edited
Loading

Hey @haytham2597, Niklas is no longer working on the project. I have taken over. This seems like it was reviewed by him before, so if tests pass, I can merge it.

Nice, i think this test passed very well. I don't have any new upgrade/change for this so you can freely merge.
But i believe that need run test for the current branch.

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

This PR introduces a "fast" path for TensorAccessor<T> array extraction/copying, adds tests intended to validate the new behavior, and updates release notes / test tolerances accordingly.

Changes:

  • Reworked TensorAccessor<T>.ToArray(), ToNDArray(), CopyTo(), and CopyFrom() with new pointer-based implementations and a new ToArray(from_index, count) overload.
  • Added new unit tests covering TensorAccessor<T>.ToArray() and copy methods on contiguous/non-contiguous tensors.
  • Relaxed a JIT numerical comparison tolerance and documented the TensorAccessor change in release notes.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 14 comments.

Show a summary per file
File Description
src/TorchSharp/Utils/TensorAccessor.cs Adds new "fast" implementations for array extraction and copy operations.
test/TorchSharpTest/TestTorchTensor.cs Adds tests targeting the new fast TensorAccessor behaviors.
test/TorchSharpTest/TestJIT.cs Adjusts an allclose assertion tolerance.
RELEASENOTES.md Notes the "Fast TensorAccessor" change.
.gitignore Adds ignore rules for TestClear.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +236 to +240
if (_tensor.is_contiguous()) {
ToArray().CopyTo(array);
return;
}
ToArray().CopyTo(array);
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

CopyTo(Span<T> array, int arrayIndex = 0, long tensorIndex = 0) ignores both arrayIndex and tensorIndex (it calls ToArray().CopyTo(array)), and the contiguous fast path still allocates a full intermediate array. This should copy directly from the tensor starting at tensorIndex into array.Slice(arrayIndex, ...) without allocating.

Suggested change
if (_tensor.is_contiguous()) {
ToArray().CopyTo(array);
return;
}
ToArray().CopyTo(array);
if (arrayIndex < 0 || arrayIndex > array.Length)
throw new ArgumentOutOfRangeException(nameof(arrayIndex));
long count = Count;
if (tensorIndex < 0 || tensorIndex > count)
throw new ArgumentOutOfRangeException(nameof(tensorIndex));
if (_tensor.is_contiguous()) {
long availableFromTensor = count - tensorIndex;
int availableInArray = array.Length - arrayIndex;
int maxCount = Math.Min((int)availableFromTensor, availableInArray);
if (maxCount <= 0)
return;
unsafe {
// Source pointer: start at tensorIndex
T* srcPtr = (T*)_tensor_data_ptr;
srcPtr += tensorIndex;
// Destination: slice the span to the desired region
var destSlice = array.Slice(arrayIndex, maxCount);
fixed (T* destPtr = &MemoryMarshal.GetReference(destSlice)) {
long byteCount = (long)maxCount * sizeof(T);
Buffer.MemoryCopy(srcPtr, destPtr, byteCount, byteCount);
}
}
return;
}
// Non-contiguous tensor: copy element-by-element without allocating an intermediate array.
if (tensorIndex >= count || arrayIndex >= array.Length)
return;
int written = 0;
long skipped = 0;
foreach (var value in this) {
if (skipped < tensorIndex) {
skipped++;
continue;
}
int targetIndex = arrayIndex + written;
if (targetIndex >= array.Length)
break;
array[targetIndex] = value;
written++;
if (written >= array.Length - arrayIndex)
break;
}

Copilot uses AI. Check for mistakes.
Comment on lines 243 to 246
public void CopyFrom(T[] array, int arrayIndex = 0, long tensorIndex = 0)
{
int idx = arrayIndex;
foreach (int offset in GetSubsequentIndices(tensorIndex)) {
if (idx >= array.Length) break;
unsafe { ((T*)_tensor_data_ptr)[offset] = array[idx]; }
idx += 1;
}
SetValueTensor(ref array, _tensor.shape, _tensor.stride(), Count, arrayIndex, onThis:true);
}
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

CopyFrom(T[] array, int arrayIndex = 0, long tensorIndex = 0) passes arrayIndex into SetValueTensor() as if it were the starting tensor index, and ignores tensorIndex. This changes semantics vs. the old implementation (array offset != tensor offset) and can write into the wrong tensor positions.

Copilot uses AI. Check for mistakes.
Comment on lines +257 to +266
for (long index = arrayIndex; index < count; index++) {
long offset = index;
long ptrIndex = 0;
for (long d = shape.Length - 1; d >= 0; d--) // Traverse dimensions in reverse order
{
long i = offset % shape[d]; // Current index in dimension d
ptrIndex += i * strides[d]; // Calculate ptrIndex using strides
offset /= shape[d]; // Move to the next dimension
}
ptr[ptrIndex] = array[(int)index];
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

CopyFrom(ReadOnlySpan<T> array, int arrayIndex = 0, long tensorIndex = 0) uses the loop variable index as both the tensor linear index and the source span index (array[(int)index]), which will go out of range whenever Count > array.Length and also ignores tensorIndex. It should map source index src = index - tensorIndex + arrayIndex and limit the loop to the min of source/destination remaining lengths.

Suggested change
for (long index = arrayIndex; index < count; index++) {
long offset = index;
long ptrIndex = 0;
for (long d = shape.Length - 1; d >= 0; d--) // Traverse dimensions in reverse order
{
long i = offset % shape[d]; // Current index in dimension d
ptrIndex += i * strides[d]; // Calculate ptrIndex using strides
offset /= shape[d]; // Move to the next dimension
}
ptr[ptrIndex] = array[(int)index];
if (arrayIndex < 0 || arrayIndex > array.Length)
throw new ArgumentOutOfRangeException(nameof(arrayIndex));
if (tensorIndex < 0 || tensorIndex > count)
throw new ArgumentOutOfRangeException(nameof(tensorIndex));
long maxSrc = array.Length - arrayIndex;
long maxDst = count - tensorIndex;
long elementsToCopy = Math.Min(maxSrc, maxDst);
for (long i = 0; i < elementsToCopy; i++) {
long linearIndex = tensorIndex + i;
long offset = linearIndex;
long ptrIndex = 0;
for (long d = shape.Length - 1; d >= 0; d--) // Traverse dimensions in reverse order
{
long dimIndex = offset % shape[d]; // Current index in dimension d
ptrIndex += dimIndex * strides[d]; // Calculate ptrIndex using strides
offset /= shape[d]; // Move to the next dimension
}
int srcIndex = arrayIndex + (int)i;
ptr[ptrIndex] = array[srcIndex];

Copilot uses AI. Check for mistakes.
// Copyright (c) .NET Foundation and Contributors. All Rights Reserved. See LICENSE in the project root for license information.
using System;
using System.Collections.Generic;
using System.Diagnostics;
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

using System.Diagnostics; is added but not used anywhere in this test file; it should be removed to avoid unused-using warnings (and to keep the file consistent with the rest of the using list).

Suggested change
using System.Diagnostics;

Copilot uses AI. Check for mistakes.
Comment on lines +8244 to +8249
float[] v = t.data<float>().ToArray(4);
float[] res = new float[v.Length];
res[0] = t[1, 0, 1].item<float>();
res[1] = t[1, 0, 2].item<float>();
Assert.Equal(res, v);
}
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

The new ToArrayIndexFastTensorAccessor test doesn't assert the expected length of v and leaves trailing entries of res at default values. As written, it can still pass even if ToArray(4) returns extra default elements beyond the two expected values; add an explicit length assertion and/or fully populate res to validate the contract.

Copilot uses AI. Check for mistakes.
Comment on lines 225 to 232
public void CopyTo(T[] array, int arrayIndex = 0, long tensorIndex = 0)
{
int idx = arrayIndex;
foreach (int offset in GetSubsequentIndices(tensorIndex)) {
if (idx >= array.Length) break;
unsafe { array[idx] = ((T*)_tensor_data_ptr)[offset]; }
idx += 1;
if (_tensor.is_contiguous()) {
CopyContiguous(array, arrayIndex, array.Length);
return;
}
ToArray().CopyTo(array, arrayIndex);
}
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

CopyTo(T[] array, int arrayIndex = 0, long tensorIndex = 0) no longer honors tensorIndex (it always copies from the start of the tensor). This is a behavioral change from the previous implementation and will produce wrong results for callers that request copying from a non-zero tensor offset.

Copilot uses AI. Check for mistakes.
float[] toCopy = new float[] { 1, 2 };
var t = rand(2, 1, 3, ScalarType.Float32);
t.data<float>().CopyFrom(toCopy);
Assert.True(t[0,0,0].item<float>() == toCopy[0]);
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

Equality checks on floating point values can yield unexpected results.

Copilot uses AI. Check for mistakes.
Comment on lines +8267 to +8276
Assert.True(t[0,0,0].item<float>() == toCopy[0]);
Assert.True(t[0,0,1].item<float>() == toCopy[1]);
}
{
//With offset
float[] toCopy = new float[] { 9, 3 };
var t = rand(2, 1, 3, ScalarType.Float32);
t.data<float>().CopyFrom(toCopy, 2);
Assert.True(t[0, 0, 2].item<float>() == toCopy[0]);
Assert.True(t[1, 0, 0].item<float>() == toCopy[1]);
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

Equality checks on floating point values can yield unexpected results.

Suggested change
Assert.True(t[0,0,0].item<float>()==toCopy[0]);
Assert.True(t[0,0,1].item<float>()==toCopy[1]);
}
{
//With offset
float[] toCopy = new float[] { 9, 3 };
var t = rand(2, 1, 3, ScalarType.Float32);
t.data<float>().CopyFrom(toCopy, 2);
Assert.True(t[0, 0, 2].item<float>()==toCopy[0]);
Assert.True(t[1, 0, 0].item<float>()==toCopy[1]);
Assert.Equal(toCopy[0],t[0,0,0].item<float>());
Assert.Equal(toCopy[1],t[0,0,1].item<float>());
}
{
//With offset
float[] toCopy = new float[] { 9, 3 };
var t = rand(2, 1, 3, ScalarType.Float32);
t.data<float>().CopyFrom(toCopy, 2);
Assert.Equal(toCopy[0],t[0, 0, 2].item<float>());
Assert.Equal(toCopy[1],t[1, 0, 0].item<float>());

Copilot uses AI. Check for mistakes.
float[] toCopy = new float[] { 9, 3 };
var t = rand(2, 1, 3, ScalarType.Float32);
t.data<float>().CopyFrom(toCopy, 2);
Assert.True(t[0, 0, 2].item<float>() == toCopy[0]);
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

Equality checks on floating point values can yield unexpected results.

Copilot uses AI. Check for mistakes.
Comment on lines +8267 to +8276
Assert.True(t[0,0,0].item<float>() == toCopy[0]);
Assert.True(t[0,0,1].item<float>() == toCopy[1]);
}
{
//With offset
float[] toCopy = new float[] { 9, 3 };
var t = rand(2, 1, 3, ScalarType.Float32);
t.data<float>().CopyFrom(toCopy, 2);
Assert.True(t[0, 0, 2].item<float>() == toCopy[0]);
Assert.True(t[1, 0, 0].item<float>() == toCopy[1]);
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

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

Equality checks on floating point values can yield unexpected results.

Suggested change
Assert.True(t[0,0,0].item<float>()==toCopy[0]);
Assert.True(t[0,0,1].item<float>()==toCopy[1]);
}
{
//With offset
float[] toCopy = new float[] { 9, 3 };
var t = rand(2, 1, 3, ScalarType.Float32);
t.data<float>().CopyFrom(toCopy, 2);
Assert.True(t[0, 0, 2].item<float>()==toCopy[0]);
Assert.True(t[1, 0, 0].item<float>()==toCopy[1]);
Assert.Equal(toCopy[0],t[0,0,0].item<float>(),5);
Assert.Equal(toCopy[1],t[0,0,1].item<float>(),5);
}
{
//With offset
float[] toCopy = new float[] { 9, 3 };
var t = rand(2, 1, 3, ScalarType.Float32);
t.data<float>().CopyFrom(toCopy, 2);
Assert.Equal(toCopy[0],t[0, 0, 2].item<float>(),5);
Assert.Equal(toCopy[1],t[1, 0, 0].item<float>(),5);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@alinpahontu2912 alinpahontu2912 Awaiting requested review from alinpahontu2912

+1 more reviewer

@NiklasGustafsson NiklasGustafsson NiklasGustafsson left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Comments

AltStyle によって変換されたページ (->オリジナル) /