Skip to content

Conversation

@mfedderly
Copy link
Collaborator

@mfedderly mfedderly commented Dec 22, 2025

These are the changes to the tests required to pick up an alternate version of polyclip-ts that has been migrated from the bignumber usage to native javascript floats for perf.

It did seem to reintroduce some errors that were previously fixed, but I'm thinking these are down to the precision issues at the ~12th decimal place.

The test failures here are expected, as I merely built polyclip-ts locally and linked to it on my machine instead of going through the work to create a self-publishing npm package.

@turf/union benchmarks:

Before:
maximum-callstack-size-exceeded-2317 x 2.53 ops/sec ±1.52% (11 runs sampled)
not-overlapping x 14,076 ops/sec ±0.42% (99 runs sampled)
unable-to-complete-output-ring-1983-1 x 653 ops/sec ±0.54% (95 runs sampled)
unable-to-complete-output-ring-1983-2 x 297 ops/sec ±0.32% (90 runs sampled)
unable-to-find-segment-2258-1 x 1,512 ops/sec ±0.61% (97 runs sampled)
unable-to-find-segment-2258-2 x 49.03 ops/sec ±0.39% (65 runs sampled)
union1 x 6,552 ops/sec ±0.95% (97 runs sampled)
union2 x 1,298 ops/sec ±0.67% (97 runs sampled)
union3 x 3,980 ops/sec ±0.09% (101 runs sampled)
union4 x 4,860 ops/sec ±0.82% (99 runs sampled)

After:
maximum-callstack-size-exceeded-2317 x 19.74 ops/sec ±3.27% (38 runs sampled)
not-overlapping x 187,964 ops/sec ±0.24% (98 runs sampled)
unable-to-complete-output-ring-1983-1: 
unable-to-complete-output-ring-1983-2: 
unable-to-find-segment-2258-1 x 32,471 ops/sec ±0.15% (97 runs sampled)
unable-to-find-segment-2258-2 x 834 ops/sec ±0.31% (97 runs sampled)
union1 x 94,692 ops/sec ±0.15% (99 runs sampled)
union2 x 27,464 ops/sec ±0.23% (97 runs sampled)
union3 x 55,613 ops/sec ±0.22% (99 runs sampled)
union4 x 56,143 ops/sec ±0.55% (92 runs sampled)

So there's a massive speed improvement, but the unable-to-complete-output-ring tests are failing (sort of as expected).

These are the changes to the tests required to pick up an alternate version of polyclip-ts that
has been migrated from the bignumber usage to native javascript floats for perf.

It did seem to reintroduce some errors that were previously fixed, but I'm thinking these are
down to the precision issues at the ~12th decimal place
@mfedderly
Copy link
Collaborator Author

I confirmed that migrating those two skipped tests to 8 decimal places fixes the errors. According to a stackexchange post that's 1.1mm of precision, which seems about as far as any geo system can be pushed.

I don't think we should proactively truncate our inputs or outputs to a given precision (for speed purposes), but I think we should push back on anyone filing issues that only trigger past some relatively-arbitrary precision level.


if (process.env.REGEN)
writeJsonFileSync(directories.out + filename, result);
if (process.env.REGEN) writeJsonFileSync(directories.out + filename, fc);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was writing the wrong thing to disk (pre modification above), and so a REGEN actually broke all the tests.

@mfedderly
Copy link
Collaborator Author

I reserve the right to drop this PR and move from polyclip-ts to clipper2-ts if the @turf/buffer PR goes well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants