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

Vectorize TensorPrimitives.Acosh/sinh/tanh in terms of Log #98302

Closed
wants to merge 1 commit into from

Conversation

stephentoub
Copy link
Member

Contributes to #97193

@ghost
Copy link

ghost commented Feb 12, 2024

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

Issue Details

Contributes to #97193

Author: stephentoub
Assignees: -
Labels:

area-System.Numerics.Tensors

Milestone: -

@@ -365,7 +365,7 @@ public static IEnumerable<object[]> SpanDestinationFunctionsToTest()
yield return new object[] { new SpanDestinationDelegate(TensorPrimitives.Acosh), new Func<T, T>(T.Acosh) };
yield return new object[] { new SpanDestinationDelegate(TensorPrimitives.AcosPi), new Func<T, T>(T.AcosPi) };
yield return new object[] { new SpanDestinationDelegate(TensorPrimitives.Acos), new Func<T, T>(T.Acos) };
yield return new object[] { new SpanDestinationDelegate(TensorPrimitives.Asinh), new Func<T, T>(T.Asinh) };
yield return new object[] { new SpanDestinationDelegate(TensorPrimitives.Asinh), new Func<T, T>(T.Asinh), T.CreateTruncating(0.001) };
Copy link
Member

Choose a reason for hiding this comment

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

This is quite a lot of variance being allowed, we're basically saying we can't get within 3 digits of accuracy for some inputs.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you suggest?

(Note that the default being used everywhere right now is 0.0001.)

Copy link
Member

Choose a reason for hiding this comment

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

What I'd like to understand is if this a variance being introduced more generally, or just for the special values we test like MaxValue/MinValue. If it's all or most values, then I don't think we can take the implementation. If its only for these extreme boundary values, then I think we're perfectly fine.

For a TL;DR. I think the upper boundary for a normal input is:

  • float - 2^17
  • double - 2^46

Beyond that we've already started with enough precision loss that its not going to be super meaningful no matter what we do.


For float we can exactly represent any integer up to 2^24. We can only track any fractional portion up to 2^23 (where the distance between values is 0.5). At 2^8 (256) the distance between values is 0.000030517578125, which is just enough to represent 5 significant digits of PI. For double, the same distance between values occurs at 2^37.

The ability to represent 5 digits for PI (3.1415) is a reasonably boundary at which these types of trigonometric functions can be expected to be most accurate, regardless of underlying implementation. On the more extreme end, we can represent 3 digits for PI (3.14) at 2^17 (float) or 2^46 (double). With any fewer digits we've lost enough precision that results are less meaningful, so I think this is a good maximum range for expected inputs to be accurate.

For typical inputs, we really want to expect 6-9 digits of accuracy for float and 15-17 digits of accuracy for double. With an allowance for this accuracy to taper off as the input grows in magnitude.

if (typeof(T) == typeof(float))
{
Vector128<float> f = x.AsSingle();
return (Vector128.Create(0.5f) * LogOperator<float>.Invoke((Vector128<float>.One + f) / (Vector128<float>.One - f))).As<float, T>();
Copy link
Contributor

@lilinus lilinus Feb 12, 2024

Choose a reason for hiding this comment

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

A more numerically stable formula for small f would be to use logp1
Vector128.Create(0.5f) * LogP1Operator<float>.Invoke(Vector128<float>.Create(2f) * f / (Vector128<float>.One - f))

Although I think logp1 is just implemented as log(x+1) at the moment...

@stephentoub
Copy link
Member Author

This was a background activity for me and I'd intended for it to be a background activity for others, but I'm told it's a distraction, so I'm going to close it for now and it can be resuscitated at some point in the future. Thanks.

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