-
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
Implement SinPi, CosPi and TanPi and inverses #53675
Conversation
Note regarding the 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. |
Tagging subscribers to this area: @tannergooding, @pgovind Issue DetailsPart 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?
|
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
Yes. We need explicit tests for all edge cases explicitly specified by IEEE 754. That includes the special handled of
|
Yes I've read IEEE spec (I have access through my university). |
Inverses should be fairly easier, since there's no need to fold the numbers. |
c8bd147
to
4b809ba
Compare
@tannergooding This PR is assigned to you for follow-up/decision before the RC1 snap. |
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) |
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? |
@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. |
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. |
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?