Skip to content

Minor tensor fixes #115125

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

Merged
merged 9 commits into from
May 2, 2025
Merged

Conversation

michaelgsharp
Copy link
Member

There were some minor nit suggestions from #114927, this pr fixes all those suggestions.

@michaelgsharp michaelgsharp self-assigned this Apr 28, 2025
@Copilot Copilot AI review requested due to automatic review settings April 28, 2025 19:14
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.

Copy link
Contributor

@Copilot 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 addresses minor tensor fixes by updating exception naming, improving generic parameter handling in tensor operations, and refining validation logic in tensor transformation methods.

  • Renamed exception methods and updated messages from "InvalidAxis" to "InvalidDimension".
  • Modified generic parameters and overloads in TensorOperation for improved type consistency.
  • Adjusted validation and update logic in Tensor methods (concatenation, reshape, filtered updates, permutation, and stacking).

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/libraries/System.Numerics.Tensors/src/System/ThrowHelper.cs Renamed throw helper methods and updated error messages.
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorOperation.cs Updated generic parameters and added a new overload for binary operations as well as refining filtered update mechanics.
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs Revised concatenation, reshape, update, permutation, and stacking methods with updated validation and delegation improvements.
Files not reviewed (1)
  • src/libraries/System.Numerics.Tensors/src/Resources/Strings.resx: Language not supported

@tannergooding tannergooding requested a review from tarekgh April 28, 2025 19:35
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

Left minor comments/suggestion. LGTM otherwise.

Please ensure fixing the issue raised by the copilot.

{
totalLength += (int)tensors[i].FlattenedLength;
}
}

tensor = Tensor.Create<T>([totalLength]);
Copy link
Member

Choose a reason for hiding this comment

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

Don't think this is correct and it's now calling the Tensor.Create<T>(T[] array) overload, rather than the Tensor.Create<T>(scoped ReadOnlySpan<nint> lengths, bool pinned = false) overload

I think this should stay nint and we can rely on Tensor.Create<T>(scoped ReadOnlySpan<nint> lengths, bool pinned = false) validating that it can be allocated by the underlying tensor storage.

We just need to ensure that adding the combined tensors flattened lengths together doesn't overflow the nint.


I also think that this represents a UX issue with the Create APIs.

I think we may want to disambiguate as CreateFromShape or similar so that a user passing in an nint[] isn't confused on whether its going to create a Tensor<nint> or a Tensor<T> where nint is the lengths.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed back to nint.

But it still was calling the right overload, not the Tensor.Create<T>(T[] array) one since T != int.

@michaelgsharp
Copy link
Member Author

/azp build-analysis

Copy link

Command 'build-analysis' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@michaelgsharp
Copy link
Member Author

/azp run build-analysis

Copy link

No pipelines are associated with this pull request.

@michaelgsharp
Copy link
Member Author

/ba-g unrelated test failure and build-analysis isn't finishing for some reason.

@michaelgsharp michaelgsharp merged commit 0c6ac9c into dotnet:main May 2, 2025
82 of 85 checks passed
@michaelgsharp michaelgsharp deleted the tensors-minorfixes branch May 2, 2025 23:56
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