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

Remove font warning when legend is added while using Tex #17521

Merged
merged 2 commits into from
Sep 20, 2020

Conversation

Struan-Murray
Copy link
Contributor

@Struan-Murray Struan-Murray commented May 27, 2020

PR Summary

Addresses issue #17518 regarding a warning being shown when legend is added while using Tex. Code by @anntzer.

PR Checklist

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

QuLogic
QuLogic previously approved these changes May 27, 2020
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Is there a way we can test this?

@Struan-Murray
Copy link
Contributor Author

Struan-Murray commented May 28, 2020

I can confirm that after building from source, with diff

git diff master e17518 
diff --git a/lib/matplotlib/offsetbox.py b/lib/matplotlib/offsetbox.py
index 7bcf3f3de..60ddf06ef 100644
--- a/lib/matplotlib/offsetbox.py
+++ b/lib/matplotlib/offsetbox.py
@@ -876,7 +876,8 @@ class TextArea(OffsetBox):
 
     def get_extent(self, renderer):
         _, h_, d_ = renderer.get_text_width_height_descent(
-            "lp", self._text._fontproperties, ismath=False)
+            "lp", self._text._fontproperties,
+            ismath="TeX" if self._text.get_usetex() else False)
 
         bbox, info, d = self._text._get_layout(renderer)
         w, h = bbox.width, bbox.height

and confirming that I have moved on from matplotlib 3.2.1:

Python 3.8.2 (default, Apr 27 2020, 15:53:34) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import matplotlib
>>> matplotlib.__version__
'3.2.1.post2648+g80935b1f9'

That the MWE:

from matplotlib import rc
import matplotlib.pyplot as plt

rc('font', **{'family':'serif','serif':'Computer Modern'})
rc('text', usetex=True)

plt.figure()
plt.plot(0,0,label='Input')
plt.title('Commenting out legend removes warning')
plt.text(0, 0, 'Text test')
plt.xlabel('Label Test')
plt.xticks(ticks=[-1,1],labels=['TestA','$MATH_{\pi}$'])

plt.legend()

plt.show()

now does not give an error while using plt.legend().

@QuLogic
Copy link
Member

QuLogic commented May 28, 2020

I mean an automated test.

@Struan-Murray
Copy link
Contributor Author

I don't know how to do that, where should I start?

@jklymak
Copy link
Member

jklymak commented May 28, 2020

Hi @Struan-Murray There are instructions in the developer guide. But really, just do this if you are interested a) not sure that a test is easy for this and b) its a bit of a process if you are just trying to get a bug squashed...

@Struan-Murray
Copy link
Contributor Author

Hey @jklymak, I'm reading up on pytest just now in the developer guide, I assume this is what you meant unless it's the online tests further down, let me know. I just started using matplotlib a few days ago (it's working great so far, thanks!) so I might be a bit out of my depth. I'm understanding that I would need to try find a way to assert that the code does not throw a warning when the second argument (ismath) is set to "TeX" as opposed to "False" where it does, I don't know how to do this within a test environment, yet. From my side the bug is squashed, I'll probably continue reading up on testing but might just observe any further changes as it seems a little too complicated for what was just a minor annoyance. Thanks for the help.

@jklymak
Copy link
Member

jklymak commented May 29, 2020

Well the nice thing about warnings is that they lead to a failed test, so you could just add your routine above (boil it down as simple as you can) to the end of lib/matplotlib/tests/test_legend.py; no need to assert anything. The only gotchya is that I think you need to put a bad font in?

@Struan-Murray
Copy link
Contributor Author

Smaller MWE:

from matplotlib import rc
import matplotlib.pyplot as plt

rc('font', **{'family':'serif','serif':'Computer Modern'})
rc('text', usetex=True)

plt.figure()
plt.plot(0,0,label='Input')
plt.legend()
plt.show()

So as I understand it, this test should give a warning to show that it will still alert the user that an incorrect font has been selected? I see what you mean, I'll have to find a way to select a font that is guaranteed to not be installed to show that it still throws a warning code, maybe I'll just put some random characters in. Either way it's late here so I'll work on it later.

@jklymak jklymak marked this pull request as draft August 24, 2020 15:05
@jklymak
Copy link
Member

jklymak commented Aug 24, 2020

ping @Struan-Murray thanks for working on this - any progress on an automated test?

@Struan-Murray
Copy link
Contributor Author

Hi @jklymak. I tried to create a test for this at the time but unfortunately have not had the time to get it to work, especially for something which was such a small issue. Had I worked on anything like this before I would likely be able to do it but I am interested to see how this is handled (if anyone is willing) so I am capable of creating tests in the future.

@QuLogic QuLogic marked this pull request as ready for review August 27, 2020 05:03
@QuLogic
Copy link
Member

QuLogic commented Aug 27, 2020

I rebased and added a test for you. Note, this is a little harder than @jklymak guessed, as it's not a warning, but a warning-level log message.

@dopplershift dopplershift dismissed QuLogic’s stale review August 28, 2020 02:31

Since Qulogic wrote the test.

@dstansby dstansby added this to the v3.4.0 milestone Sep 20, 2020
@dstansby
Copy link
Member

Thanks for the PR!

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.

None yet

5 participants