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

Fix very large argument expansion in besselj0 and friends #57

Merged
merged 1 commit into from
Oct 17, 2022

Conversation

heltonmc
Copy link
Member

@heltonmc heltonmc commented Oct 17, 2022

A fix for #56.

This reduces the error over the whole range
Screen Shot 2022-10-16 at 10 44 52 PM

There might be a better way to do this but you have to reduce x and xn first then make the subtraction so used the formula cos(x + y) = cosx cosy - sinx siny. Definitely open to other ideas. We could have a different branch within the cos_sum function but then we hit that statement for all arguments instead of just big ones. Alternatively we can just improve the cos_sum function for large arguments....

@heltonmc heltonmc linked an issue Oct 17, 2022 that may be closed by this pull request
@oscardssmith
Copy link
Member

the faster way to fix this would be to do a little extra reduction in cos_sum, but honestly this world pretty well as is.

@heltonmc
Copy link
Member Author

Happy to do whatever you think is best. I think your right that ideally this could happen within cos_sum. I think a little extra compensation there is fine because that section of the routine is already like 2.5x faster than for x < 25. And as we showed in the benchmarks the runtime seems to be really dominated by the slowest branch anyway....

@heltonmc
Copy link
Member Author

I think I'll just merge as is. I think in the future if we want to take a look at this whole section to provide full compensation this can be changed.

But #22 covers these concerns and documents this as an open issue if someone wants to tackle it (I think right now I want to focus on complex support, etc.). We just need to keep in mind in the future that if we go the route of fully compensating we must also test for x > 1e20.

@oscardssmith
Copy link
Member

that makes sense. I have somewhat gotten lost in the rabbit-hole that Julia's rem_pio2 isn't very good.

@heltonmc
Copy link
Member Author

No worries that is good to know! It's something I definitely want to come back to and keep in mind considering all the other discourse right now.

@heltonmc heltonmc merged commit b4bf95d into master Oct 17, 2022
@heltonmc heltonmc deleted the large_arg branch October 17, 2022 15:53
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.

Improve besselj accuracy for large arguments for orders -1, 0, 1
2 participants