Skip to content
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

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 28, 2024

Backport of #106985 to release/9.0

/cc @michaelgsharp

Customer Impact

  • Customer reported
  • Found internally

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

  • Yes
  • No

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, not release/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.

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link
Contributor

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

@michaelgsharp
Copy link
Member

@ericstj @artl93

@@ -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; }
Copy link
Member

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.

Copy link
Member

@ericstj ericstj left a 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.

@artl93
Copy link
Contributor

artl93 commented Aug 29, 2024

@ericstj - so, are we done-done?

@ericstj
Copy link
Member

ericstj commented Aug 29, 2024

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.

@artl93
Copy link
Contributor

artl93 commented Aug 29, 2024

Cool - I was just talking about this PR. Thanks!

@artl93 artl93 added the Servicing-approved Approved for servicing release label Aug 29, 2024
@ericstj ericstj merged commit 00f3f3e into release/9.0 Aug 29, 2024
85 of 89 checks passed
@ViktorHofer ViktorHofer deleted the backport/pr-106985-to-release/9.0 branch September 2, 2024 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants