Skip to content

Conversation

@HalfWhitt
Copy link
Member

@HalfWhitt HalfWhitt commented Dec 24, 2025

Fixes #4005; I've been nerd-sniped. Uploading this with threshold values that work on macOS; presumably at least a couple will need bumping up a little depending on test failures.

Also removes the unused assert_pixel.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@HalfWhitt HalfWhitt marked this pull request as ready for review December 24, 2025 08:50
@HalfWhitt
Copy link
Member Author

HalfWhitt commented Dec 24, 2025

First of all, I know most of the core team's on holiday leave, so I'm aware this might not get reviewed for a while.

@corranwebster, I'd appreciate a double-check on my math. I've changed how it's written out Python-wise, but math-wise the only differences should be, per your suggestions:

  • Dividing by the four bands
  • Converting the images to RGBa to test premultiplied alpha

Evidently the premultiplying helps a lot in some cases. After all, dividing by bands should, alone, only halve the error amounts... but the threshold for the test_multiline_text was able to drop all the way from 0.09 to 0.01, and test_transparency from 0.1 to 0.01.

test_write_text (which uses a variety of fonts, not just system as in test_multiline_text) only dropped from 0.07 to the expected 0.035, but it's interesting that Gtk on Wayland was the only one to go above 0.02.

Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

Looks good, as a non-maintainer.

@HalfWhitt
Copy link
Member Author

Looks good, as a non-maintainer.

Maintainer or not, I very much appreciate your canvas-and-image-related expertise so far!

Copy link
Contributor

@johnzhou721 johnzhou721 left a comment

Choose a reason for hiding this comment

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

Nice use of ImageChops.difference!

@HalfWhitt
Copy link
Member Author

Nice use of ImageChops.difference!

Credit to corranwebster, they're the one who suggested it 👍

@kattni
Copy link
Contributor

kattni commented Dec 27, 2025

@HalfWhitt Please ping me when this is ready for review. Thanks!

@HalfWhitt
Copy link
Member Author

@kattni Ping : )

@HalfWhitt
Copy link
Member Author

(This is still ready to go, I just decided to arrange it slightly differently.)

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Can't argue with much here - ImageChops.difference() is a new one for me, but I'm very much in favor of adopting something that is a pre-existing difference measure than inventing one. And, if it means our error thresholds are lower, all the better.

@freakboy3742 freakboy3742 merged commit 88c6a48 into beeware:main Dec 29, 2025
56 checks passed
@HalfWhitt HalfWhitt deleted the rmse branch December 29, 2025 06:51
@HalfWhitt
Copy link
Member Author

HalfWhitt commented Dec 29, 2025

ImageChops.difference() is a new one for me, but I'm very much in favor of adopting something that is a pre-existing difference measure than inventing one. And, if it means our error thresholds are lower, all the better.

Just for the record, ImageChops.difference is only a tidier way of doing the same math as what I initially submitted:

        total = sum(
            ((actual - expected) / 255) ** 2
            for actual, expected in zip(
                chain(*scaled_image.convert("RGBa").getdata()),
                chain(*reference_image.convert("RGBa").getdata()),
                strict=True,
            )
        )

It's not relevant to lowering the error; that comes entirely from (1) dividing by the number of bands, and (2) premultiplying the alpha, so we're not testing invisible differences. (Both of which were corranwebster's idea on the original ticket).

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.

Canvas Image Comparison RMSE Calculation Incorrect

5 participants