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
improved doc for MathUtils sin lookup table #5807
Conversation
What are these graphs even showing? |
X axis : input angle It illustrates precision loss when converting from an angle to lookup table index. |
I'd be very happy to have a graph with a proper scale :) |
Sure, I can get on that. The main difference is obvious: some values are produced more often than others when the input is large, vs. when it is small. In particular, the correct graph should produce 0 (the center of the x axis is 0) as the least frequent value, but it's either the most frequent or second-most frequent value when using large degree inputs. |
@@ -59,22 +59,26 @@ | |||
} | |||
} | |||
|
|||
/** Returns the sine in radians from a lookup table. */ | |||
/** Returns the sine in radians from a lookup table. For optimal precision, use radians that are not drastically more positive |
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 this the same as: "For optimal precision use radians in the range [-PI2, PI2]."?
If so in my opinion this would be more understandable.
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.
Phrasing is going to be tricky here. I can try to figure out where it starts losing precision, but it's probably somewhere greater than PI2
or less than -PI2
. The lookup table has 18 bits of precision I think, with 65536 entries corresponding to one quadrant, but it could be lower. That might be just 16 bits of precision, depending on how you measure it. A float has 23 mantissa bits, and those represent larger and larger units as the exponent increases, so you'd only run out of mantissa outside (-32,32)
or (-128,128)
, and I'll need to run some tests to make sure. Even then, the difference is small around that limit, but if you don't somehow limit how high or low the radians
argument goes, it is very easy to exceed the precise range.
But yeah, I like your comment text better except for the quirk where not every school system teaches that syntax for a closed interval (it might be like the local difference between 1,000,000
and 1.000.000
, and I don't know specifics other than sometimes people don't understand when I use that syntax). Maybe "For optimal precision, use radians between -PI2 and PI2, both inclusive."
Would it make sense to apply the bounding (% 360) in the implementation itself? Performance wise would probably be unnoticeable. |
I was going to suggest that as well. |
The modulus, done internally or externally, does help up to a point. It doesn't improve the speed at all though, and with recent Java versions public static float asin(float a) {
return (a * (1f + (a *= a) * (-0.141514171442891431f + a * -0.719110791477959357f))) /
(1f + a * (-0.439110389941411144f + a * -0.471306172023844527f));
}
public static float acos(float a) {
return 1.5707963267948966f - (a * (1f + (a *= a) * (-0.141514171442891431f + a * -0.719110791477959357f))) /
(1f + a * (-0.439110389941411144f + a * -0.471306172023844527f));
} The idea of putting a note about the domain By the way, passing any integer larger than 823550 to |
Generally LUTs should be avoided unless working around a performance issue, in which case you wouldn't want modulus 360 on the input. Javadoc improvements look good. acos/asin is interesting, is there anything we can add to the javadocs to give users a better idea of how to use them? |
Uh sure, the Javadocs in the project that I originally put them in weren't great, but the behaviors for those two are supposed to be the same as Math.asin() and Math.acos(). It may make sense to refer to the docs for those JDK methods. For asin(), it takes a float from -1 to 1 inclusive and returns a float from -PI/2f to PI/2f inclusive. For acos(), it takes a float from -1 to 1 inclusive and returns a float from 0 to PI inclusive. It doesn't check that input is in range, but asin() and acos() are only defined for inputs from -1 to 1 in math. If you want to give credit to the guy who found this approximation, both approximations use formula number 201 in Dennis Kjaer Christensen's unfinished math work on arc sine approximation. That's just a formula, and I wrote the gnarly Java two-liners, but the magic numbers this time are not mine, they're Christenson's. |
Thanks for the extra information. More specifically, it may help people use the functions vs Math if they have some idea about the accuracy of the approximations. |
The most exact info I have is from Christensen, this for asin() on doubles:
But, I'll also get some graphs up for MathUtils.asin() relative to Math.sin() and also for acos(). The error will probably be worse than the double version because MathUtils uses floats. |
As you can see in graphs below, using large angle values can give bad precisions :
This one is using high values unbounded (360000 to 360360)
This one is using same values but bounded (using % 360 before lookup table) which is the same as (0 to 360) range.
Thanks to @tommyettinger for these graphs and his help.