-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Removal of baseline images from matplotlib_baseline_images package #17793
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
Removal of baseline images from matplotlib_baseline_images package #17793
Conversation
25eb8c5
to
678b778
Compare
c5b1486
to
a000051
Compare
lib/matplotlib/testing/decorators.py
Outdated
stdout=subprocess.DEVNULL), | ||
universal_newlines=True) | ||
# Clone mpl repo to tmpvenv and run pytests from new mpl repo created | ||
subprocess.run(["git", "clone", str(mplRepoPath), "tmp/testenv"], |
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.
What version of git do we get worktree
? I suspect that would be both faster and small footprint.
lib/matplotlib/testing/decorators.py
Outdated
stderr=subprocess.STDOUT, | ||
stdout=subprocess.DEVNULL, | ||
universal_newlines=True) | ||
subprocess.run(["python", "-mpip", "install", "-e", mplRepoPath], |
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 this be the new venv python?
lib/matplotlib/testing/decorators.py
Outdated
stderr=subprocess.STDOUT, | ||
stdout=subprocess.DEVNULL, | ||
universal_newlines=True) | ||
subprocess.run(["python", "-mpytest"], |
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 also install pytest and all of the extra dependencies (pandas, ...) to make sure we generate all of the images or should we only match what the user has installed in their development environment?
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.
I think installing all extra dependencies is a better option as testing aims to check integrity amongst the components too, which can't be achieved if only some of the installs are done.
First time all the images will be generated, later on we can move on specific missing installs and modifications of the baseline images.
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.
What do you think @anntzer ?
7a6aedd
to
c885729
Compare
Series of commands executed in the terminal:
The first pytest command generates the baseline images with the |
631f056
to
cd7dda9
Compare
15d0e90
to
6982bb1
Compare
doc/devel/contributing.rst
Outdated
|
||
Baseline image generation | ||
------------------------- | ||
The idea is to eventually remove all of the images but we have options about where devs get the baselines from. |
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.
This needs a paragraph explaining the current state of things to the reader. To us (who have been thinking about this all summer), this makes perfect sense, but to a new contributor who does not even know we have baseline images checked into the repo this is going to be consufing.
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.
Done
f29ecf8
to
c010250
Compare
…ectory and matplotlib directory
430fc90
to
14f0c9a
Compare
14f0c9a
to
57bfa89
Compare
@anntzer this seems like a good start, but is there any reason to keep it open? |
Let's close this for now; we can always revisit... |
PR Summary
This is useful e.g. for mplcairo, whose test suite relies on matplotlib's one.
Also, useful for Baseline image problem optimisation.
test_data
folder needed for additional test images apart from the baseline imagesmatplotlib
andmpl_toolkits
foldersFixes #16447
attn: @anntzer
PR Checklist