Repository navigation
Add exact-arithmetic test of the projection - #1280
justinhwang wants to merge 1 commit into
Conversation
Adds testExactProjectionExhaustive, which measures how far the Vec3d from _faceIjkToVec3, cellToLatLng and every vertex of cellToBoundary land from the exact value of the projection they implement, using a 256-bit MPFR evaluation of the algebraic form of the same chain as the reference rather than another implementation. Separations are measured as the chord in unit-vector space and reported in degrees of arc. Every cell at resolutions 0 to 2 is checked by default, plus one deterministic descendant per base cell at resolutions 3 to 15 (the same cells as h3-go's exact_projection_test.go). H3_EXACT_PROJECTION_RES raises the full enumeration and H3_EXACT_PROJECTION_REPORT=1 prints the worst case per resolution and a histogram in ulps instead of asserting. The bound is 1e-13 degrees; the worst observed is 5.3e-14 degrees (4.1 ulp) over every cell to resolution 4 on macOS arm64. faceCenterPoint, faceAxesAzRadsCII and faceNeighbors are exported from faceijk.c so the test reads the library's own definitions. The test is built only when MPFR and GMP are found (ENABLE_EXACT_PROJECTION_TEST), installed in the gcc Linux and macOS CI jobs, and left off in the clang sanitizer jobs since libmpfr is not instrumented. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Expose projection geometry tables src/h3lib/include/faceijk.h, src/h3lib/lib/faceijk.c, src/apps/miscapps/generateFaceCenterPoint.c |
The face-center, face-axis azimuth, and face-neighbor tables now have external linkage and declarations. The face-center generator emits an externally linked array. |
Add MPFR reference and exhaustive checks src/apps/testapps/testExactProjectionExhaustive.c, CMakeTests.cmake, CMakeLists.txt |
The new test compares cell centers, vectors, and boundary vertices with high-precision reference values. CMake detects MPFR and GMP, registers the test when enabled, and includes its source in the test application list. |
Configure CI test coverage .github/workflows/test-linux.yml, .github/workflows/test-macos.yml, .github/workflows/test-linux-32bit.yml |
Linux GCC and macOS jobs install MPFR and enable the test. Linux 32-bit jobs and the Clang sanitizer job disable it. |
Priority:
Estimated code review effort: 4 (Complex) | ~50 minutes
Change: Other
Sequence Diagram(s)
sequenceDiagram
participant Test as testExactProjectionExhaustive
participant MPFR
participant CellToLatLng as cellToLatLng
participant FaceIjk as _faceIjkToVec3
participant Boundary as cellToBoundary
Test->>MPFR: Compute high-precision projection reference
Test->>CellToLatLng: Get cell center coordinates
Test->>FaceIjk: Get projected vector
Test->>Boundary: Get cell boundary vertices
Test->>Test: Compare outputs with reference values
Suggested reviewers: isaacbrodsky, wewei, ajfriend
Merge Risk: 
9226c
-
This PR adds an optional exact-arithmetic projection test, gated on MPFR and GMP, and exports three internal tables without changing library behavior. No actionable merge-blocking risk was found. The Linux CI results are still pending.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 51.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 4 files. (5 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title "Add exact-arithmetic test of the projection" matches the main change. The PR adds testExactProjectionExhaustive, which compares the projection code against a 256-bit MPFR reference. The tit… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The pull request description directly explains the new exact-arithmetic projection test, its MPFR and GMP dependencies, CI configuration, exported tables, test coverage, and reported results. These de… Full details: Title check
-
Explanation
The title "Add exact-arithmetic test of the projection" matches the main change. The PR adds testExactProjectionExhaustive, which compares the projection code against a 256-bit MPFR reference. The title is short, specific, and does not use vague terms. It omits the supporting build and export changes, which is acceptable.
Full details: Description check
-
Explanation
The pull request description directly explains the new exact-arithmetic projection test, its MPFR and GMP dependencies, CI configuration, exported tables, test coverage, and reported results. These details match the changeset.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 51.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 4 files. (5 skipped: 5 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
coverage: 99.758%. remained the same — justinhwang:exact-projection-test into uber:master |
Summary
Every floating-point test in H3 compares one implementation with another, or with values an implementation produced earlier. That shows agreement, not accuracy: both sides can be wrong in the same way. This adds
testExactProjectionExhaustive, which measures how far_faceIjkToVec3,cellToLatLngand every vertex ofcellToBoundaryland from the exact value of the formula they implement, using a 256-bit MPFR evaluation as the reference.The reference does not reproduce the transcendental calls. It evaluates the algebraic form of the same chain:
atan2thencos/sinon the azimuth becomesx/r,y/r; the Class III rotation and the face axis subtraction become angle-addition identities withsqrt(3/28),sqrt(25/28)andsin/cosof the twentyfaceAxesAzRadsCIIliterals;atanthencos/sinon the gnomonic radius becomes1/sqrt(1+r²),r/sqrt(1+r²). Everything else is rational arithmetic and square roots, which MPFR does correctly rounded.faceCenterPoint,faceAxesAzRadsCIIandRES0_U_GNOMONICare taken as exact definitions;M_SQRT3_2,M_RSQRT7,M_ONETHIRDare the exact numbers their constants approximate. Separation is the chord between the library's answer (converted back to a unit vector at full precision) and the exact unit vector, reported in degrees of arc, which is uniform over the sphere where a per-coordinate lat/lng comparison is not.The same test exists for the pure-Go port (uber/h3-go#155) with the same oracle; the sampled cells are chosen by the same rule so the two can be compared cell for cell.
What is covered
H3_EXACT_PROJECTION_RES=4raises the full enumeration to res 4 (335k cells, 2.0M vertices, ~13 s).H3_EXACT_PROJECTION_REPORT=1prints the worst case per resolution and a histogram in ulps instead of asserting._faceIjkToCellBoundary/_faceIjkPentToCellBoundarystep for step in the test, including the_v2dIntersectedge-crossing vertices, and assert the vertex count matchescellToBoundary. Nothing in the boundary code is refactored.Results (macOS arm64, Apple clang, after #1255)
Worst separation, one ulp of a unit-vector component = 1.3e-14°:
_faceIjkToVec3cellToLatLngcellToBoundaryverticesThe bound is set at 1e-13° (about 8 ulp), roughly twice the worst observed. Before #1255 the
cellToLatLngcolumn reached 6.9e-12° (541 ulp) at res 4 near the pole; this test is what found that.Linux (Debian bookworm, gcc 12.2, glibc 2.36, Docker)
Full enumeration to res 3, samples to 15. Worst separation over all outputs per platform:
The three libms land within half an ulp of each other at every resolution, so the cross-platform drift seen in #917 / #891 is each platform being 2–4 ulp from the exact value in slightly different directions, not one platform being wrong.
Build and CI
ENABLE_EXACT_PROJECTION_TESTdefaults to ON whenmpfr.h,libmpfrandlibgmpare found and OFF with a status message otherwise. Nothing else in the build touches MPFR.libmpfr-dev, macOS jobsbrew install mpfr. The clang sanitizer jobs and the 32-bit jobs set it OFF explicitly (libmpfr is not instrumented, and there is no 32-bit MPFR in the multilib install). Windows is unchanged (not found, so skipped).Exhaustivesuffix keeps it out oftest-fast, so the Valgrind job does not run it.Library change
faceCenterPoint,faceAxesAzRadsCIIandfaceNeighborsinfaceijk.closestaticand gainexterndeclarations infaceijk.h, in the same waybaseCells.hexports its tables, so the test reads the library's own definitions instead of a copy. No behaviour change.Test plan
ctest: 326/326 pass (Release, macOS arm64), including the new test🤖 Generated with Claude Code