-
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
Tensor wave 1 API's. #101196
Tensor wave 1 API's. #101196
Conversation
Note regarding the
|
Tagging subscribers to this area: @dotnet/area-system-numerics |
a4ce322
to
61faee1
Compare
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.
I've skimmed through some. Will review the rest in a bit.
src/libraries/System.Numerics.Tensors/src/System/NativeIndex.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/NativeIndex.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/NativeIndex.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/NativeIndex.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/NativeIndex.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs
Show resolved
Hide resolved
{ | ||
public static partial class Tensor | ||
{ | ||
public static Tensor<T> Create<T>(bool mustPin, ReadOnlySpan<nint> lengths) |
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.
Missing XML comments on a bunch of these public APIs
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.Factory.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/TensorExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/NativeIndex.cs
Outdated
Show resolved
Hide resolved
dfc0745
to
7872a18
Compare
src/libraries/System.Numerics.Tensors/ref/System.Numerics.Tensors.netcore.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/ref/System.Numerics.Tensors.netcore.cs
Outdated
Show resolved
Hide resolved
public bool MoveNext() { throw null; } | ||
} | ||
} | ||
public static partial class Tensor |
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.
Most of these APIs that both take and return some form of Span
are missing scoped
where relevant.
Many of the span APIs are also unnecessarily mutable, for example Add
should only be taking in ReadOnlyTensorSpan
while AddInPlace
should take one TensorSpan
and one ReadOnlyTensorSpan
.
I'm fine with this class in particular being handled in a follow up PR, but I think it's important to do that cleanup prior to the first preview.
src/libraries/System.Numerics.Tensors/ref/System.Numerics.Tensors.netcore.cs
Outdated
Show resolved
Hide resolved
static System.Numerics.Tensors.Tensor<T> System.Numerics.Tensors.ITensor<System.Numerics.Tensors.Tensor<T>, T>.CreateUninitialized(scoped System.ReadOnlySpan<nint> lengths, scoped System.ReadOnlySpan<nint> strides, bool pinned) { throw null; } | ||
public string ToString(scoped System.ReadOnlySpan<nint> maximumLengths) { throw null; } | ||
public bool TryCopyTo(scoped System.Numerics.Tensors.TensorSpan<T> destination) { throw null; } | ||
public bool TryFlattenTo(scoped System.Span<T> destination) { 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.
Likewise fine with it being cleaned up in a separate PR, but scoped
shouldn't be needed on any API where the underlying ref field cannot be captured.
Tensor<T>
cannot have a ref field
and most of these APIs do not return a ref
. It's different for TensorSpan<T>
since the hidden this
parameter allows the ref to be captured.
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/ReadOnlyTensorSpan.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/ReadOnlyTensorSpan.cs
Outdated
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.
I think there's a lot of cleanup that still needs to be done in the form of optimization work, validating inputs are correct/sensible to the BCL/FDG standards, ensuring we do the relevant security audit, etc
But, this looks like it now matches the so far reviewed API proposal and should be good to merge to keep the PR from growing exponentially. We should ideally log issues to track most of this additional work/cleanup prior to merging
|
* Native Index/Range and ref. * SpanND without slice * index/range testing and implicit conversions * SpanND Tests * tensor working, still need more statics * more tensors updates * ref files updated * final ref update * span updates * all but broadcast and some TensorPrimitives * broadcast in * organizational changes * ref and implicit broadcast * build failures * updates from PR comments * error text moved to strings.resx * exception strings moved to strings.resc * comments from PR * more fixes from PR and API review * rebase on main. XML comments. API updates * NIndex,NRange,RO/TensorSpan API updates * IROTensor,ITensor,Tensor API updates * changes from pr comments * changes from PR comments * fixed test failure
This is the initial PR for the Tensor Prototype. The design doc is here, and its still in discussion so updates will be coming from the result of those discussions. Further updates will also be coming in regards to error handling/etc, but this will allow for preliminary reviews.