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

Use arctan2 instead of arcsin when converting to spherical coordinates #979

Merged

Conversation

wedesoft
Copy link
Contributor

This pull request avoids using the _AVOID_DIVIDE_BY_ZERO term in two cases.

@brandon-rhodes
Copy link
Member

Wow! This pull request is a surprise. I had always imagined arctan2(), with its multiple arguments, being much slower than arcsin(). And the call to hypot(), which I had thought might involve an intermediate Python call, in fact looks to be implemented in C. With the result that this patch seems to only incur about a 1% speed penalty for the routine, while eliminating the use of a somewhat awkward constant.

I'm inclined, therefore, to merge. I'll wait till tomorrow in case any edge cases occur to me, but I think this is likely to land. Thanks!

@wedesoft
Copy link
Contributor Author

Ok great. I think atan2 is numerically more stable than arcsin especially for angles close to +90 or -90 degrees where sinus is near 1.0 or -1.0.

@brandon-rhodes brandon-rhodes merged commit b270bcb into skyfielders:master Jul 17, 2024
3 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.

2 participants