Fixing API to use in and ReadOnly as needed#103368
Fixing API to use in and ReadOnly as needed#103368michaelgsharp merged 5 commits intodotnet:mainfrom
Conversation
|
Note regarding the |
1 similar comment
|
Note regarding the |
|
Tagging subscribers to this area: @dotnet/area-system-numerics-tensors |
| destination = Tensor.Create(intermediate.ToArray(), intermediate.Lengths); | ||
| return true; | ||
| } | ||
| catch |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
Can we avoid intermediate allocations like this? This could stack alloc for some reasonable number of lengths.
| @@ -504,177 +506,179 @@ public static void Xor<T>(System.ReadOnlySpan<T> x, T y, System.Span<T> destinat | |||
| } | |||
| public static partial class TensorSpan | |||
There was a problem hiding this comment.
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.
Fixes the TensorExtensions to use
infor theTensorSpanandReadOnlyTensorSpan. Also changesTensorSpantoReadOnlyTensorSpanwhere appropriate.