BUG: fix orientation of colorbar in multiplanel plot

[xshaokun]

PR Summary

fix #4382

PR Checklist

• New features are documented, with docstrings and narrative docs
• Adds a test for any bugs fixed. Adds tests for new features.

[welcome]

Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution!

[neutrinoceros]

Great !
Ideally we need an image test for this, here's the guide https://yt-project.org/docs/dev/developing/testing.html#creating-and-updating-image-baselines-for-pytest-mpl-tests

[chrishavlin]

The minimal py3.9.2 test build is failing because it uses yt's minimally supported matplotlib (3.5.0), for which colorbar does not have a location kwarg. Looks like the location kwarg was introduced in matplotlib 3.7.0(release notes here). IMO it's OK for this bug to persist in yt with the minimally supported matplotlib, so the easiest thing to do is just add a check for matplotlib version before passing in a location kwarg.

[neutrinoceros]

Agreed. Note that matplotlib natively supports runtime version checking as

import maptlotlib as MPL
if mpl.__version_info__ >= (3, 7):
    ...
else:
    ...

[neutrinoceros]

Maybe we could even emit a warning if we can detect that the colorbar is horizontal and matplotlib is too old.

[xshaokun]

I have a question regarding writing a test. Since the bug persists for older versions of MPL and a warning is emitted, the same test will generate different baseline images in new and old versions of MPL, but they will have the same file name. This prevents me from updating the reference image into pytest_mpl_baseline. Do you have any better suggestions for handling this?

[neutrinoceros]

Yes, image tests are tricky an hardly portable; in fact we only run them on a specific runner image (ubuntu-latest) and against the latest version of matplotlib. You shouldn't (and cannot) generate a new baseline image on your own machine, as it might not exactly match what is produced in CI, as previous experiments showed that the only diff tolerance we can support is 0.

Good job on adding a @pytest.mark.skipif marker anyway, this is helpful documentation of how much tension we have to bump our requirement on matplotlib !

[xshaokun]

in fact we only run them on a specific runner image (ubuntu-latest) and against the latest version of matplotlib.

That makes it easier! In fact, I added @pytest.mark.skipif to avoid testing on platforms with a minimal version of matplotlib.

[neutrinoceros]

In fact, I added @pytest.mark.skipif to avoid testing on platforms with a minimal version of matplotlib.

Doesn't hurt !

[xshaokun]

@neutrinoceros Passing an empty keyword dict cb_kwargs = {} failed the type-checking in cf4e14d, but passed the test in c47732d before. Would you happen to know why?

Since for now, the workflow online for tests requires approval, I abandoned cb_kwargs in the latest commit to make the tests pass locally.

[neutrinoceros]

Passing an empty keyword dict cb_kwargs = {} failed the type-checking in cf4e14d, but passed the test in c47732d before. Would you happen to know why?

It's not obvious to me either (especially since I didn't get to see mypy's output), but it's not crucial, if mypy is happy with your current implementation that's good enough for me !

[neutrinoceros]

@xshaokun please revert commit ed399da : adding content to the submodule should be done as a separate pull request there (sorry the process isn't simpler ... it is documented though))

[xshaokun]

I have created the PR there and I thought something should also be done here.

[neutrinoceros]

Sorry I missed that. For future reference, this is yt-project/yt_pytest_mpl_baseline#6
And now that I see the images, I also see a problem: the font family used in colorbar labels doesn't seem to be properly set properly when the colorbar is horizontal. We need to figure this out and update your companion PR accordingly when we have a fix.

[xshaokun]

The current output is as follows, there are still some formatting issues, but it might be beyond this PR.

[neutrinoceros]

Great, the output looks good to me. I'll merge your companion PR on the submodule repo when it's up to date ! thank you for your patience !

[neutrinoceros]

Thanks a lot for your patience, I think this is almost ready for merge but here's a last round of review

[welcome]

Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆