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

Fixing API to use in and ReadOnly as needed #103368

Merged
merged 5 commits into from
Jun 19, 2024

Conversation

michaelgsharp
Copy link
Member

Fixes the TensorExtensions to use in for the TensorSpan and ReadOnlyTensorSpan. Also changes TensorSpan to ReadOnlyTensorSpan where appropriate.

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.

destination = Tensor.Create(intermediate.ToArray(), intermediate.Lengths);
return true;
}
catch
Copy link
Member

Choose a reason for hiding this comment

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

There's no way to implement this without using exceptions for control flow? This largely defeats the intended purpose of the Try pattern, which is to avoid the expense of exceptions for the condition represented by the Try.

public static TensorSpan<T> Reshape<T>(this in TensorSpan<T> input, params ReadOnlySpan<nint> lengths)
where T : IEquatable<T>, IEqualityOperators<T, T, bool>
{
nint[] arrLengths = lengths.ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid intermediate allocations like this? This could stack alloc for some reasonable number of lengths.

@@ -504,177 +506,179 @@ public static partial class TensorPrimitives
}
public static partial class TensorSpan
Copy link
Member

@tannergooding tannergooding Jun 13, 2024

Choose a reason for hiding this comment

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

I think we should talk about the Tensor vs TensorSpan class split and how it impacts discoverability and usability of the APIs -- CC. @stephentoub

Right now we basically have (for example):

static class Tensor
{
    public static Tensor<T> Abs<T>(Tensor<T> input);
    public static Tensor<T> AbsInPlace<T>(Tensor<T> input);
}

static class TensorSpan
{
    public static TensorSpan<T> Abs<T>(in ReadOnlyTensorSpan<T> input);
    public static TensorSpan<T> AbsInPlace<T>(in TensorSpan<T> input);
}

The former APIs typically refer to the latter APIs (Tensor<T> defers to TensorSpan<T>) but the latter APIs sometimes need to allocate a new buffer and are returning that as a span, which is somewhat problematic. It also lacks the ability for users to support their own buffers.

I think rather we should set this up as:

static class Tensor
{
    public static Tensor<T> Abs<T>(in ReadOnlyTensorSpan<T> input);
    public static void Abs<T>(in ReadOnlyTensorSpan<T> input, in TensorSpan<T> destination);
    public static void AbsInPlace<T>(in TensorSpan<T> input);
}

This minimizes the total number of APIs we need to expose, centralizes the APIs we expose so users only need to look in "one place", ensures that users can take ownership of the allocated buffer in case we need to allocate one, and that they can provide their own buffer in case they want to customize how that is allocated.

It's worth noting that the latter two APIs are returning void just like TensorPrimitives returns void because that's the default expectation for most similar APIs. Returning the destination buffer (which is input in the case of AbsInPlace) is a matter of convenience and allows for easier chaining and more fluent like API usage. -- I don't have a particular preference here and personally lean towards the more fluent like approach, but I expect it will entail discussion in API review either way.

@michaelgsharp michaelgsharp merged commit 88013cb into dotnet:main Jun 19, 2024
81 of 83 checks passed
@michaelgsharp michaelgsharp deleted the tensor-api-updates branch June 19, 2024 15:16
@github-actions github-actions bot locked and limited conversation to collaborators Jul 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants