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

Implement SinPi, CosPi and TanPi and inverses #53675

Closed
wants to merge 15 commits into from

Conversation

huoyaoyuan
Copy link
Member

Part of #20137

Implementations are copied from boost/math. However, I can't find AsinPi/AcosPi/AtanPi implementation there. It feels better to add them together.

Question: IEEE754 spec explicitly specifies +0 and -0. Do we need to test it?

@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, to 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
Copy link

ghost commented Jun 3, 2021

Tagging subscribers to this area: @tannergooding, @pgovind
See info in area-owners.md if you want to be subscribed.

Issue Details

Part of #20137

Implementations are copied from boost/math. However, I can't find AsinPi/AcosPi/AtanPi implementation there. It feels better to add them together.

Question: IEEE754 spec explicitly specifies +0 and -0. Do we need to test it?

Author: huoyaoyuan
Assignees: -
Labels:

area-System.Numerics, new-api-needs-documentation

Milestone: -

@tannergooding
Copy link
Member

Thanks for your contribution. I'll need to do a license check to confirm boost is compatible (I believe it is) and also consider whether this is something we are willing to take as the implementation.

In particular, there are some potential problems to implementing this as managed code such as the ease in which we can add constant folding support in the JIT. This likewise differs from the amd/aocl-libm-ose implementation I was looking into for this already, which is known to be performant and which is effectively an extension of the code that already powers the x64 Windows math implementations (which is amd/win-libm).

Question: IEEE754 spec explicitly specifies +0 and -0. Do we need to test it?

Yes. We need explicit tests for all edge cases explicitly specified by IEEE 754. That includes the special handled of +0 and -0 for sinpi, cospi, and tanpi. It likewise includes:

  • sinpi(+n) is +0 and sinpi(-n) is -0 for positive integer n
  • cospi(n + 0.5) is +0 for any integer n where n + 0.5 is representable
  • tanpi(2 * n + 0.5) is +infinity for positive integer n
  • tanpi(2 * n + 1) is -0 for positive integer n
  • tanpi(2 * n + 1.5) is -infinity for positive integer n
  • tanpi(2 * n + 2) is +0 for positive integer n
  • tanpi(2 * n - 0.5) is -infinity for negative integer n
  • tanpi(2 * n - 1) is +0 for negative integer n
  • tanpi(2 * n - 1.5) is +infinity for negative integer n
  • tanpi(2 * n - 2) is -0 for negative integer n

@huoyaoyuan
Copy link
Member Author

Yes I've read IEEE spec (I have access through my university).
So boost's implementation is effectively wrong, because it returns -0 for odd numbers.
I think I can write a new implementation.

@huoyaoyuan huoyaoyuan changed the title Implement SinPi, CosPi and TanPi Implement SinPi, CosPi and TanPi and inverses Jun 4, 2021
@huoyaoyuan
Copy link
Member Author

Inverses should be fairly easier, since there's no need to fold the numbers.

@huoyaoyuan huoyaoyuan marked this pull request as ready for review June 4, 2021 15:35
@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@jeffhandley
Copy link
Member

@tannergooding This PR is assigned to you for follow-up/decision before the RC1 snap.

@tannergooding
Copy link
Member

Thanks for the contribution here @huoyaoyuan. Due to it being so late in the cycle, this is going to miss out on the .NET 6 release and we'll instead be able to merge it after the RC1 snap and we begin accepting changes for .NET 7 (ETA: 3-4 weeks)

@tannergooding tannergooding added this to the 7.0.0 milestone Aug 2, 2021
@huoyaoyuan
Copy link
Member Author

The implementation (in fact software fallbacks) may benefit from generic math. I agree to delay it and make it a very first consumer of generic math.

@jeffhandley
Copy link
Member

The implementation (in fact software fallbacks) may benefit from generic math. I agree to delay it and make it a very first consumer of generic math.

@huoyaoyuan / @tannergooding Should we close this PR then to have it replaced with a new one that uses that approach?

@tannergooding
Copy link
Member

@jeffhandley, beyond that I think this needs more consideration with regards to the future direction of System.Math (as alluded to in #53675 (comment)).

We still need to determine if implementing this in managed vs native is appropriate and should ensure that whatever we choose stays generally consistent (in terms of algorithm source and implementation location) for the various Math APIs.

@jeffhandley
Copy link
Member

Closing this PR as the direction is uncertain at this time. Thank you, @huoyaoyuan; this effort is contributing to our strategic thinking in the Math space, so your PR was valuable even though it's not being merged.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
@huoyaoyuan huoyaoyuan deleted the trigonometric-pi branch June 24, 2022 07:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Numerics community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants