Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase test result precision #2024

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Increase test result precision #2024

wants to merge 1 commit into from

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Sep 28, 2024

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

// The max distance two Sass numbers can be from each another before they're
// considered different (2 decimals).
//
// Uses ** instead of Math.pow() for constant folding.
// TODO: Ideally this should be more precise, but ColorJS does not always match.
const epsilon = 10 ** -2;

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.

@ntkme
Copy link
Contributor Author

ntkme commented Sep 28, 2024

Root cause: sass/sass#3953

@nex3
Copy link
Contributor

nex3 commented Oct 1, 2024

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
Copy link
Contributor Author

ntkme commented Oct 1, 2024

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
Copy link
Contributor Author

ntkme commented Oct 1, 2024

BTW, w3c/csswg-drafts#6642 (comment) might explain what's happening with the precision issue here:

// Certain channel values cause equality issues on 1-3 of 16*16*3
// cases. 0.45 is a magic number that works around this until the
// root cause is determined.
const scale = 0.45;
const channelValue = destinationSpace.ranges[index][1] * scale;

nex3 added a commit that referenced this pull request Oct 2, 2024
nex3 added a commit that referenced this pull request Oct 2, 2024
nex3 added a commit to sass/dart-sass that referenced this pull request Oct 2, 2024
This recomputes all existing LMS-related matrices based on the
improved XYZD65/LMS matrices in
w3c/csswg-drafts#6642#issuecomment-1490068959.

See sass/sass-spec#2024
Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

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

Increasing precision here sounds good, given that all the tests pass. However, it's probably worth merging the changes for below and making sure that the high-precision values are still correct, given that OK* space conversions are now subtly different than they were before.

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

Oh, thanks for the tip. I'm updating our matrices in sass/dart-sass#2374.

BTW, w3c/csswg-drafts#6642 (comment) might explain what's happening with the precision issue here:

// Certain channel values cause equality issues on 1-3 of 16*16*3
// cases. 0.45 is a magic number that works around this until the
// root cause is determined.
const scale = 0.45;
const channelValue = destinationSpace.ranges[index][1] * scale;

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

nex3 added a commit to sass/dart-sass that referenced this pull request Oct 2, 2024
This recomputes all existing LMS-related matrices based on the
improved XYZD65/LMS matrices in
w3c/csswg-drafts#6642#issuecomment-1490068959.

See sass/sass-spec#2024
@ntkme
Copy link
Contributor Author

ntkme commented Oct 2, 2024

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
Copy link
Contributor

nex3 commented Oct 2, 2024

Hmm... the matrices added in color-js/color.js#357 match the CSS spec, but they're different from those described in w3c/csswg-drafts#6642 (comment), which the comments on that issue seem to indicate are the best currently available. I've asked for clarification in w3c/csswg-drafts#6642 (comment).

nex3 added a commit that referenced this pull request Oct 2, 2024
@ntkme
Copy link
Contributor Author

ntkme commented Oct 3, 2024

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.

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