-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tensor.Slice No Longer Copies #113166
base: main
Are you sure you want to change the base?
Tensor.Slice No Longer Copies #113166
Conversation
Tagging subscribers to this area: @dotnet/area-system-numerics-tensors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR updates Tensor slicing behavior so that slices no longer copy the underlying data but instead reference the same backing memory.
- Updated test assertions in TensorTests.cs and TensorSpanTests.cs to reflect that slicing now modifies the original tensor data for the sliced region.
- Modified the Tensor implementation in Tensor.cs to introduce a memoryOffset field and adjust the AsTensorSpan and Slice methods accordingly.
Reviewed Changes
File | Description |
---|---|
src/libraries/System.Numerics.Tensors/tests/TensorTests.cs | Updated assertions and comments to match new non-copy slicing behavior |
src/libraries/System.Numerics.Tensors/tests/TensorSpanTests.cs | Simplified test assertions by removing redundant span creation |
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs | Introduced memoryOffset and updated AsTensorSpan/Slice to avoid data copying |
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
public TensorSpan<T> AsTensorSpan() => new TensorSpan<T>(ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(_values), _memoryOffset), _lengths, _strides, _values.Length - _memoryOffset); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using '_values.Length - _memoryOffset' to determine the flattened length of the slice may not correctly reflect the actual number of elements in the sliced tensor. Consider calculating this value from the sliced tensor's dimensions (e.g., using TensorSpanHelpers.CalculateTotalLength(_lengths)) to ensure it accurately represents the slice's data range.
public TensorSpan<T> AsTensorSpan() => new TensorSpan<T>(ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(_values), _memoryOffset), _lengths, _strides, _values.Length - _memoryOffset); | |
public TensorSpan<T> AsTensorSpan() => new TensorSpan<T>(ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(_values), _memoryOffset), _lengths, _strides, TensorSpanHelpers.CalculateTotalLength(_lengths)); |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
Previously our Tensor.Slice would copy the underlying data. This was undesirable for many reasons. With these changes we no longer perform a copy of the underlying data for a slice.