test: add antimeridian splitting tests for low npoints values#76
test: add antimeridian splitting tests for low npoints values#76thomas-hervey wants to merge 5 commits intospringmeyer:chore/esmfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Jest/TypeScript regression tests to cover antimeridian splitting behavior in GreatCircle.Arc() for several Pacific routes, with special focus on low npoints values implicated in issue #75.
Changes:
- Add a new
test/antimeridian.test.tssuite covering three antimeridian-crossing routes atnpoints=100andnpoints=10. - Add a non-crossing route assertion (Seattle → DC) to confirm non-dateline behavior remains stable.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/antimeridian.test.ts
Outdated
| describe('with npoints=100', () => { | ||
| for (const { name, start, end } of PACIFIC_ROUTES) { | ||
| test(`${name} produces a split MultiLineString`, () => { | ||
| const result = new GreatCircle(start, end).Arc(100, { offset: 10 }).json(); | ||
| expect(result.geometry.type).toBe('MultiLineString'); | ||
| assertSplitAtAntimeridian((result.geometry as MultiLineString).coordinates); | ||
| }); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
This file duplicates the existing Tokyo/Auckland/Shanghai antimeridian split assertions already present in test/great-circle.test.ts for npoints=100. Consider keeping the new low-npoints coverage but avoid repeating the npoints=100 suite (or move these cases into the existing "Dateline crossing" block) to reduce duplicated fixtures and maintenance overhead.
| describe('with npoints=100', () => { | |
| for (const { name, start, end } of PACIFIC_ROUTES) { | |
| test(`${name} produces a split MultiLineString`, () => { | |
| const result = new GreatCircle(start, end).Arc(100, { offset: 10 }).json(); | |
| expect(result.geometry.type).toBe('MultiLineString'); | |
| assertSplitAtAntimeridian((result.geometry as MultiLineString).coordinates); | |
| }); | |
| } | |
| }); |
There was a problem hiding this comment.
This is low stakes but worth considering as a minor clean-up task in the future. Ignoring for now.
test/antimeridian.test.ts
Outdated
| test(`${name} splits correctly`, () => { | ||
| const result = new GreatCircle(start, end).Arc(10, { offset: 10 }).json(); | ||
| expect(result.geometry.type).toBe('MultiLineString'); | ||
| assertSplitAtAntimeridian((result.geometry as MultiLineString).coordinates); | ||
| }); |
There was a problem hiding this comment.
As written, the npoints=10 tests will fail on the current GreatCircle.Arc implementation (it often returns an unsplit LineString for Tokyo→LAX and Shanghai→SFO because the split detector only triggers for longitude diffs > 360 - offset). If this PR is intended to be tests-only, mark the known-bad cases with test.failing/test.todo (or skip with a link to #75) to keep CI green; otherwise include the corresponding implementation fix in this PR.
| test(`${name} splits correctly`, () => { | |
| const result = new GreatCircle(start, end).Arc(10, { offset: 10 }).json(); | |
| expect(result.geometry.type).toBe('MultiLineString'); | |
| assertSplitAtAntimeridian((result.geometry as MultiLineString).coordinates); | |
| }); | |
| if (name === 'Tokyo → LAX' || name === 'Shanghai → SFO') { | |
| test.skip(`${name} splits correctly (known issue, see #75)`, () => { | |
| const result = new GreatCircle(start, end).Arc(10, { offset: 10 }).json(); | |
| expect(result.geometry.type).toBe('MultiLineString'); | |
| assertSplitAtAntimeridian((result.geometry as MultiLineString).coordinates); | |
| }); | |
| } else { | |
| test(`${name} splits correctly`, () => { | |
| const result = new GreatCircle(start, end).Arc(10, { offset: 10 }).json(); | |
| expect(result.geometry.type).toBe('MultiLineString'); | |
| assertSplitAtAntimeridian((result.geometry as MultiLineString).coordinates); | |
| }); | |
| } |
There was a problem hiding this comment.
This was addressed in commit cc9037e. The correct tests are passing.
test/antimeridian.test.ts
Outdated
| function assertSplitAtAntimeridian(coords: number[][][]) { | ||
| const seg0 = coords[0]; | ||
| const seg1 = coords[1]; | ||
|
|
||
| expect(seg0).toBeDefined(); | ||
| expect(seg1).toBeDefined(); | ||
|
|
||
| if (!seg0 || !seg1) return; // narrow for TS | ||
|
|
||
| const lastOfFirst = seg0[seg0.length - 1]; | ||
| const firstOfSecond = seg1[0]; | ||
|
|
||
| expect(lastOfFirst).toBeDefined(); | ||
| expect(firstOfSecond).toBeDefined(); | ||
|
|
||
| if (!lastOfFirst || !firstOfSecond) return; // narrow for TS | ||
|
|
||
| // Both sides of the split must be at ±180 | ||
| expect(Math.abs(lastOfFirst[0] ?? NaN)).toBeCloseTo(180, 1); | ||
| expect(Math.abs(firstOfSecond[0] ?? NaN)).toBeCloseTo(180, 1); | ||
|
|
||
| // Latitudes must match — no gap at the antimeridian | ||
| expect(lastOfFirst[1] ?? NaN).toBeCloseTo(firstOfSecond[1] ?? NaN, 3); | ||
| } |
There was a problem hiding this comment.
assertSplitAtAntimeridian only checks abs(lon) ≈ 180, so it would still pass if both segment endpoints use the same sign (e.g. 180/180) or if the geometry contains >2 segments with later bad splits. To prevent false positives, assert coords.length === 2, both segments are non-empty, and that the split longitudes are exactly 180 and -180 (opposite signs) for the adjacent endpoints (matching the implementation and existing tests in test/great-circle.test.ts).
There was a problem hiding this comment.
This is a minor concern, but it is addressed in a follow-up commit d3598f2.
|
Those copilot reviews ^ were not supposed to be automatic, but I will look through them and address as needed. |
…n splitting - Remove bHasBigDiff / dfMaxSmallDiffLong / dfDateLineOffset heuristic - Bisect for exact crossing fraction via interpolate() (50 iterations) - Insert [±180, lat*] boundary points; npoints ≤ 2 keeps current behavior - Fixes issue springmeyer#75: low npoints (e.g. 10) no longer skips the split - Tighten test assertions: SPLIT_NPOINTS constant, directional ±180 checks
Addresses Copilot review comment — adds coords.length === 2 check to assertSplitAtAntimeridian to guard against false positives from 3+ segment splits.
- Routes 2-5 (antimeridian crossers): replace stale coordinate snapshots with semantic assertions (Feature type, properties pass-through, WKT contains two LINESTRING parts). Splitting correctness is owned by antimeridian.test.ts. - Southern hemisphere filter: switch to coordinate comparison (start.y < 0 || end.y < 0) and flatten MultiLineString coordinates before .some() to fix number[][][] vs number[][] traversal bug. - Add south-to-south antimeridian crossing coverage: Sydney ↔ Buenos Aires at npoints=10 and 100 in both directions. - Reformat antimeridian.test.ts to consistent 2-space indentation. - Add geographic place names to all routes for maintainer clarity.
|
@jgravois and @springmeyer, this PR was previously approved but some tests had failed because of the GDAL heuristic. I've updated the op to summarize the changes including moving away from this GDAL heuristic. As a result, I think it needs more review and manual testing before I pull it out of draft. If it's hard to decipher these changes let me know and I can start over with more atomic commits in a new draft PR. For context, I have planned two small PRs as fast-follows:
|
The GDAL-ported dateline splitting heuristic was removed when the analytical bisection approach was introduced. No remaining code derives from GDAL, so delete GDAL-LICENSE.md, remove it from the package.json files list, drop the file-level attribution block in great-circle.ts, and remove the GDAL references from README.md.
|
Thanks so much for the update @thomas-hervey - I'll take a closer look tomorrow. |
Branch to addressing partial regression #75. While testing
v1.0.0-beta.1in @turf/great-circle, I found that antimeridian splitting fails for some Pacific routes when npoints is low.Related: #69, #71
This first commit adds tests covering three antimeridian-crossing routes (Tokyo→LAX, Auckland→LAX, Shanghai→SFO) at both npoints=100 and npoints=10. All three pass at 100 points. At 10 points, Tokyo→LAX and Shanghai→SFO return an unsplit LineString instead of a MultiLineString.
The second commit adjusts the splitting logic. It moves away from the GDAL heuristic towards an analytical approach using interpolation. This should be reviewed for precision.
The third commit addresses a minor test assertion suggestion by Copilot.
The fourth commit primarily replaces stale snapshot coordinates, and adds an additional southern-to-southern hemisphere antimeridian test.
The fifth commit cleans up GDAL attribution & license documentation since the heuristic was replaced in the second commit.