S2Cell test failure

[lukemckinstry]

What happened?

The following spec is failing in CI on the main branch.

It seems to have originated in this PR #13581 (review)
but the reason is not clear why as it did not seem to involve relevant code changes.

 Core/S2Cell
    ✗ gets correct center of cell
	Expected (-1691083.4101336685, 6039960.550535122, -1153592.7688487188) to equal epsilon (-1691083.4101336703, 6039960.550535122, -1153592.7688487188), 1e-15.
	    at <Jasmine>
	    at UserContext.<anonymous> (packages/engine/Specs/Core/S2CellSpec.js:294:62 <- Build/Specs/SpecList.js:157538:74)
	    at <Jasmine>

2.006 secs: Scene/Scene flyTo camera after morph to CV
1.74 secs: Core/createWorldBathymetryAsync resolves to CesiumTerrainProvider instance with default parameters
1.508 secs: Scene/Scene flyTo camera after morph to 2D
1.332 secs: Scene/Cesium3DTileset Restrict tileset memory usage with maximumCacheOverflowBytes
1.186 secs: Core/TaskProcessor works with a simple worker
1.02 secs: DataSources/KmlDataSource NetworkLink: onInterval
1.005 secs: Scene/Scene flyTo camera after morph to 3D
0.921 secs: Scene/GroundPrimitive larger scene renders batched instances
0.885 secs: Scene/GaussianSplat3DTileContent Load multiple instances of Gaussian splat tileset and validate transformed attributes
0.84 secs: DataSources/PolygonGeometryUpdater Computes dynamic geometry bounding sphere for fill.

Chrome Headless 150.0.0.0 (Linux 0.0.0): Executed 15614 of 15687 (1 FAILED) (skipped 73) (1 min 2.877 secs / 57.397 secs)
TOTAL: 1 FAILED, 15613 SUCCESS


1) gets correct center of cell
     Core/S2Cell
     Expected (-1691083.4101336685, 6039960.550535122, -1153592.7688487188) to equal epsilon (-1691083.4101336703, 6039960.550535122, -1153592.7688487188), 1e-15.
    at <Jasmine>
    at UserContext.<anonymous> (packages/engine/Specs/Core/S2CellSpec.js:294:62 <- Build/Specs/SpecList.js:157538:74)
    at <Jasmine>


TOTAL: 1 FAILED, 15613 SUCCESS
[14:32:55] 'test' errored after 1.35 min
[14:32:55] Error: 1
    at formatError (/home/runner/work/cesium/cesium/node_modules/gulp-cli/lib/versioned/^5.0.0/format-error.js:21:10)
    at Gulp.<anonymous> (/home/runner/work/cesium/cesium/node_modules/gulp-cli/lib/versioned/^5.0.0/log/events.js:33:54)
    at Gulp.emit (node:events:531:35)
    at Gulp.emit (node:domain:489:12)
    at Object.error (/home/runner/work/cesium/cesium/node_modules/undertaker/lib/helpers/createExtensions.js:61:10)
    at handler (/home/runner/work/cesium/cesium/node_modules/now-and-later/lib/map.js:68:20)
    at f (/home/runner/work/cesium/cesium/node_modules/once/once.js:25:25)
    at f (/home/runner/work/cesium/cesium/node_modules/once/once.js:25:25)
    at tryCatch (/home/runner/work/cesium/cesium/node_modules/async-done/index.js:21:15)
    at done (/home/runner/work/cesium/cesium/node_modules/async-done/index.js:37:12)
Error: Process completed with exit code 1.

Reproduction steps

1. Occurring in CI on main branch

...

Sandcastle example

No response

Environment

Browser:
CesiumJS Version:
Operating System:

AI acknowledgment

• I used AI to generate this issue report.
• (If the above is checked) I have reviewed the AI-generated content before submitting.

[javagl]

This triggered my nerd-senses.

I tried it out with CesiumJS versions back to 1.141, and it kept happening. So it's ~"not a CesiumJS issue", in some way. This was confirmed by checking that the test fails in Chrome, but passes in FireFox.

Some debugstepping into the S2Cell.getCenter() call led to Cartographic.fromCartesian, where the difference showed up: Left is FireFox, right is Chrome:

The longitude is different, even though it is computed with Math.atan2 from the same values in both cases. So I'm placing my bets on this being caused by the first Chrom(e/ium) version being rolled out that contains https://chromium.googlesource.com/v8/v8/+/43f04a52f24dab40d3d9ff7aa4849e70a0d10225

What does that mean for CesiumJS? Well. You're gonna need a bigger boat epsilon 𓀠

[lukemckinstry]

Thanks for identifying the likely cause. The version of Chrome headless is set by the GitHub Actions runner image, not CesiumJS. And it looks like this test started failing when it upgraded from 149 to 150.

I think it is the case that the new implementation of Math.atan & Math.atan2 using libm is actually more accurate than the previous using fdlibm, which means we are testing against an outdated and less accurate benchmark.

I think using a bigger epsilon as proposed in #13621 is the right short term fix to fully unblock CI.

But I think we should leave this issue open to consider if there is a benefit to testing to setting back to epsilon15 and updating the benchmark to reflect the now more accurate calculation.

[lukemckinstry]

Keeping open to consider if we should reset the benchmarks and measure with epsilon15 again to reflect more accurate Math.atan & Math.atan2 in Chrome

[javagl]

OK to keep open. While I'm reasonably sure that the sudden failure was related to the atan2 change in Chrome/Chromium, the linked PR says

Implement Math.{atan,atan2}() using LLVM's libm
...
Note that LLVM-libm's atan / atan2 isn't exact (yet?):
llvm/llvm-project#197072

This sounds like this might still change by roughly an ULP sooner or later. But I won't claim to be 100% sure whether FireFox or Chrome provides the "correct" result right now. It might even be that IEEE754 does only define the results for "special values", and not the exact results down to the last digit...