-
Notifications
You must be signed in to change notification settings - Fork 11
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
faster besselj0 for x<25 #15
Conversation
Though it's another branch. It might be best to keep the power series for very small arguments to ensure that |
Fixed. I used 1 to few terms, and didn't force the 1st coefficient to be 1.0. |
Codecov Report
@@ Coverage Diff @@
## master #15 +/- ##
==========================================
- Coverage 99.56% 99.55% -0.01%
==========================================
Files 12 13 +1
Lines 683 676 -7
==========================================
- Hits 680 673 -7
Misses 3 3
Continue to review full report at Codecov.
|
Also, for future reference, here is the code used to generate the polynomials
for the 0-pi/2 range, the invocation was |
Makes sense. Do you want to do the honors, or should I? |
If you have it set up that might be easiest! Otherwise, I can look more at this tomorrow and also for |
updated (the post with computation details and error plots are also updated). |
To clarify the above code. You will need using ArbNumerics, Remez
pts = (
(0.73676709589402, 0.0),
(2.404825557695773, -1.176691651530894e-16),
(3.8317059702075125, -1.5269184090088067e-16),
(5.520078110286311, 8.088597146146722e-17),
(7.015586669815619, -9.414165653410389e-17),
(8.653727912911013, -2.92812607320779e-16 ),
(10.173468135062722, 4.482162274768888e-16),
(11.791534439014281, 2.812956912778735e-16),
(13.323691936314223, 2.600408064718813e-16),
(14.930917708487787, -7.070514505983074e-16),
(16.470630050877634, -1.619019544798128e-15),
(18.071063967910924, -9.658048089426209e-16),
(19.615858510468243, -1.004445634526616e-15),
(21.21163662987926 , 4.947077428784068e-16),
(22.760084380592772, -4.925749373614922e-16),
(24.352471530749302, 9.169067133951066e-16),
(25.903672087618382, 4.894530726419825e-16),
(26.1,0.0)
)
function r(x,root)
abs(x)<eps(Float64) && return -besselj(1,ArbFloat(x)+root[1]+root[2])
return besselj(0,ArbFloat(x)+root[1]+root[2])/x
end
function r2(x,root)
return besselj(0,ArbFloat(x)+root[1]+root[2])
end
polys = []
for i in 2:2:16
poly = Float64.(Tuple(ratfn_minimax(x->r(x,pts[i]), ((pts[i-1][1]-pts[i][1]-.1)/2, (pts[i+1][1]-pts[i][1]+.1)/2), 12, 0)[1]))
push!(polys, (0.0, poly...))
poly = Float64.(Tuple(ratfn_minimax(x->r2(x,pts[i+1]), ((pts[i][1]-pts[i+1][1]-.1)/2, (pts[i+2][1]-pts[i+1][1]+.1)/2), 13, 0)[1]))
push!(polys, poly)
end
Tuple(polys) Edited with correct values. |
The last 2 points are wrong. They should be |
It looks like I also misspoke were |
The derivative of |
This now is implemented for |
This is now failing tests because SpecialFunctions is too inaccurate near |
I agree that we should probably switch the tests over to Arb for all this. I actually had a draft of this early on but ran into JeffreySarnoff/ArbNumerics.jl#48 causing some issues. Now that it is fixed we should move over. |
2.56987256757748830383E5, 6.20836478118054335476E2, | ||
1.00000000000000000000E0 | ||
) | ||
const PP_j1(::Type{Float64}) = ( |
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.
Unfortunately several of these constants are used for the bessely
implementations.
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.
it will work out once we replace those implementations as well :)
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.
O most definitely!! Didn't know if you wanted to do that in separate PR or all at once 😃
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.
I'm probably going to save those for a separate PR. They look worse.
src/besselj.jl
Outdated
(24.352471530749302, 9.169067133951066e-16), | ||
(25.903672087618382, 4.894530726419825e-16) | ||
) | ||
@inline J0_POLYS(::Type{Float64}) = ( |
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.
Is there a reason we sometimes inline these, sometimes we don't, and sometimes we use constants for the polynomials? It might be good to have a defined way going forward that is consistent. Though this one is slightly different as we have to determine which polynomial at runtime. Does this affect compile time?
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.
Also, I would be a fan of moving the polynomials to their own file and slightly different file structure to make the core code easier to read. I think this approach could also be used for Double64
implementations that would also take up a lot of space in main file.
src/besselj.jl
Outdated
@@ -114,34 +217,29 @@ function besselj1(x::Float64) | |||
x = abs(x) | |||
isinf(x) && return zero(x) |
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.
We should probably be consistent with the structure between besselj0
and besselj1
. Either have the isinf check in the large argument branch (where I check if xinv is zero) or just have it at beginning.
src/besselj.jl
Outdated
p = p * sc[2] - w * q * sc[1] | ||
return p * SQ2OPI(T) / sqrt(x) | ||
if x <= 26.0 | ||
x <= pi/2 && return x*evalpoly(x*x, J1_POLY_PIO2(T)) |
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.
Also, do we want to set a code style for this repo? I think in general I've been trying to use blue but sometimes I obsess over spaces too much 😅
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.
I tend to write with relatively little spacing consistency. If you have a style guide you would prefer, I can adhere to it, but my default tends to be somewhat arbitrary.
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.
I usually try to follow https://github.com/jrevels/YASGuide. I can make some spacing changes if that's ok.
Otherwise I think this is ready to merge?
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.
sounds good. (make any changes you want).
Tests look good now. Do we want to also update the Float32 implementations here or later? |
I think we should do Float32 separately because I think this method might not be the best for lower precision. |
I added a fix to the sign error for |
else | ||
xinv = inv(x) | ||
x2 = xinv*xinv | ||
iszero(xinv) && return zero(T) |
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.
technically this should probably return xinv
so it returns -0.0
when applicable.
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.
I believe since we have x=abs(x)
this would still always return 0.0
so might need to handle that separately. I'm actually starting to not appreciate that syntax of overwriting that variable and prefering xabs = abs(x)
. Thoughts?
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.
I don't think it really matters. I would probably just merge this as is.
this is about 2x faster on my computer where applicable. This also should be done for
besselj1
andbessely0
,bessely1