8000 Make legend title fontsize obey fontsize kwarg by default by gepcel · Pull Request #9731 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Make legend title fontsize obey fontsize kwarg by default #9731

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

Closed
wants to merge 4 commits into from

Conversation

gepcel
Copy link
Contributor
@gepcel gepcel commented Nov 9, 2017

As the issue #8699 from long time ago stats, I often found this annoying. Seems like nobody is against this idea, and it is a really simple modification. So I'm pulling a request.

PR Summary

PR Checklist

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

As the issue [matplotlib#8699](matplotlib#8699) from long time ago stats, I often found this annoying. Seems like nobody is against this idea, and it is a really simple modification. So I'm pulling a request.
@jklymak
Copy link
Member
jklymak commented Nov 9, 2017

Thanks for the PR. Seems reasonable to me.

To get this to pass, you'll (unfortunately) need to update some of the test_legend image files. See https://travis-ci.org/matplotlib/matplotlib/jobs/299639390#L2312

@gepcel
Copy link
Contributor Author
gepcel commented Nov 10, 2017

I'll find some time to see what I can.

@jklymak
Copy link
Member
jklymak commented Nov 10, 2017

You could alternatively add a fontsize argument to the legend entries on the failing tests to match the test image sizes. That would be easier on the repository size.

@gepcel
Copy link
Contributor Author
gepcel commented Nov 13, 2017

@jklymak Sorry for the delay. Now I'm planning to complete this. But I'm new to this. Can you help me out?

If I understands right, there are a few images failed in the tests due to the font size of legend title. fancy, and scatter_rc1, and scatter_rc3 (.pdf, .png, .svg) to be specific. So I'm supposed to generate those 9 images with the latest master code and include those in a commits. Right?

And by what you say a fantasize argument, I'm guessing you mean I can modify the test code so that the font size of legend title matches the old ones. Maybe like:

lg = legend(...)
lg.get_title().set_fontsize(10)

Am I guessing right? Would you please explain a little?

I'm really new to this. Now I'm actually starting from reading The Matplotlib Developers’ Guide. Any help would be appreciated.

Set the fontsize of legend title back to 10, in order to pass thhose tests.
@WeatherGod
Copy link
Member

Why would it be necessary to set the fontsize in the tests? That would mean that this is a breaking change. I would have figured that this should only fix the situation where fontsize was explicitly specified as a keyword argument? In which case, we would need a test that demonstrated that that works now.

@jklymak
Copy link
Member
jklymak commented Nov 13, 2017

Yes, I'm confused now. There is a bunch of fontsize handling stuff above this. Why is it wrong and why don't you fix up there? Certainly if you have to grab the title fontsize after instantiation there is something awry. I think whats happening is the legend title is set to whatever the legend fontsize is perfectly reasonable....

@jklymak
Copy link
Member
jklymak commented Nov 13, 2017

Oh, OK, I see, it wasn't respecting the legend fontsize or the legend font properties.

I'm not clear on what TextArea defaults to in terms of properties? Axes default fonts? In which case it begs the question what is better, to default to the rest of legend or default to the rest of the axes.

@gepcel
Copy link
Contributor Author
gepcel commented Nov 13, 2017

@WeatherGod, You were right, by this pull request, I just want the fontsize of legend title to obay the fontsize kwarg of plt.legend(). But since the tests failed, and after trying very hard I failed to set up a develop environment, thus I can't test changes locally. Since the only thing changed was the legend title fontsize, I thought that setting the fontsize back in the tests would be an easy way to get this commits passed. Still trying to figure out how to make this work.

Please anyone give me some instructions on what to do next. Or just make this right (much easier than teaching me to do it).

Maybe the default fontsize is 12, let me see if this passes.
@jklymak
Copy link
Member
jklymak commented Nov 14, 2017

I think @WeatherGod point is that the legend title's font properties are set by the axes font properties. The logic is that the legend font properties are for inside the legend, and the title is outside. If you want to change the behaviour thats a bigger discussion. I appologize if I mislead you above. I thought that the legend's title font properties weren't controllable at all.

@gepcel
Copy link
Contributor Author
gepcel commented Nov 14, 2017

@jklymak. Before this, the fontsize of legend item labels is 'large'. And the legend title doesn't pick this as its own size, so maybe 10 12 as the default font size. After the change, the fontsize title would be large, the same as legend item labels.

After another looking, maybe I picked a wrong version of matplotlibrc, which says font.size=10, legend.fontsize='medium', instead of 12, and large. And of all tests, this will only affect those with a legend and more importantly with a legend title. That's the reason I only changes those few lines in tests. Maybe there's more I haven't noticed.

As your question "what is better, to default to the rest of legend or default to the rest of the axes". I think default to the legend is more sensible. And besides that, it makes more sense that plt.legend(fontsize=somevalue) would change the title all together (the kwarg is not something like label_fontsize after all). And lg.get_title().set_fontsize(some_other_value) to change it if not suited.

@gepcel
Copy link
Contributor Author
gepcel commented Nov 14, 2017

@WeatherGod, So these falls to 2 questions:

  • Without a fontsize kwarg, should the fontsize of lebend title defaults to label.fontsize of legend or font.size of the default of others.
  • With a fontsize kwargs, would it better to set the whole legend font size or better to only set the item labels without the title.

@jklymak
Copy link
Member
jklymak commented Nov 14, 2017

There is merit to that argument, but as @WeatherGod says, its a "breaking change" in that people who were counting on the old behaviour will be discomfited by the new behaviour. So, it needs a consensus that the breakage is acceptable.

@dstansby dstansby added this to the v2.2 milestone Nov 22, 2017
@jklymak jklymak modified the milestones: needs sorting, v3.0 Mar 7, 2018
@jklymak
Copy link
Member
jklymak commented Mar 7, 2018

I think for 3.0 we could consider adding this kwarg again. @gepcel did you manage to set up a development environment, or did you want someone else to take this over?

@gepcel
Copy link
Contributor Author
gepcel commented Mar 8, 2018

@jklymak It would be better if someone take this over. I had the development evvironment, but lost it long time ago during the os reinstallation.

And this may need some more discussion. It's not uncommon that people might need the fontsize of legend title larger than that of legend items. It would be better to have a convenient way to make this happen without make things complicated.

Since the matplotlib is some kind of foundation of many other tools, it need to accomplish so many features, convenient or not.

But matplotlib is also used a lot to quickly plot figures in day to day data analyse works. For this, it would be better that the workflow is more intuitive and convenient.

Thanks.

@jklymak
Copy link
Member
jklymak commented Mar 8, 2018

If its OK, I'll just push to your branch to keep this history. I still think this change is reasonable, and I think sticking it in 3.0 should be fine.

@gepcel
Copy link
Contributor Author
gepcel commented Mar 8, 2018

@jklymak, Do what ever you want. Anything I can help, please let me know.

@jklymak
Copy link
Member
jklymak commented Mar 8, 2018

@gepcel Thanks so much for your work on this. This approach makes some sense, but is a breaking change. I've proposed an alternate at #10715 that is not a breaking change. i.e it will keep the old behaviour by default, but allow users to set the legend title to a different fontsize if they wish. I hope this is OK w/ you and would suit your needs. I'll keep this open in the case that it does not.

@gepcel gepcel closed this Mar 8, 2018
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.

4 participants
0