Skip to content

Conversation

@oscardssmith
Copy link
Member

Instead of using sin_kernel and cos_kernel, use better kernels that need less precision.

I benchmark this as 14% faster for Float64, 7% faster for Float32.

Accuracy for Float64 is .64 ULPs compared to .75 for master.

Accuracy for Float32 is .5 ULP

@oscardssmith oscardssmith added performance Must go faster maths Mathematical functions labels Jul 31, 2021
@staticfloat
Copy link
Member

Thanks Oscar. For my own curiosity, how did you generate these polynomial coefficients? If it's not too much code, it would be cool to record that for future usage with other special functions, so that you're not the only person who knows how to do this.

Test failures look like cospi_kernel(::BigFloat) is trying to be called.

@oscardssmith
Copy link
Member Author

oscardssmith commented Aug 13, 2021

The coefficient generation is slightly complex, but the TLDR is to use Remez to generate a minimax polynomial over the range of inputs, specialized to the even/oddness of the polynomial. I really need to put the general principals all into a blogpost at some point, but the TLDR for this is

using Remez
sinpi_coeffs = [(big(pi))^n/factorial(big(n)) for n in 1:2:40] #nonzero taylor coefficients in bigfloat
sinpi_kinda(x) = evalpoly(x, sinpi_coeffs)
bounds = (0, 0.25^2) #squared because we are getting coefficients for sqrt(sinpi(x))
n = 7 # degree 7 polynomial
exact_coeficients = ratfn_minimax(sinpi_kinda, bounds, n, 0)[1]
#now do lots of messing around with rounding to get things exact.

@oscardssmith
Copy link
Member Author

This has been surprisingly tricky to get right. At the moment, the only failing tests is the behavior as to whether 0.0 or -0.0 is returned. To me, neither is clearly right. Is there a good reason why the specific signs need to be tested? If not, I will probably loosen the tests slightly to check for ==0 rather than ===0.0

@oscardssmith oscardssmith added the triage This should be discussed on a triage call label Sep 14, 2021
@oscardssmith
Copy link
Member Author

adding triage label to discus signs of zeros.

@oscardssmith oscardssmith removed the triage This should be discussed on a triage call label Sep 16, 2021
@oscardssmith
Copy link
Member Author

removing triage tag after research suggests Base was correct.

@ViralBShah
Copy link
Member

ViralBShah commented Feb 20, 2022

Is it worth trying to get this done? I suppose too late for 1.8 but maybe 1.9?

@oscardssmith
Copy link
Member Author

yeah. it's still on my radar. I just haven't had the time recently.

@simonbyrne
Copy link
Member

re: signed zeros, the IEEE754-2019 spec states

sinPi(+n) is +0 and sinPi(−n) is −0 for positive integers n
cosPi(n + 1⁄2) is +0 for any integer n when n + 1⁄2 is representable

@oscardssmith
Copy link
Member Author

I think this is finally ready to go. Anyone want to review?

Copy link
Member

@gbaraldi gbaraldi left a comment

Choose a reason for hiding this comment

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

LGTM

@oscardssmith oscardssmith merged commit afe10f9 into JuliaLang:master Nov 15, 2022
@oscardssmith oscardssmith deleted the better-sinpi branch November 15, 2022 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maths Mathematical functions performance Must go faster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants