Skip to content

MNT: Bump matplotlib minversion to 3.2 #14321

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
pllim merged 2 commits into from
Jan 30, 2023
Merged

MNT: Bump matplotlib minversion to 3.2 #14321

pllim merged 2 commits into from
Jan 30, 2023

Conversation

@pllim
Copy link
Member

@pllim pllim commented Jan 25, 2023 •
edited
Loading

Description

This pull request is to bump minversion of matplotlib to 3.2 and this able to remove the now unnecessary code added in

Fixes #8832

After merge

Add py39-test-image-mpl322-cov job to branch protection rule for main branch.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

@pllim pllim added this to the v5.3 milestone Jan 25, 2023
@pllim pllim requested a review from mhvk January 25, 2023 21:57
mhvk
mhvk reviewed Jan 25, 2023
View reviewed changes
Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good but CI is erroring.

astropy/visualization/tests/test_norm.py
if HAS_MATPLOTLIB:
import matplotlib

MATPLOTLIB_LT_32 = Version(matplotlib.__version__) < Version("3.2")
Copy link
Contributor

@mhvk mhvk Jan 25, 2023

Choose a reason for hiding this comment

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

Funny that this wasn't even used in the tests!

larrybradley
larrybradley approved these changes Jan 25, 2023
View reviewed changes
Copy link
Member

@larrybradley larrybradley left a comment

Choose a reason for hiding this comment

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

visualization changes look fine. Not sure why the mpl image test failed.

@pllim
Copy link
Member Author

pllim commented Jan 25, 2023

I think that is because there is no baseline for mpl322 config. But according to https://docs.astropy.org/en/latest/development/testguide.html#generating-reference-images , there is nothing I need to do? Things will just magically pass when this is merged?

@astrofrog
Copy link
Member

astrofrog commented Jan 25, 2023

I need to double check, I think we need to copy the hash file to this PR - will confirm tomorrow

@astrofrog
Copy link
Member

astrofrog commented Jan 26, 2023

@pllim - see #14329, you'll need to generate the hash library locally (or copy a previous one)

@pllim
Copy link
Member Author

pllim commented Jan 26, 2023

Does the failure log look normal? I cannot tell... https://app.circleci.com/pipelines/github/astropy/astropy/14299/workflows/3c0ffa75-a3a2-4f99-a213-0b6f9dcab803/jobs/108267

@pllim
Copy link
Member Author

pllim commented Jan 26, 2023

@astrofrog , you should look at the diff, some look suspicious to me... https://output.circle-artifacts.com/output/job/1f033492-7a62-4c8d-9720-f1c20e714941/artifacts/0/results/fig_comparison.html

@astrofrog
Copy link
Member

astrofrog commented Jan 28, 2023

The failures look ok to me, so I think you can safely update the hash file to the one you can download from the figure comparison page.

@pllim
MNT: Bump matplotlib minversion to 3.2
b58ca80 
and remove unnecessary code added in astropy#8818 and astropy#8782

Add new JSON for figure tests.
@pllim

This comment was marked as outdated.

Sign in to view
@pllim
Copy link
Member Author

pllim commented Jan 30, 2023

pre-commit.ci autofix

@pllim pllim merged commit 3510bc2 into astropy:main Jan 30, 2023
@pllim pllim deleted the bump-mpl-3.2 branch January 30, 2023 22:06
@pllim
Copy link
Member Author

pllim commented Jan 30, 2023

Thanks, all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhvk mhvk mhvk left review comments

@larrybradley larrybradley larrybradley approved these changes

@astrofrog astrofrog Awaiting requested review from astrofrog astrofrog is a code owner

@Cadair Cadair Awaiting requested review from Cadair

Assignees

No one assigned

Labels

installation testing visualization

Projects

None yet

Milestone

v5.3

Development

Successfully merging this pull request may close these issues.

MNT: Revert PR 8818 when minversion of matplotlib is 3.2

4 participants

@pllim @astrofrog @mhvk @larrybradley