Compute paired sine/cosine together in the coordinate transforms#1188
Open
alexey-milovidov wants to merge 1 commit into
Open
Compute paired sine/cosine together in the coordinate transforms#1188alexey-milovidov wants to merge 1 commit into
alexey-milovidov wants to merge 1 commit into
Conversation
The lat/lng <-> cell coordinate transforms repeatedly need both sin(x) and cos(x) of the same angle but computed them with two separate libm calls: * latLngToVec3 - sin/cos of the latitude and of the longitude * _vec3ToHex2d - sin/cos of theta * _hex2dToVec3 - sin/cos of theta and of r Add a small _sincos() helper (it lowers to a single libm `sincos` call where available and otherwise falls back to separate sin()/cos()) and use it in these spots. Computing the pair together shares the argument reduction, so it is faster while returning identical values. The results are bit-for-bit identical; all existing tests pass unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dfellis
approved these changes
Jul 4, 2026
isaacbrodsky
reviewed
Jul 5, 2026
| */ | ||
| static inline void _sincos(double x, double *s, double *c) { | ||
| #if defined(__GNUC__) || defined(__clang__) | ||
| __builtin_sincos(x, s, c); |
Collaborator
There was a problem hiding this comment.
It looks like this is not compiling as expected. Does it maybe require #define _GNU_SOURCE before the include?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Compute paired sine/cosine together in the coordinate transforms
The
lat/lng <-> cellcoordinate transforms repeatedly need bothsin(x)andcos(x)of the same angle, but currently compute them with two independent libm calls:latLngToVec3(vec3d.h) —sin/cosof the latitude and of the longitude_vec3ToHex2d(faceijk.c) —sin/cosoftheta_hex2dToVec3(faceijk.c) —sin/cosofthetaand ofrThis adds a tiny
_sincos()helper tomathExtensions.h(it lowers to a single libmsincoscall where one exists via__builtin_sincos, and falls back to separatesin()/cos()otherwise, so it stays portable) and uses it in those spots. Computing the pair together shares the argument reduction and polynomial evaluation.The recent switch to a 3D-Cartesian projection already removed most of the spherical trig (e.g.
_vec3AzimuthRadsno longer callssin/cos); this cleans up the paired calls that remain.Correctness
The results are bit-for-bit identical —
sincosreturns the same values as separatesin/cos, and the operand order is unchanged. All existing tests pass unchanged (ctest: 317/317).Performance
On a synthetic 10M-point benchmark (
clang -O3), fusing the paired calls measurably speeds up the boundary/center/area transforms (single-digit to low-double-digit percent, depending on the function and platform libm).sincosis ~1.2–1.3× cheaper thansin+cosfor real-range arguments on glibc.No behavior, API, or output changes.