Add legend title size rc parameter #10121

Raab70 · 2017-12-27T20:30:36Z

PR Summary

Fixes #9201 by adding an rc parameter to customize legend title size. Following the checklist from that issue to add this parameter. This allows for customization of the legend title font size from the matplotlibrc file, a requested feature addition.

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

dstansby

A few small changes, but 👍 overall

dstansby · 2017-12-28T21:59:09Z

doc/users/next_whats_new/2017-12-27_legend_title_size_rc.rst

+Legend Title Size rc parameter
+---------------------
+
+A new rc parameter has been added as an option in your matplotlibrc allowing you to control the font size of the legend title.


Looks like this is longer than 79 characters, so is failing the PEP8 tests - please could you make it <= 79 characters on each line?

We should not be pep8 checking the rst files....

If we are that seems like a bug in the tests.

I think @dstansby misread something; pep8 is failing in test_legend.py:

.../lib/matplotlib/tests/test_legend.py:193:1: E302 expected 2 blank lines, found 1

dstansby · 2017-12-28T21:59:23Z

doc/users/next_whats_new/2017-12-27_legend_title_size_rc.rst

@@ -0,0 +1,6 @@
+Legend Title Size rc parameter
+---------------------


I think the underline needs to be exactly the same length as the title

dstansby · 2017-12-28T21:59:52Z

matplotlibrc.template

@@ -414,6 +414,7 @@ backend      : $TEMPLATE_BACKEND
 #legend.scatterpoints : 1        # number of scatter points
 #legend.markerscale   : 1.0      # the relative size of legend markers vs. original
 #legend.fontsize      : medium
+#legend.titlesize     : medium    # size of the legend title


Could this be "fontsize of the legend title" just to be clear?

tacaswell · 2017-12-29T02:02:18Z

Thanks for your work on this @Raab70 !

Just a minor formatting change in the formatting of the tests and an rst issue and this looks good to go. It is a bit odd to have something that is only controlled by an rcparam (rather than a function argument) but there is currently no control over this (other than just reaching in and poking at the Text object) and it is definitely something that makes sense to control via a stylesheet. If and when someone asks for per-call-to-legend control of this we will sort out how to do kwargs with this.

The white-space-in-the-tests issue may seem petty / annoying, but having automated tests on codestyle really does help on a codebase the size of Matplotlib.

Raab70 · 2017-12-29T03:14:42Z

I have addressed the following changes:

Fixed the title designation on the next_whats_next doc
Fixed the pep8 error on the test
Clarified the matplotlibrc text as requested

Thanks for all your help. I don't think any of this is petty, definitely impressive the amount of time you have all put into such a small PR. I appreciate it. This is my first issue and I look forward to more (suggestions welcome! This was way easy and I look forward to tackling some harder ones).

jklymak

I agree w/ @tacaswell that its a bit odd to not also have a kwarg for this...

jklymak · 2017-12-29T03:17:54Z

matplotlibrc.template

@@ -414,6 +414,7 @@ backend      : $TEMPLATE_BACKEND
 #legend.scatterpoints : 1        # number of scatter points
 #legend.markerscale   : 1.0      # the relative size of legend markers vs. original
 #legend.fontsize      : medium
+#legend.titlesize     : medium    # fontsize of the legend title


This is a breaking change? Because the legend fontsize used to be the axes fontsize, so if someone changed the axes fontsize they may expect the legend title to also change size. So... I think it needs an API entry as well.

I see what you're saying. On unpatched matplotlib:

In [28]: mpl.rcParams.update({'font.size': 22}) In [29]: ax = plt.subplot(111);ax.scatter(np.arange(0,5), np.arange(0,5), label='One') In [30]: ax.legend(fontsize=18, title='My Great Legend') In [31]: ax.get_legend().get_title().get_fontsize() Out[31]: 22.0

With this patch that new font size would be this default value. Suggestions for how to proceed? Just an entry in the API doc?

Two choices, I guess. Either just add an API change. I'd personally be fine w/ that.

Or, if you want to be fancy, the default could be "None", in which case it could be made to just use the axes fontsize. If the user specifies a fontsize, then that gets used instead.

Given that there was no test that broke, I assume the default is 'medium'. OTOH, its a bit of a pain to set all the different fontsizes if you want them to be bigger, and this one is particularly obscure. So maybe "None" is the way to go? I don't feel strongly either way.

I'm not sure off the top of my head how "None" gets parsed in the rcParams, but there are a few entries with a None entry, so I imagine you could figure it out.

I looked again and there is no validate_fontsize_or_none validation and I do t see why you should do it here. I’d say just add an API doc.

I have added a first pass at an API doc.

Another option here is to default to 'inherit' which falls back to not passing anything into set_title (we do this a few other places).

@tacaswell I could do that. Do we also want this to be a kwarg?

@Raab70 if you take a look at rcsetup.py you'll see how validate_color_or_inherit works.

I am fine either way about adding a kwarg. On one hand, it would be reasonable to expect an kwarg on the other hand legend already has too many kwrags.

In that case I would go with your comment above to keep the clutter down for something that is not likely to be used very often.

If and when someone asks for per-call-to-legend control of this we will sort out how to do kwargs with this.

jklymak

👍 This looks good to me. Sorry for a couple more fussy things.

I'm not going to approve, just because I think someone more senior should approve this. The change is somewhat of a breaking change, and I don't have the experience to know how much guff from downstream packages this might cause.

jklymak · 2017-12-30T04:49:11Z

doc/api/api_changes/2017-12-28-RH.rst

+New rcParam for Legend Title Font Size
+``````````````````````````````````````
+
+The introduction of a new rcParam for legend title font size (``legend.titlesize``) deprecates the previous usage of the ``font.size`` rcParam for modification of the legend title font size.


I think you mean "deprecate" -> "replaces". To me, at least, "deprecation" means you will still let it work, but it will stop working at some point. You aren't deprecating, you are doing a hard replace.

I think this should explicitly state that old code where font.size was changed will now also need to change legend.titlesize to get the same effect, and that the default is medium (as is font.size).

jklymak · 2017-12-30T04:53:03Z

doc/users/next_whats_new/2017-12-27_legend_title_size_rc.rst

+
+A new rc parameter has been added as an option in your matplotlibrc allowing you to control the font size of the legend title.
+
+`legend.titlesize = 'large'`


This is a little ambiguous to a new user as to where they would enter this command. I'd be more explicit with a bit of code:

.. code-block:: python import matplotlib.rcParams as rcParams rcParams['legend.titlesize'] = 'large'

Raab70 · 2017-12-30T14:31:44Z

The latest commit addresses your comments @dstansby, thanks for the feedback. The documentation is much clearer now.

jklymak · 2018-01-02T17:34:43Z

Ooops, sorry, you need an extra carriage return as per the error linked above (under details). Sorry for the mistype.

.. code-block:: python

    import matplotlib.rcParams as rcParams
    
    rcParams['legend.titlesize'] = 'large'

Raab70 · 2018-01-03T13:33:46Z

This latest commit changes the default behavior to inherit as suggested by @tacaswell. For this change I have:

Added a new validate_fontsize_or_inherit function to rcsetup.py
Made the appropriate changes for the inherit option to work in legend.py
Updated the next_whats_next documentation to reflect this change
Removed the api doc since this is no longer a breaking change
Added a test that inherit is working properly

jklymak · 2018-01-06T03:35:09Z

ping @dstansby - I think @Raab70 covered all the suggestions...

Raab70 · 2018-02-02T20:54:37Z

ping @jklymak @dstansby Is there anything else needed on this?

jklymak · 2018-02-02T21:58:04Z

We need 2 approvals, and no disapprovals, so you are at the mercy of @dstansby!

andrewfowlie · 2018-02-02T22:55:53Z

Nice one, sorry I didn't get back into this

Raab70 · 2018-03-05T21:23:18Z

Ping @dstansby, Also I see there's now a merge conflict on matplotlibrc.template, I guess that means I need to merge upstream changes into this branch? I'm not certain I understand the conflict by clicking the link in here. It's a rather poor interface.

timhoffm · 2018-05-06T00:04:12Z

Also I see there's now a merge conflict on matplotlibrc.template, I guess that means I need to merge upstream changes into this branch?

You should rebase this branch onto the current master.

# Update the mirror of trunk
git fetch upstream
# go to the feature branch
git checkout feature/legend-title-rcparam
# make a backup in case you mess up
git branch tmp feature/legend-title-rcparam
# rebase
git rebase upstream/master

If neccessary, resolve any conflicts and force-push your feature-branch again.

When all looks good you can delete your backup branch: git branch -D tmp

ImportanceOfBeingErnest · 2018-05-06T00:06:56Z

I think the functionality is much more easily provided via #11172, which would just need to be merged.

Raab70 added 5 commits December 27, 2017 14:20

add legend title size to rcsetup.py

d25ebb3

add legend title size rcparam check to legend creation

b346930

add test for legend title size rc param

51759e6

add documentation on new rcparam

ed918b6

adding next_whats_next

98ac73b

dstansby added the topic: rcparams label Dec 28, 2017

dstansby requested changes Dec 28, 2017

View reviewed changes

tacaswell added this to the v2.2 milestone Dec 29, 2017

address PR review changes

3d20863

jklymak reviewed Dec 29, 2017

View reviewed changes

Raab70 added 2 commits December 28, 2017 21:59

test legend title font size without accessing protected members

2bfef64

add api doc

ecfd688

jklymak reviewed Dec 30, 2017

View reviewed changes

documentation clarification

550b6a6

Raab70 added 2 commits January 2, 2018 15:25

fix documentation typo

d2a4e7e

change default behavior to inherit

3807000

jklymak approved these changes Jan 3, 2018

View reviewed changes

jklymak mentioned this pull request May 4, 2018

ENH: add title_fontsize to legend #10715

Merged

4 tasks

ImportanceOfBeingErnest mentioned this pull request May 5, 2018

ENH add rcparam to legend_title #11172

Merged

6 tasks

Raab70 closed this May 6, 2018

QuLogic removed this from the needs sorting milestone May 6, 2018

Raab70 deleted the feature/legend-title-rcparam branch September 2, 2018 13:51

		@@ -0,0 +1,6 @@
		Legend Title Size rc parameter
		---------------------


		A new rc parameter has been added as an option in your matplotlibrc allowing you to control the font size of the legend title.

		`legend.titlesize = 'large'`

Uh oh!

Add legend title size rc parameter #10121

Add legend title size rc parameter #10121

Uh oh!

Conversation

Raab70 commented Dec 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

dstansby left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Raab70 commented Dec 29, 2017

Uh oh!

jklymak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Raab70 commented Dec 30, 2017

Uh oh!

jklymak commented Jan 2, 2018

Uh oh!

Raab70 commented Jan 3, 2018

Uh oh!

jklymak commented Jan 6, 2018

Uh oh!

Raab70 commented Feb 2, 2018

Uh oh!

jklymak commented Feb 2, 2018

Uh oh!

andrewfowlie commented Feb 2, 2018

Uh oh!

Raab70 commented Mar 5, 2018

Uh oh!

timhoffm commented May 6, 2018

Uh oh!

ImportanceOfBeingErnest commented May 6, 2018

Uh oh!

Raab70 commented Dec 27, 2017 •

edited

Loading

QuLogic Dec 29, 2017 •

edited

Loading

tacaswell commented Dec 29, 2017 •

edited

Loading