-
Notifications
You must be signed in to change notification settings - Fork 369
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
Add Angle::sinCosSnap to avoid small errors, e.g. with buffer operations #978
Conversation
Are these called infrequently? Is there any advantage to inlining? |
A typical point buffer will call these trig functions 33 times. I'll investigate inlining... (I'm still novice with C++) |
I've been checking the performance of this PR with mpgrid.txt (a 20x20 MultiPoint grid): ./bin/geosop -a mpgrid.txt buffer 0.1 -v -r 100 > /dev/null and on my oldish laptop the results show a minor slowdown, e.g. 810,667 usec with main vs. 822,922 usec with this PR. I'm unsure how to restructure this PR as an |
Hi @mwtoews , you can move the implementation of |
Thanks @dbaston that was helpful! I've not pushed the inline version here, but can if needed. Running the same multipoint grid example shown above, I'm seeing pretty much the same range of benchmarks for three groups: (1) from main, (2) this PR and (3) an inline version of this PR. There isn't any clear outlier. I'm inclined to stick with the current form. Further review or ideas are welcome. |
Another idea is to cache 32 pairs of results. Running ctest and capturing the frequency of angles shows some prominent hits, Here is the number of counts for the top 200 hits: Rank, count, angle, turns
However, I'm not sure how to implement this caching/lookup. Or if it would make single point buffers slower. It might be more worthwhile adding more prepared trig values beyond 0, 45, 90 etc. already implemented, e.g. exact trigonometric values. Looking at the results a bit more, it could make sense to try adding a |
Don't polish the 💩 too much, you've already shown that it's only a small performance change, hard to measure, I think know that it's a super critical path, so perhaps just go ahead and commit. |
Ok, I think I'm done the pedanticness. I tried the look-up-of-32-angles approach and didn't see any performance gains, so I'm going with a much simpler inline approach to just clip trig values within 5e-16 to zero. The performance is the same. |
This is a rather pedantic PR to avoid small rounding errors from
std::sin(M_PI)
andstd::cos(M_PI_2)
, which are small non-zero values.Consider a unit buffer
geosop -a "POINT(0 0)" buffer 1
here with GEOS 3.12.0:POLYGON ((1 0, 0.9807852804032304 -0.1950903220161282, 0.9238795325112867 -0.3826834323650898, 0.8314696123025452 -0.5555702330196022, 0.7071067811865476 -0.7071067811865475, 0.5555702330196023 -0.8314696123025452, 0.3826834323650898 -0.9238795325112867, 0.1950903220161283 -0.9807852804032304, 0.0000000000000001 -1, -0.1950903220161282 -0.9807852804032304, -0.3826834323650897 -0.9238795325112867, -0.555570233019602 -0.8314696123025455, -0.7071067811865475 -0.7071067811865476, -0.8314696123025453 -0.5555702330196022, -0.9238795325112867 -0.3826834323650899, -0.9807852804032304 -0.1950903220161286, -1 -0.0000000000000001, -0.9807852804032304 0.1950903220161284, -0.9238795325112868 0.3826834323650897, -0.8314696123025455 0.555570233019602, -0.7071067811865477 0.7071067811865475, -0.5555702330196022 0.8314696123025452, -0.3826834323650903 0.9238795325112865, -0.1950903220161287 0.9807852804032303, -0.0000000000000002 1, 0.1950903220161283 0.9807852804032304, 0.38268343236509 0.9238795325112866, 0.5555702330196018 0.8314696123025455, 0.7071067811865474 0.7071067811865477, 0.8314696123025452 0.5555702330196022, 0.9238795325112865 0.3826834323650904, 0.9807852804032303 0.1950903220161287, 1 0))
and since #973 for
main
the small values are formatted using scientific notation:POLYGON ((1 0, 0.9807852804032304 -0.1950903220161282, 0.9238795325112867 -0.3826834323650898, 0.8314696123025452 -0.5555702330196022, 0.7071067811865476 -0.7071067811865475, 0.5555702330196023 -0.8314696123025452, 0.3826834323650898 -0.9238795325112867, 0.1950903220161283 -0.9807852804032304, 6.123233995736766e-17 -1, -0.1950903220161282 -0.9807852804032304, -0.3826834323650897 -0.9238795325112867, -0.555570233019602 -0.8314696123025455, -0.7071067811865475 -0.7071067811865476, -0.8314696123025453 -0.5555702330196022, -0.9238795325112867 -0.3826834323650899, -0.9807852804032304 -0.1950903220161286, -1 -1.2246467991473532e-16, -0.9807852804032304 0.1950903220161284, -0.9238795325112868 0.3826834323650897, -0.8314696123025455 0.555570233019602, -0.7071067811865477 0.7071067811865475, -0.5555702330196022 0.8314696123025452, -0.3826834323650903 0.9238795325112865, -0.1950903220161287 0.9807852804032303, -1.8369701987210297e-16 1, 0.1950903220161283 0.9807852804032304, 0.38268343236509 0.9238795325112866, 0.5555702330196018 0.8314696123025455, 0.7071067811865474 0.7071067811865477, 0.8314696123025452 0.5555702330196022, 0.9238795325112865 0.3826834323650904, 0.9807852804032303 0.1950903220161287, 1 0))
this PR would use exact values if the angles are a multiple of 45 degrees (pi/4) on the unit circle:
POLYGON ((1 0, 0.9807852804032304 -0.1950903220161282, 0.9238795325112867 -0.3826834323650898, 0.8314696123025452 -0.5555702330196022, 0.7071067811865476 -0.7071067811865476, 0.5555702330196023 -0.8314696123025452, 0.3826834323650898 -0.9238795325112867, 0.1950903220161283 -0.9807852804032304, 0 -1, -0.1950903220161282 -0.9807852804032304, -0.3826834323650897 -0.9238795325112867, -0.555570233019602 -0.8314696123025455, -0.7071067811865476 -0.7071067811865476, -0.8314696123025453 -0.5555702330196022, -0.9238795325112867 -0.3826834323650899, -0.9807852804032304 -0.1950903220161286, -1 0, -0.9807852804032304 0.1950903220161284, -0.9238795325112868 0.3826834323650897, -0.8314696123025455 0.555570233019602, -0.7071067811865476 0.7071067811865476, -0.5555702330196022 0.8314696123025452, -0.3826834323650903 0.9238795325112865, -0.1950903220161287 0.9807852804032303, 0 1, 0.1950903220161283 0.9807852804032304, 0.38268343236509 0.9238795325112866, 0.5555702330196018 0.8314696123025455, 0.7071067811865476 0.7071067811865476, 0.8314696123025452 0.5555702330196022, 0.9238795325112865 0.3826834323650904, 0.9807852804032303 0.1950903220161287, 1 0))
Other changes in this PR use angle constants a bit more consistently, an also share
Angle::sinCos
in other places, where both calculations are performed.Note that an alternative proposal for a much simpler
Angle::sinCos
would only zap small values fromstd::sin
andstd::cos
to zero, rather than a lookup approach on 45 degrees of a unit circle currently in this PR. (Update: that's actually the one that ultimately ended up in this PR.)