Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
TST: Remove unnecessary test images#29827
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
If code coverage passes, that should be at least some indicator that we didn't drop anything relevant. |
OK, I've also removed |
Why prefer png over svg/pdf? A spot check of sizes looks like the pdf's are the smallest (presumably because they are compressed by default) and svgs are the most "text like" so should play the best with git. I was going to assert that the vector layouts would be insensative to freetype but began to doubt that as I was typing (I am not sure if we rely on freetype to do any placement even in those cases). The biggest upside I see of png is that it is no optional test-deps to run. |
Unfortunately not, or this wouldn't have come up in#29816. Admittedly, I don't think it'sall of them, though.
This is definitely the main reason, but I have no issues with changing to SVG/PDF if we think that's okay. |
I also suspect (but have not measured) that the png tests are faster as we don't effectively render 2x and require a sub-process to be running. Despite asking the question, I'm coming around to png being the right choice. |
Should we change the default extensions of check_figures_equal to only png? Figure comparisons often create the same artists but with different code paths. Therefore rendering to any output should be sufficient. I believe it's quite the exception that we need more than one output. |
Good idea, but it is in the public API right now. I seem to recall us saying that |
I'd be so bold and just change it, documenting it in an API change note. It should be "what users want" 99% of the time, and there's no simple deprecation/migration route. |
OK, I've changed the default and removed the extraneous |
| @@ -677,8 +677,7 @@ def test_contains_points(): | |||
| assert np.all(result == expected) | |||
| # Currently fails with pdf/svg, probably because some parts assume a dpi of 72. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we leave this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It looks like it does still fail on PDF/SVG, so we can leave it in.
Uh oh!
There was an error while loading.Please reload this page.
timhoffm commentedApr 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Not a 100% exact comparison but this test run is 9630 tests in 12:37min, where a test run on another recent PR is 9845 tests in 13:54min. So a rough estimate is that we have shaved off 200 tests and 1:20min test runtime. That's a significant improvement 🎉. |
This and#29816 will need to be backported to 3.10.x. |
lib/matplotlib/tests/test_figure.py Outdated
| @@ -25,7 +25,7 @@ | |||
| import matplotlib.dates as mdates | |||
| @image_comparison(['figure_align_labels'], extensions=['png', 'svg'], | |||
| @image_comparison(['figure_align_labels.png'], | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure, can revert. That commit also has some backcompat stuff that maybe could be removed in#29816, looks like...
There's no need to test multiple file formats for tests that:- are for wrappers that simplify or combine other primitives, such as `boxplot`, `eventplot`, `axhspan`, or `fill_between`.- are for spine or tick settings, such as the `autoscale_tiny_range`, `formatter_ticker_*`.- are for alternate Axes projections, such as polar, Mollweide, or skewed Axes.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
662239c intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free tosuggest an improvement. |
…3.10.xBackport PR#29827 on branch v3.10.x (TST: Remove unnecessary test images)
Uh oh!
There was an error while loading.Please reload this page.
PR summary
There's no need to test multiple file formats for tests that:
boxplot,eventplot,axhspan, orfill_between.autoscale_tiny_range,formatter_ticker_*.This is certainly not exhaustive; I did not remove any of the mathtext formats (seemed like a good glyph test), anything with hatching/clipping/alpha (probably could be pared down to just a few),
markevery tests (don't recall if that required anything in the backend),or tight layout tests.PR checklist