Skip to content
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

MNT: matplotlib 3.5 compat #3670

Merged
merged 4 commits into from
Nov 19, 2021
Merged

Conversation

neutrinoceros
Copy link
Member

PR Summary

I'm fixing a couple error messages here as an excuse to trigger CI so that I can update the answer store.
This PR will be a blocker unless it turns out that no update is needed.

goal ? fix #3558
why now ? matplotlib 3.5.0 just came out. I'm hoping that my work with the release candidate was enough to make this update pretty transparent
but I still expect we're going to see some small changes in our test images, at a level that will bother CI but not humans 🤞🏻

@neutrinoceros neutrinoceros added blocker Highest priority level tests: running tests Issues with the test setup labels Nov 16, 2021
@neutrinoceros
Copy link
Member Author

As expected we're only getting insignificant differences, and only 2 of them I might add
Screenshot 2021-11-16 at 10 38 25

@neutrinoceros
Copy link
Member Author

Updates for the answer store are here:
yt-project/answer-store#27

regarding Jenkins, I'll wait for allowance to bump images everywhere needed. From what I've seen, every difference we're getting is equally insignificant so it shouldn't be controversial.

@neutrinoceros
Copy link
Member Author

adding "bug" label to satisfy the Mergeable bot.

I note that the prediction I gave in #3558 came true: symlog colorbars suddenly have minor ticks again, hurray !
Screenshot 2021-11-17 at 08 40 36

@neutrinoceros
Copy link
Member Author

Added a minimal patch to fix #3673 while I'm at it

@neutrinoceros neutrinoceros linked an issue Nov 17, 2021 that may be closed by this pull request
@@ -337,8 +336,7 @@ def _init_image(self, data, cbnorm, cblinthresh, cmap, extent, aspect):
self.cb.set_ticks(yticks)
else:
self.cb = self.figure.colorbar(self.image, self.cax)
for which in ["major", "minor"]:
self.cax.tick_params(which=which, axis="y", direction="in")
self.cax.tick_params(which="both", axis="y", direction="in")
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the explicit loop was necessary with old versions of matplotlib but it's not needed for the current oldest supported version (2.2.0)
see https://matplotlib.org/2.2.0/api/_as_gen/matplotlib.axes.Axes.tick_params.html?highlight=tick_params#matplotlib.axes.Axes.tick_params

Copy link
Member

Choose a reason for hiding this comment

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

OK, makes sense.

 - force inwards ticks for colorbar in PhasePlot
 - simplify calls to tick_params using modern matplotlib api
@matthewturk
Copy link
Member

So if I'm reading it right, now our answer tests mandate matplotlib 3.5, right?

matthewturk
matthewturk previously approved these changes Nov 17, 2021
Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

I don't see any reason this can't go in other than the minor comments about CI.

@@ -337,8 +336,7 @@ def _init_image(self, data, cbnorm, cblinthresh, cmap, extent, aspect):
self.cb.set_ticks(yticks)
else:
self.cb = self.figure.colorbar(self.image, self.cax)
for which in ["major", "minor"]:
self.cax.tick_params(which=which, axis="y", direction="in")
self.cax.tick_params(which="both", axis="y", direction="in")
Copy link
Member

Choose a reason for hiding this comment

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

OK, makes sense.

@neutrinoceros
Copy link
Member Author

So if I'm reading it right, now our answer tests mandate matplotlib 3.5, right?

not explicitly, but answer testing is always run against the latest available version of matplotlib, so it's ran against 3.5 since it came out yesterday

@matthewturk
Copy link
Member

Once we get the new answers in and respawn testing this should go in.

@neutrinoceros
Copy link
Member Author

Cool, I'm bumping baselines for Jenkins then.

@neutrinoceros
Copy link
Member Author

We need to merge yt-project/answer-store#27 as well so I can update here

@neutrinoceros
Copy link
Member Author

I forgot to bump one of the references, forced pushed. Now I expect images tests on Jenkins to pass

@neutrinoceros neutrinoceros removed the backport-yt-4.0.x on-merge: backport to yt-4.0.x label Nov 19, 2021
@neutrinoceros
Copy link
Member Author

removing the "backport" label since its purpose is now covered by having the PR triaged in the 4.0.2 milestone already.

@matthewturk matthewturk merged commit 62b6458 into yt-project:main Nov 19, 2021
@lumberbot-app
Copy link

lumberbot-app bot commented Nov 19, 2021

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout yt-4.0.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -m1 62b645889ae0eb27171ef310dae03069e4181e46
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #3670: MNT: matplotlib 3.5 compat'
  1. Push to a named branch:
git push YOURFORK yt-4.0.x:auto-backport-of-pr-3670-on-yt-4.0.x
  1. Create a PR against branch yt-4.0.x, I would have named this PR:

"Backport PR #3670 on branch yt-4.0.x (MNT: matplotlib 3.5 compat)"

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 Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@neutrinoceros
Copy link
Member Author

Ah, turns out this can't be back ported (not automatically OR manually) unless we backport #3441 as well. This is because local_pw tests were modified when switching to cmyt as a dependency. Now it seems to me that there are two ways we can keep CI green on the backport branch:

After writing this I don't think there's a way around it so I'll just manually backport #3441 + this, hopefully this is acceptable.

neutrinoceros pushed a commit to neutrinoceros/yt that referenced this pull request Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Highest priority level bug tests: running tests Issues with the test setup viz: 2D
Projects
None yet
3 participants