Repository navigation
Fix precision loss in cellAreaRads2 for cells crossing the antimeridian - #1274
Open
thevar1able wants to merge 2 commits into
Open
thevar1able wants to merge 2 commits into
thevar1able wants to merge 2 commits into
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe area calculation now adjusts longitude differences for edges that cross the antimeridian. New tests compare loop and cell areas across the antimeridian and after rotating cell boundaries. ChangesArea calculation precision
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely identifies the main change: fixing precision loss in cellAreaRads2 for antimeridian-crossing cells. Description check The description directly explains the precision issue, the implementation approach, affected APIs, regression tests, and expected results. 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.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files. (1 skipped: 1 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.
thevar1able
force-pushed
the
fix-cagnoli-antimeridian
branch
from
October 19, 2026 16:27
2194649 to
94b48e8
Compare
|
|
isaacbrodsky
requested review from
ajfriend,
dfellis,
dmitryzv,
isaacbrodsky,
jongbinjung and
nrabinowitz
October 19, 2026 19:07
isaacbrodsky
approved these changes
Oct 19, 2026
|
coverage: 99.758%. remained the same — thevar1able:fix-cagnoli-antimeridian into uber:master |
5 tasks done
justinhwang
added a commit
to uber/h3-go
that referenced
this pull request
Oct 21, 2026
cagnoli subtracted the raw longitudes of an edge's endpoints. For an edge crossing +-180 degrees the difference is close to +-360 and the true, tiny longitude step is lost to the rounding of a value that large, so cells straddling the antimeridian came out too small: 1e-8 relative at res 7, 0.1% at res 13 and 9% at res 15. Shifting both longitudes to the same side of the antimeridian before subtracting keeps the subtraction between two nearby values; sin and cos are 2pi-periodic, so nothing else changes. This is upstream uber/h3#1274, which the pinned release does not yet contain. The conformance suite could not see the defect: its seeded sample has straddling cells only at resolutions 0 and 1, where the loss is far below tolerance. The generator now carries the upstream change as a second patch and writes floats/curated.jsonl, the upstream test pairs at res 13 to 15 plus the cell containing (lat, 180) at every resolution for three latitudes, which the floats/cells checks run. The fifteen straddling res 0 and 1 cells already in floats/cells.jsonl move by at most 4e-14 relative; every other file, the digests included, is byte for byte unchanged. Three tests port the upstream ones: straddling cells agree with their non-straddling neighbours, a cell on the antimeridian keeps its area when rotated onto the prime meridian, and a small triangle across +-180 has the area of the same triangle across 0. All three fail without the shift. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
justinhwang
added a commit
to uber/h3-go
that referenced
this pull request
Oct 21, 2026
cagnoli subtracted the raw longitudes of an edge's endpoints. For an edge crossing +-180 degrees the difference is close to +-360 and the true, tiny longitude step is lost to the rounding of a value that large, so cells straddling the antimeridian came out too small: 1e-8 relative at res 7, 0.1% at res 13 and 9% at res 15. Shifting both longitudes to the same side of the antimeridian before subtracting keeps the subtraction between two nearby values; sin and cos are 2pi-periodic, so nothing else changes. This is upstream uber/h3#1274, which the pinned release does not yet contain. The conformance suite could not see the defect: its seeded sample has straddling cells only at resolutions 0 and 1, where the loss is far below tolerance. The generator now carries the upstream change as a second patch and writes floats/curated.jsonl, the upstream test pairs at res 13 to 15 plus the cell containing (lat, 180) at every resolution for three latitudes, which the floats/cells checks run. The fifteen straddling res 0 and 1 cells already in floats/cells.jsonl move by at most 4e-14 relative; every other file, the digests included, is byte for byte unchanged. Three tests port the upstream ones: straddling cells agree with their non-straddling neighbours, a cell on the antimeridian keeps its area when rotated onto the prime meridian, and a small triangle across +-180 has the area of the same triangle across 0. All three fail without the shift. Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
justinhwang
approved these changes
Oct 26, 2026
This branch has not been deployed
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
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.
Since #1101,
cellAreaRads2adds up a term per cell edge (cagnoliinarea.c). For an edge that crosses ±180°, the raw longitude difference is close to ±2π. The real gap between the two vertices is tiny, and at that magnitude it gets lost to floating-point rounding beforesinandcosare applied. Nothing cancels the error, so cells that straddle the antimeridian come out too small. Compared with a neighbouring cell, a straddling cell is about 0.07% too small at res 13, 0.4% at res 14 and 2.5% at res 15. This also affectscellAreaKm2andcellAreaM2, andgeoLoopAreaRads2and the other polygon area helpers. Releases before #1101 used a different formula and don't have this problem.The fix shifts both longitudes to the same side of the antimeridian before subtracting, so the subtraction is between two nearby values.
sinandcosrepeat every 2π, so the result is mathematically the same. Edges that don't cross the antimeridian take the old path unchanged.Tests:
cell_area_antimeridian: three pairs of cells at res 13–15, each a straddling cell next to one that doesn't straddle. Their areas must agree to within 1e-5.cell_area_antimeridian_rotation: at every resolution, cells along ±180° must keep their area when their boundary is shifted 180° in longitude, to within 1e-6. Shifting around the polar axis can't change an area, so this needs no reference values. The worst result on fixed code is 3e-8 at res 15; unfixed code is off by up to 7.6e-2.antimeridianTriangle: a small triangle across ±180° must have the same area as the same triangle across 0°. This uses a triangle and not a rectangle on purpose: a rectangle's two crossing edges have identical longitude differences, so their rounding errors cancel and the test passes even without the fix.Each new test fails without the fix. The existing exhaustive area tests still pass. They only go up to res 4, where the error is too small for them to see, which is why nothing caught this.
This was found and fixed in ClickHouse's fork (ClickHouse#13, reported in ClickHouse/ClickHouse#124447). This PR ports that fix and adds the rotation and triangle tests.
Related: #1101, #1108