Skip to content

relax two test tolerances on x86_64 #16002

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 9, 2020
Merged

relax two test tolerances on x86_64 #16002

merged 2 commits into from
Mar 9, 2020

Conversation

nschloe
Copy link
Contributor

@nschloe nschloe commented Dec 22, 2019

When running the tests locally on x86_64, I had to relax two tolerances to the same value as aarch64.

This is on Ubuntu 20.04, so perhaps the reason why no one else has run into this before is an upgraded version in the dependency stack.

@nschloe nschloe mentioned this pull request Dec 22, 2019
Merged
6 tasks
@timhoffm
Copy link
Member

flake8 errors:

./lib/matplotlib/tests/test_usetex.py:15:80: E501 line too long (87 > 79 characters)
./lib/matplotlib/tests/test_backend_pgf.py:171:80: E501 line too long (87 > 79 characters)

timhoffm
timhoffm reviewed Dec 30, 2019
View reviewed changes
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

I'm a bit uneasy relaxing tolerances just because "then it works". If it worked before and does not work now, we should at least have a rough idea what changed. Can you provide the diff image of the failed tests?

OTOH this was done already for aarch64, so relaxing is maybe not too bad?

@nschloe
Copy link
Contributor Author

nschloe commented Dec 30, 2019

I'm a bit uneasy relaxing tolerances just because "then it works".

I'm with you there. When running the tests locally I noticed those tests are failing with the same tolerance as the aarch64 exceptions, so I threw that curve ball at you. I would guess that the exceptions aren't related to the architecture but the version of some dependency, perhaps a few layers down. Might be hard to figure out.

@timhoffm
Copy link
Member

timhoffm commented Jan 1, 2020

Can you provide the diff image of the failed tests? They should be in the folder result_images after running the test. That may give an indication what changed.

@anntzer
Copy link
Contributor

anntzer commented Jan 1, 2020

Also, do you still observe the need for this after rm -rf ~/.cache/matplotlib/test_cache?

@nschloe
Copy link
Contributor Author

nschloe commented Jan 1, 2020

Also, do you still observe the need for this after rm -rf ~/.cache/matplotlib/test_cache?

Yes.

Here are the plots for the failing lib/matplotlib/tests/test_backend_pgf.py test with the visual diff created with

compare image1 image2 -compose src diff.png

pgf_mixedmode-expected_pdf
pgf_mixedmode_pdf
diff2

And for lib/matplotlib/tests/test_usetex.py:

test_usetex
test_usetex-expected
diff

timhoffm
timhoffm reviewed Jan 1, 2020
View reviewed changes
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Thanks for the images! Both are subpixel positioning issues. I think it's ok to increase the tolerance in these cases without digging into the actual difference in the environment.

QuLogic
QuLogic approved these changes Mar 9, 2020
View reviewed changes
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

These fail for me too, but I never had time to investigate. So if people are okay with bumping the tolerance, I'm happy to see two less failures.

@timhoffm timhoffm merged commit 191ee22 into matplotlib:master Mar 9, 2020
@QuLogic QuLogic added this to the v3.3.0 milestone Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@nschloe @timhoffm @anntzer @QuLogic