Skip to content

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

Conversation

SidharthBansal
Copy link
Contributor

@SidharthBansal SidharthBansal commented Jun 29, 2020

PR Summary

  • Generated a matplotlib.tests wheel that can be uploaded to PyPI as a separate PyPI package ("distribution", in distutils parlance), to make it possible to install test data from PyPI.
    This is useful e.g. for mplcairo, whose test suite relies on matplotlib's one.
    Also, useful for Baseline image problem optimisation.
  • Created the test_data folder needed for additional test images apart from the baseline images
  • Created the logic for the generation of the baseline images on fresh install of matplotlib
  • In case, the baseline images exists exist and have changed then the logic for modification of the baseline images is created.
  • Creation of a plugin to generate images for both matplotlib and mpl_toolkits folders
  • Added related documentation.

Fixes #16447

attn: @anntzer

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@SidharthBansal SidharthBansal force-pushed the removal_of_baseline_images_from_baseline_package branch 2 times, most recently from 25eb8c5 to 678b778 Compare June 30, 2020 15:03
@SidharthBansal SidharthBansal force-pushed the removal_of_baseline_images_from_baseline_package branch 3 times, most recently from c5b1486 to a000051 Compare July 1, 2020 14:26
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"],
Copy link
Member

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.

stderr=subprocess.STDOUT,
stdout=subprocess.DEVNULL,
universal_newlines=True)
subprocess.run(["python", "-mpip", "install", "-e", mplRepoPath],
Copy link
Member

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?

stderr=subprocess.STDOUT,
stdout=subprocess.DEVNULL,
universal_newlines=True)
subprocess.run(["python", "-mpytest"],
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

@SidharthBansal SidharthBansal Jul 2, 2020

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 ?

@SidharthBansal SidharthBansal force-pushed the removal_of_baseline_images_from_baseline_package branch 5 times, most recently from 7a6aedd to c885729 Compare July 6, 2020 10:26
@SidharthBansal
Copy link
Contributor Author

Series of commands executed in the terminal:

python3 -mpytest lib/matplotlib/tests -m "baseline_image_generation_test"
mkdir lib/matplotlib/tests/baseline_images
cp -r result_images/test_agg lib/matplotlib/tests/baseline_images/
python3 -mpytest lib/matplotlib/tests -m "baseline_image_generation_test"

The first pytest command generates the baseline images with the baseline images not found error. Then, in the second pytest command, tests passes.
Similar approach for mpl_toolkits_tests need to be worked upon.

@SidharthBansal SidharthBansal force-pushed the removal_of_baseline_images_from_baseline_package branch 5 times, most recently from 631f056 to cd7dda9 Compare July 15, 2020 03:59
@SidharthBansal SidharthBansal force-pushed the removal_of_baseline_images_from_baseline_package branch 2 times, most recently from 15d0e90 to 6982bb1 Compare September 7, 2020 11:45

Baseline image generation
-------------------------
The idea is to eventually remove all of the images but we have options about where devs get the baselines from.
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@SidharthBansal SidharthBansal force-pushed the removal_of_baseline_images_from_baseline_package branch 3 times, most recently from f29ecf8 to c010250 Compare September 8, 2020 15:14
@SidharthBansal SidharthBansal force-pushed the removal_of_baseline_images_from_baseline_package branch 3 times, most recently from 430fc90 to 14f0c9a Compare September 9, 2020 08:40
@SidharthBansal SidharthBansal force-pushed the removal_of_baseline_images_from_baseline_package branch from 14f0c9a to 57bfa89 Compare September 9, 2020 11:27
@tacaswell tacaswell modified the milestones: v3.4.0, v3.5.0 Jan 27, 2021
@jklymak jklymak marked this pull request as draft May 8, 2021 20:44
@jklymak
Copy link
Member

jklymak commented May 8, 2021

@anntzer this seems like a good start, but is there any reason to keep it open?

@anntzer
Copy link
Contributor

anntzer commented May 8, 2021

Let's close this for now; we can always revisit...

@anntzer anntzer closed this May 8, 2021
@QuLogic QuLogic modified the milestones: v3.5.0, unassigned May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal for the baseline images problem
5 participants