sass / sass-spec

Official Sass Spec Suite
MIT License
200 stars 89 forks source link

Increase test result precision #2024

Closed ntkme closed 1 month ago

ntkme commented 1 month ago

Currently the js-api-spec runs at very low precision at epsilon = (10 ** -2), so things are passing now.

https://github.com/sass/sass-spec/blob/3bb39e6df44c025fc878ec35cfee45faca815afe/js-api-spec/setup.ts#L358-L363

However, when I change the epsilon to 10 ** -11 (the value sass uses internally), I got 6 test failures because the "expected" result does not have enough precision.

nex3 commented 1 month ago

The primary reason for this comparison being so low, as the comment indicates, is that the Color.js API which the embedded host uses tends to accumulate more errors than the Dart color API (I think mostly because the Dart API uses more precise transformation matrices). Have you verified that that's no longer the case?

ntkme commented 1 month ago

Have you verified that that's no longer the case?

It is still not consistent. Thus in this PR I'm not trying to really increase the actual the precision used for testing.

I noticed some matrices in w3 draft examples and color.js got updated and have different values than dart-sass: For example https://github.com/color-js/color.js/blob/40e7a059c639bafde14504627e62791588c63100/src/spaces/oklab.js#L10-L24

@nex3 Probably worth cross checking all matrices and see if they are accurate.

ntkme commented 1 month ago

BTW, https://github.com/w3c/csswg-drafts/issues/6642#issuecomment-943521484 might explain what's happening with the precision issue here: https://github.com/sass/sass-spec/blob/83bd499823036a81994f319a0a6fd7714b43d64e/js-api-spec/value/color/color-4-conversions.test.ts#L164-L168

ntkme commented 1 month ago

I doubt we can fix this until Color.js is also using all the latest matrices (if it ever does).

If I read the comment from the linked color.js code correctly I think they are already using updated matrices, and dart-sass being outdated might be why the results were off between the two, but maybe there will be other differences. Hard to know as the way color conversion path works is quite different.

nex3 commented 1 month ago

Hmm... the matrices added in https://github.com/color-js/color.js/pull/357 match the CSS spec, but they're different from those described in https://github.com/w3c/csswg-drafts/issues/6642#issuecomment-1490068959, which the comments on that issue seem to indicate are the best currently available. I've asked for clarification in https://github.com/w3c/csswg-drafts/issues/6642#issuecomment-2389764762.

ntkme commented 1 month ago

One good-ish news is that after I updated the matrices in ruby implementation (a port of dart implementation) now I can reproduce the issue where the color conversion tests pass at precision of 4 digits, but fails at precision of 5 digits. So the LMS matrices might have been the root cause of the precision difference. Next thing to try is re-baseline all color conversion tests, and validate them against dart-sass vs color.js and see if they pass at higher precision.

ntkme commented 1 month ago

@nex3 I spent sometime re-baselining all the outputs with embedded-host-node implementation on color.js 0.5.2 - and now when running at epsilon = 10 ** -11 on dart-sass, we still see failed tests and all of them involves oklch and oklab - might be something we want to take a look.

nex3 commented 1 month ago

It's generally expected that we differ slightly from Color.js, because we're generally using more precise color transform inputs (especially for oklch and oklab, where we computed many steps losslessly).

ntkme commented 1 month ago

Never mind, I forgot to update my dart-sass build.

ntkme commented 1 month ago

Tests are now passing at epsilon = 10 ** -11.