-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Fix ACOS precision issue for boundary values (x=±1.0) #18582
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
Conversation
The ACOS operator was producing incorrect results for boundary values due to poor precision of ASIN's Taylor series expansion near x=±1.0. Root cause: - ASIN used a 6-term Taylor series that converges slowly near boundaries - ACOS was implemented as acos(x) = π/2 - asin(x), inheriting ASIN errors - At x=1.0, ASIN error of 0.354874 (22.6%) caused ACOS to output 0.354874 instead of 0.0 Solution: - Modified ASIN to use system library function (asinf) for |x| >= 0.9 - Modified ACOS to use system library function (acosf) for |x| >= 0.9 - For |x| < 0.9, continue using Taylor series (accurate in this range) This ensures high precision for boundary values while maintaining the existing behavior for values in the middle range. Fixes apache#18580
Summary of ChangesHello @Dayuxiaoshui, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the accuracy of the ACOS and ASIN trigonometric functions, particularly at their domain boundaries. By strategically switching to system library functions for extreme input values while retaining the efficient Taylor series for central values, the change ensures robust and precise mathematical computations without sacrificing performance where the series remains effective. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request addresses a precision issue with ACOS and ASIN operators for boundary values (x=±1.0) by switching to system library functions when |x| >= 0.9. The changes for tir.asin look good, with an explicit domain check and conditional logic to use either the system library or a Taylor series expansion. For tir.acos, I've suggested adding a similar explicit domain check to ensure consistent and robust behavior for out-of-domain inputs, mirroring the implementation of tir.asin.
|
Thanks for the fix! Would you mind adding a test to cover the input? The test should be at |
|
@tlopex Ok, no problem, I'll go add some tests first. |
2f98016 to
f20911b
Compare
…ests
- Lower threshold from 0.9 to 0.5 for asin/acos legalization
* For |x| >= 0.5: use system library function for better accuracy
* For |x| < 0.5: use Taylor series for efficiency
* This improves precision for values near the threshold while maintaining
performance for smaller values
- Add test_asin_acos_boundary_values() test function
* Tests boundary values (±1.0)
* Tests threshold switching point (±0.5)
* Tests values below threshold (±0.49, ±0.3, 0.0)
* Tests out-of-domain values (should return NaN)
This addresses precision issues with asin/acos near boundary values.
f20911b to
b584e10
Compare
tlopex
left a comment
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.
LGTM Thanks!
The ACOS operator was producing incorrect results for boundary values due to poor precision of ASIN's Taylor series expansion near x=±1.0.
Root cause:
Solution:
This ensures high precision for boundary values while maintaining the existing behavior for values in the middle range.
Fixes #18580