-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[release/9.0] Tensor Operation fixes when input is sliced (non-contiguous) #107097
Conversation
Note regarding the
|
1 similar comment
Note regarding the
|
Tagging subscribers to this area: @dotnet/area-system-numerics-tensors |
@@ -299,7 +299,7 @@ public static void BroadcastTo<T>(this System.Numerics.Tensors.Tensor<T> source, | |||
public static bool GreaterThanAny<T>(in System.Numerics.Tensors.ReadOnlyTensorSpan<T> x, T y) where T : System.Numerics.IComparisonOperators<T, T, bool> { throw null; } | |||
public static bool GreaterThanAny<T>(T x, in System.Numerics.Tensors.ReadOnlyTensorSpan<T> y) where T : System.Numerics.IComparisonOperators<T, T, bool> { throw null; } | |||
public static bool GreaterThanOrEqualAll<T>(in System.Numerics.Tensors.ReadOnlyTensorSpan<T> x, in System.Numerics.Tensors.ReadOnlyTensorSpan<T> y) where T : System.Numerics.IComparisonOperators<T, T, bool> { throw null; } | |||
public static bool GreaterThanOrEqualAll<T>(in System.Numerics.Tensors.ReadOnlyTensorSpan<T> s, T y) where T : System.Numerics.IComparisonOperators<T, T, bool> { throw null; } | |||
public static bool GreaterThanOrEqualAll<T>(in System.Numerics.Tensors.ReadOnlyTensorSpan<T> x, T y) where T : System.Numerics.IComparisonOperators<T, T, bool> { throw null; } |
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.
These should be OK - I see this is for consistency with other API. This is non-binary breaking. It's source breaking, but only in the edge case where someone was using named parameters - which I find highly unlikely here.
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorHelpers.cs
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/Properties/InternalVisibleTo.cs
Show resolved
Hide resolved
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.
LGTM, please remove use of IVT in a follow up PR.
@ericstj - so, are we done-done? |
There was one build job that timed out - which has been rerun. We need to wait for that to ensure it passes before merging. If you'd like to give it your signoff and label that will make it ready to merge and one of us can merge once the tests complete. As for all the tensor changes - no we are not done-done. We'll still have a few more over the next week. |
Cool - I was just talking about this PR. Thanks! |
Backport of #106985 to release/9.0
/cc @michaelgsharp
Customer Impact
This fixes #106280, a bug when using slicing for certain operations. The exact issue would depend on which operation was but, but for some it could cause extra data to be processed resulting in a different final result (due to some being vectorized and some not) and other would just cause a fully incorrect result if all the data was used in the final result of the operation.
Regression
Testing
[How was the fix verified? How was the issue missed previously? What tests were added?]
Bug was missed due to slicing not being tested for all operations. Fix was verified by adding in more automatic testing specifically around slicing for all operations.
Risk
Low risk.
Tensor<T>
is going to be a preview release and was added in this 9.0 release.[High/Medium/Low. Justify the indication by mentioning how risks were measured and addressed.]
IMPORTANT: If this backport is for a servicing release, please verify that:
The PR target branch is
release/X.0-staging
, notrelease/X.0
.If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.