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.

Sign up for GitHub

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

Jump to bottom

6 more naive methods for Tensor Primitives. #92142

Merged
merged 2 commits into from
Sep 16, 2023
Merged

6 more naive methods for Tensor Primitives. #92142

merged 2 commits into from
Sep 16, 2023

Conversation

michaelgsharp
Copy link
Member

Added in naive implementations for

  • CosineSimilarty
  • Distance
  • Dot
  • Normalize
  • SoftMax
  • Sigmoid

@tannergooding @stephentoub @jeffhandley should we rename normalize as discussed since its not really the operation we are doing?

@dotnet-issue-labeler
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.

@ghost ghost assigned michaelgsharp Sep 15, 2023
@ghost
Copy link

ghost commented Sep 15, 2023

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

Issue Details

Added in naive implementations for

  • CosineSimilarty
  • Distance
  • Dot
  • Normalize
  • SoftMax
  • Sigmoid

@tannergooding @stephentoub @jeffhandley should we rename normalize as discussed since its not really the operation we are doing?

Author: michaelgsharp
Assignees: michaelgsharp
Labels:

area-System.Numerics.Tensors, new-api-needs-documentation
Milestone: -

@stephentoub stephentoub mentioned this pull request Sep 15, 2023
Merged
ericstj
ericstj approved these changes Sep 16, 2023
View reviewed changes
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

stephentoub
stephentoub approved these changes Sep 16, 2023
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

I'm going to merge this and then fix up a few things after rebasing my PR on it.

@stephentoub stephentoub merged commit 7be3913 into dotnet:main Sep 16, 2023
101 of 105 checks passed
jeffhandley
jeffhandley reviewed Sep 16, 2023
View reviewed changes
Copy link
Member

@jeffhandley jeffhandley left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Echoing the existing feedback about tests, we could add a lot more tests for all these methods to capture edge/corner cases and more exceptional data too.

I meant to say, but we can add more tests after the RC2 milestone/snap

src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/TensorPrimitives.cs Show resolved Hide resolved
tannergooding
tannergooding reviewed Sep 18, 2023
View reviewed changes
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/TensorPrimitives.cs
{
ThrowHelper.ThrowArgument_SpansMustHaveSameLength();
}
if (x.Length == 0 || y.Length == 0)
Copy link
Member

Choose a reason for hiding this comment

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

We only need one of the checks here since we've already determined the lengths are equal.

That being said, is it desirable to throw here rather than allowing it to just return NaN?

tannergooding
tannergooding reviewed Sep 18, 2023
View reviewed changes
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/TensorPrimitives.cs
/// </summary>
/// <param name="x">The first tensor, represented as a span.</param>
/// <returns>The L2 norm.</returns>
public static float L2Normalize(ReadOnlySpan<float> x) // BLAS1: nrm2
Copy link
Member

@tannergooding tannergooding Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why is this L2Normalize? The standard API computes the "euclidean norm", that is the length or magnitude of the vector.

The name should likely just be Norm (or Length which is what it's called for Vector2/3/4.Length())

tannergooding
tannergooding reviewed Sep 18, 2023
View reviewed changes
src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/TensorPrimitives.cs

for (int i = 0; i < x.Length; i++)
{
expSum += MathF.Pow((float)Math.E, x[i]);
Copy link
Member

Choose a reason for hiding this comment

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

Why MathF.Pow and not MathF.Exp?

Why (float)Math.E and not MathF.E?

Copy link
Member

Choose a reason for hiding this comment

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

@tannergooding, can you submit a cleanup PR with the changes you think should be made? Thanks.

michaelgsharp added a commit to michaelgsharp/runtime that referenced this pull request Sep 18, 2023
@michaelgsharp
6 more naive methods for Tensor Primitives. (dotnet#92142)
96ba8c1 
* 6 more naive methods

* updates from pr comments
ericstj pushed a commit that referenced this pull request Sep 19, 2023
@michaelgsharp @tannergooding
@ViktorHofer @stephentoub @tmds
[release/8.0] Numerics and Tensors backport (#92245)
4b7c754 
* added Bcl.Numerics

* Adding a naive implementation of various primitive tensor operations (#91228)

* Adding a naive implementation of various primitive tensor operations

* Adding tests covering the new tensor primitives APIs

* Adding tensor primitives APIs to the ref assembly

* Allow .NET Framework to build/run

* Sync TFMs between ref and src, csproj simplication and clean-up

* Apply suggestions from code review

Co-authored-by: Viktor Hofer <[email protected]>

* Don't use var

* Fix the S.N.Tensors readme and remove the file marking it as non-shipping

---------

Co-authored-by: Viktor Hofer <[email protected]>
Co-authored-by: Michael Sharp <[email protected]>

* Start vectorizing TensorPrimitives (#91596)

* Start vectorizing TensorPrimitives

Just does two functions to establish the files into which the rest of the implementations can be moved.

* 6 more naive methods for Tensor Primitives. (#92142)

* 6 more naive methods

* updates from pr comments

* Add remaining set of TensorPrimitives APIs for .NET 8 (#92154)

* Add remaining set of TensorPrimitives APIs for .NET 8

Adds non-vectorized implementations of:
- Max
- Min
- MaxMagnitude
- MinMagnitude
- IndexOfMax
- IndexOfMin
- IndexOfMaxMagnitude
- ConvertToHalf (only on .NET Core)
- ConvertToSingle (only on .NET Core)
- IndexOfMinMagnitude

Adds vectorized implementations of:
- Sum
- SumOfSquares
- SumOfMagnitudes
- Product
- ProductOfSums
- ProductOfDifferences

Also includes the helpers that'll make it trivial to vectorize Dot.

Beyond vectorizing the non-vectorized ones, the vectorized implementations should be improved further, including:
- Handling alignment better
- Vectorizing the remainder that doesn't fit in a vector rather than falling back to scalar

* Cleanup after previous PR, vectorize CosineSimilarity/Dot/L2Normalize/Distance, add tests

* Address PR feedback, and fix a few other issues

* Fix TensorPrimitives.CosineSimilarity to use vectorized implementations (#92204)

* Fixed duplicated code from merge.

* New Microsoft.BCL.Numerics package (#91074)

* bcl numberics library added

* bcl done

* added explicit 2.1 target

* Minor doc updates

* Apply suggestions from code review

Co-authored-by: Viktor Hofer <[email protected]>

* fixes from PR comments

* minor csproj fixes

* fixed ref target frameworks

* minor ref csproj updates

* minor csproj updates

---------

Co-authored-by: Viktor Hofer <[email protected]>

* Microsoft.Bcl.Numerics.Tests: fix restore failure when DotNetBuildFromSource. (#91402)

* Microsoft.Bcl.Numerics.Tests: fix restore failure when DotNetBuildFromSource.

* Use NetCoreAppCurrent.

* Try fix CI test failures.

---------

Co-authored-by: Tanner Gooding <[email protected]>
Co-authored-by: Viktor Hofer <[email protected]>
Co-authored-by: Stephen Toub <[email protected]>
Co-authored-by: Tom Deseyn <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tannergooding tannergooding tannergooding left review comments

@gfoidl gfoidl gfoidl left review comments

@stephentoub stephentoub stephentoub approved these changes

@jeffhandley jeffhandley jeffhandley left review comments

@ericstj ericstj ericstj approved these changes

Assignees

@michaelgsharp michaelgsharp

Labels
area-System.Numerics.Tensors new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@michaelgsharp @jeffhandley @stephentoub @gfoidl @ericstj @tannergooding