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

implement a fast atan approximation function #1583

Merged
merged 33 commits into from
Jun 22, 2024

Conversation

zhichen3
Copy link
Collaborator

@zhichen3 zhichen3 commented Jun 18, 2024

implement a fast atan function using an approximate function for x in [-1, 1], for |x| > 1, the identity arctan(x) = sign(pi/2, x) - arctan(1/x) (Thanks to Eric) is used.

Two versions of the approximate functions are included.

  1. Efficient Approximations for the Arctangent Function by Rajan 2006: Equation 9: Max error ~0.0015 rad
  2. https://stackoverflow.com/questions/42537957/fast-accurate-atan-arctan-approximation-algorithm: Max error ~0.00063 rad

Generally I find the form from stackoverflow is more accurate and also faster. 1) is also discussed in 2).

In general, from testing, atanf is roughly more than 2 times faster than std::atan.

@zhichen3 zhichen3 changed the title Fast atan implement a fast atan approximation function Jun 19, 2024
@zhichen3 zhichen3 marked this pull request as ready for review June 20, 2024 01:28
@zhichen3
Copy link
Collaborator Author

Not sure why hip failed to compile. Also I kept the volatile qualifier for testing because runtime wouldn't increase with iteration otherwise.

@zingale
Copy link
Member

zingale commented Jun 20, 2024

I suspect the HIP failure is due to changes in ROCm and there is a namespace clash now.

@zingale
Copy link
Member

zingale commented Jun 20, 2024

can you try not including <AMReX_GpuUtility.H> and instead directly include just what you need?

@zingale
Copy link
Member

zingale commented Jun 20, 2024

or alternately, include <AMReX_Gpu.H>, since that seems to do some namespace magic.

@yut23
Copy link
Collaborator

yut23 commented Jun 20, 2024

It's probably this line:

using namespace amrex;

@yut23
Copy link
Collaborator

yut23 commented Jun 20, 2024

atanf() may conflict with the definition from the standard library. I think fast_atan() works fine.

@zingale
Copy link
Member

zingale commented Jun 20, 2024

indeed, we should get rid of those using namespace amrex
I'll try that in a separate PR

@zhichen3
Copy link
Collaborator Author

I figured checking nan is unnecessary, so I removed it and also removed AMReX_GpuUtility.H since I don't need isnan anymore. I guess this also fixed hip compilation error.

@zingale zingale merged commit 134200e into AMReX-Astro:development Jun 22, 2024
29 checks passed
@yut23
Copy link
Collaborator

yut23 commented Jun 25, 2024

I've been looking at the screening code a bunch for the autodiff stuff, and I think we may want to include the third-order term in the Taylor series, since we do sqrt_gamma - atan(sqrt_gamma) which cancels to zero at first-order. Here's a comparison between the first and third order series: the relative error of the first-order series goes up to around 32% and levels off. There's also a discontinuity when it switches from fast_atan_1 to the Taylor series.

chugunov2009_f0_comparison

@zhichen3
Copy link
Collaborator Author

sounds good to me

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.

3 participants