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

BUG: fix default math font in yt plots against matplotlib >= 3.4 #3520

Merged
merged 3 commits into from Oct 8, 2021

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Sep 17, 2021

PR Summary

Endgoal: resolve #3514
Basic testing seems to indicate this works, however I need to further check a few things. Is it still possible to change the math font using the matplotlib API ?
Submitting as a draft for now so I can at least check if image comparison tests pass as is.

Note: this reverts a temporary patch in tests/matplotlibrc from #3153.

PR Checklist

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

@neutrinoceros
Copy link
Member Author

neutrinoceros commented Sep 17, 2021

test cases to add:

import matplotlib as mpl
from yt.testing import fake_amr_ds
from yt.visualization.api import SlicePlot

ds = fake_amr_ds(fields=[("gas", "density")], units=["g/cm**-3"])
sp = SlicePlot(ds, "z", ("gas", "density"))

Expected result

3 4 3_Slice_z_density

# this case requires matplotlib 3.4 or greater
sp = ySlicePlot(ds, "z", ("gas", "density"))
sp.set_font({"math_fontfamily": "dejavusans"})

# same result is expected here 
with mpl.rc_context({"mathtext.fontset": "dejavusans"}):
    sp = yt.SlicePlot(ds, "z", ("gas", "density"))

Expected result
3 4 3_mode_Slice_z_density

Note that this last case currently only works with matplotlib 3.4 + yt's main branch. It doesn't work with matplotlib 3.3 or earlier or with the current state of this PR, so I'm not 100% sure we need to support it, but it would be nice if we could avoid closing a door that matplotlib just opened for us.

@neutrinoceros
Copy link
Member Author

neutrinoceros commented Sep 17, 2021

So the image tests revealed two kinds of failures:

  • line plots seem to not be properly affected by this patch (acutal bug)
  • exponents in minor tick labels in colorbars for phase plots with logscale (weirdly specific...) are affected, but IMO they actually got better, so I think I'll propose to adopt this change

For illustration:

main branch

Screenshot 2021-09-17 at 18 46 00

this patch

Screenshot 2021-09-17 at 18 45 53

@neutrinoceros
Copy link
Member Author

neutrinoceros commented Sep 18, 2021

I'm now down to 9 errors (previously 41) for GH answer tests. All remaining errors are the kind I described in the previous comment, so I believe it' is mature enough to warrant bumping the answer store.
On the other hand, some of the failures on Jenkins at least are real: tests that check wether font parameters can be changed from the outside are showing that this feature is broken, so I still need to work on that. I'll open a PR to the answer store for starters, and focus on solving real issues on Jenkins before I can ask to bump this baseline too if needed.

@neutrinoceros neutrinoceros changed the title BUG: restore yt's classic math font even with matplotlib >= 3.4 BUG: fix default math font in yt plots against matplotlib >= 3.4 Sep 18, 2021
@neutrinoceros
Copy link
Member Author

Here's the PR to the answer store yt-project/answer-store#23
requesting early reviews from @matthewturk, @chummels, @Xarthisius regarding this point. Would it be okay to merge that PR before the present one is 100% finished or should I focus on getting things working on Jenkins first ?

@neutrinoceros
Copy link
Member Author

Alright I believe that all failures, GH and Jenkins alike, are the kind I propose to adopt. Opening this proposing to approvals before I can bump references.

@neutrinoceros
Copy link
Member Author

Note to reviewers: please consider reviewing this soon, the reports are not going to stay available for more than a couple days, after which we’d need to manually retrigger tests.

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.

lgtm

@neutrinoceros
Copy link
Member Author

@yt-fido test this please

@neutrinoceros
Copy link
Member Author

I got "no old answer" twice on Jenkins after bumping the answer number. Either I don't understand how it's supposed to be done anymore or something isn't working. @Xarthisius, can you have a look ?

@Xarthisius
Copy link
Member

@yt-fido test this please

@@ -1336,77 +1337,6 @@ def save(self, name=None, suffix=".png", mpl_kwargs=None):
self.plots[f].save(name, mpl_kwargs)
return names

@invalidate_plot
def set_font(self, font_dict=None):
Copy link
Member

Choose a reason for hiding this comment

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

Wait, so can we no longer manually set the font in our plots? I don't think i ever have, but it might be something people have used in the past.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it will still be possible, thanks to inheritance. Because I'm making ProfilePlot a subclass of PlotContainer, we don't need to have this method here, which was duplicated code from PlotContainer anyway :-)

@chummels
Copy link
Member

chummels commented Oct 4, 2021

OK, I'm a bit confused based on the description of this PR and the comments what this PR actually does (e.g., two "expected results" plots) Presumably this continues to use the old math font instead of the new MPL fonts for our plots. can you confirm?

@neutrinoceros
Copy link
Member Author

neutrinoceros commented Oct 5, 2021

Presumably this continues to use the old math font instead of the new MPL fonts for our plots. can you confirm?

Yes, this PR forces matplotlib to use the "cm" math font family, which was its default before matplotlib 3.4.0

A side effect of doing so, using matplotlib 3.4's api, is that negative exponents in tick labels are also forced to use this font, something we apparently were not doing in yt already, but arguably should have. This is the reason that I have a "bump answer store" PR attached :)

Copy link
Member

@chummels chummels left a comment

Choose a reason for hiding this comment

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

OK, looks good to me. Thanks!

yt/visualization/plot_container.py Show resolved Hide resolved
yt/visualization/plot_container.py Show resolved Hide resolved
@neutrinoceros
Copy link
Member Author

neutrinoceros commented Oct 7, 2021

Great, it's now down to a single error, same kind as the ones already bumped. Can I bump that one too ?

@Xarthisius
Copy link
Member

Sure, but be careful about numbering. local_cylindrical_background_008 already exists. Possibly created by some different PR

@neutrinoceros
Copy link
Member Author

alright, going straight to 009 then, thanks for the heads !

@neutrinoceros
Copy link
Member Author

Something went wrong but at least it doesn't look like I corrupted the database. Let's try bumping one last time.

@neutrinoceros
Copy link
Member Author

🎉 this is finally ready for merging

@cphyc cphyc merged commit c6caf13 into yt-project:main Oct 8, 2021
@neutrinoceros neutrinoceros deleted the default_mathfont branch October 8, 2021 12:55
Xarthisius added a commit to Xarthisius/yt that referenced this pull request Oct 11, 2021
@matthewturk
Copy link
Member

@meeseeksdev backport to yt-4.0.x

@lumberbot-app
Copy link

lumberbot-app bot commented Oct 15, 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 c6caf13b80c9495a6a8e8bb1ea6d5983ae8566e3
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #3520: BUG: fix default math font in yt plots against matplotlib >= 3.4'
  1. Push to a named branch :
git push YOURFORK yt-4.0.x:auto-backport-of-pr-3520-on-yt-4.0.x
  1. Create a PR against branch yt-4.0.x, I would have named this PR:

"Backport PR #3520 on branch yt-4.0.x (BUG: fix default math font in yt plots against matplotlib >= 3.4)"

And apply the correct labels and milestones.

Congratulation you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove Still Needs Manual Backport label once the PR gets merged.

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

@neutrinoceros
Copy link
Member Author

@meeseeksdev backport to yt-4.0.x

@lumberbot-app
Copy link

lumberbot-app bot commented Oct 16, 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 c6caf13b80c9495a6a8e8bb1ea6d5983ae8566e3
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #3520: BUG: fix default math font in yt plots against matplotlib >= 3.4'
  1. Push to a named branch :
git push YOURFORK yt-4.0.x:auto-backport-of-pr-3520-on-yt-4.0.x
  1. Create a PR against branch yt-4.0.x, I would have named this PR:

"Backport PR #3520 on branch yt-4.0.x (BUG: fix default math font in yt plots against matplotlib >= 3.4)"

And apply the correct labels and milestones.

Congratulation you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove Still Needs Manual Backport label once the PR gets merged.

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

@neutrinoceros
Copy link
Member Author

@meeseeksdev backport to yt-4.0.x

meeseeksmachine pushed a commit to meeseeksmachine/yt that referenced this pull request Oct 16, 2021
neutrinoceros added a commit that referenced this pull request Oct 16, 2021
…0-on-yt-4.0.x

Backport PR #3520 on branch yt-4.0.x (BUG: fix default math font in yt plots against matplotlib >= 3.4)
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.

Change of style in matplotlib
5 participants