Tensor<T> - Fixed missing memory offset from ReadOnlyTensorSpan#113618
Tensor<T> - Fixed missing memory offset from ReadOnlyTensorSpan#113618michaelgsharp merged 4 commits intodotnet:mainfrom
Conversation
ghost
added
the
|
Tagging subscribers to this area: @dotnet/area-system-numerics-tensors |
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes the missing memory offset issue in ReadOnlyTensorSpan by requiring an explicit memory offset in internal constructors and updating all related factory, slicing, and conversion methods. The changes ensure that the memory offset is consistently propagated throughout tensor creation, slicing, and conversion operations, and new tests have been added for verifying the correctness of summing and slicing operations.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Numerics.Tensors/tests/TensorTests.cs | Added tests for Tensor.Sum and updated tensor creation to include memory offset |
| src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs | Updated constructors, conversion operators, and slicing methods to require and propagate memory offset |
| src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs | Modified factory methods to pass the mandatory memory offset |
| src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorExtensions.cs | Adjusted extension methods to maintain memory offset consistency in tensor broadcasting, reshaping, and other operations |
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorExtensions.cs
Outdated
Show resolved
Hide resolved
tarekgh
left a comment
tarekgh
left a comment
There was a problem hiding this comment.
Added a small suggestions, LGTM otherwise.
carlossanlop
left a comment
carlossanlop
left a comment
There was a problem hiding this comment.
Out of curiosity, how did you catch this? Do you have an open issue that needs to be linked in the PR description, or did you find it by testing manually?
|
@carlossanlop I'm working on some more complex slicing operations and during that work I discovered this issue. I decided this was important enough that I'd split it off into its own PR instead of trying to piggy back it on the slicing PR since thats going to be bigger/more complex. |
Cool. Sounds good. I mainly just wanted to learn the process. |
When creating #113166 I didn't have any testing for the implicit conversions and so missed a few places where the memory offset was not being passed through. This fixes that, and makes the memory offset no longer have a default value on the internal constructor so it has to be manually specified every time to not miss it anymore. It also adds testing to catch this case moving forward.