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

Improve accuracy of sinpi and cospi #600

Merged
merged 11 commits into from
Dec 21, 2023

Conversation

OlivierHnt
Copy link
Member

@OlivierHnt OlivierHnt commented Dec 17, 2023

This PR is essentially a revival of the PR #421 which got forgotten unfortunately.
Closes #381; closes #412.

This PR also refactors the code in trigonometric.jl; the code is about 200 lines shorter.

@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (b702da4) 82.27% compared to head (b386b06) 84.76%.
Report is 1 commits behind head on master.

Files Patch % Lines
src/intervals/arithmetic/trigonometric.jl 97.01% 6 Missing ⚠️
src/intervals/rounding.jl 93.33% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #600      +/-   ##
==========================================
+ Coverage   82.27%   84.76%   +2.49%     
==========================================
  Files          23       23              
  Lines        2138     2035     -103     
==========================================
- Hits         1759     1725      -34     
+ Misses        379      310      -69     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@OlivierHnt
Copy link
Member Author

So it seems that the stack overflow comes from the fact that sinpi, and cospi are not properly defined for 32 bit system in CRlibm.jl.

Specifically, https://github.com/JuliaIntervals/CRlibm.jl/blob/d212ba9154ade995bb6c330c51092c685a8ec432/src/CRlibm.jl#L152-L155 do not include the functions sinpi, cospi, tanpi and atanpi.

I may be missing something here, but why were these functions omitted? At first glance, I would have defined function_names and MPFR_function_names to be the same.

cc @dpsanders

@OlivierHnt
Copy link
Member Author

I fixed the StackOverflow issue for cospi and sinpi in 32 bit systems; I guess I was mislead into thinking that cospi and sinpi were defined in MPFR.

@OlivierHnt OlivierHnt merged commit 945a123 into JuliaIntervals:master Dec 21, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sinpi returns unnecessarily wide intervals Use version of sin with quadrant for Float32 too
2 participants