FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add legend.labelcolor in rcParams by Carloscerq · Pull Request #20084 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Add legend.labelcolor in rcParams - #20084

Merged
tacaswell merged 1 commit into
matplotlib:masterfrom
Carloscerq:add_legend_labelcolor_rcParam
May 22, 2021
ConversationCommits (1)ChecksFiles changed
Merged

Add legend.labelcolor in rcParams#20084
tacaswell merged 1 commit into
matplotlib:masterfrom
Carloscerq:add_legend_labelcolor_rcParam

Conversation

Carloscerq commented Apr 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Summary

This is a proposal for a new legend rcParam.
The legend.labelcolor was proposed in #20049. It's for changing the labelcolor of all figures(similar to the labelcolor of plt.legend)

Example:

import numpy as np
import lib.matplotlib.pyplot as plt

a = np.arange(2, 3, .02)
c = np.exp(a)
plt.rcParams['legend.labelcolor'] = 'linecolor'

fig, ax = plt.subplots()
ax.plot(a, c, 'g', label='Model length')
ax.plot(c, a, 'r', label='Data length')

legend = ax.legend()

plt.show()

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

github-actions Bot reviewed Apr 26, 2021
View reviewed changes

github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

Carloscerq marked this pull request as draft April 27, 2021 21:03
Carloscerq marked this pull request as ready for review April 28, 2021 01:28
Carloscerq marked this pull request as draft April 28, 2021 01:42
Carloscerq marked this pull request as ready for review April 28, 2021 02:44

tacaswell commented Apr 28, 2021

Copy link
Copy Markdown
Member

Thanks for working on this!

Overall 👍, but is it possible to have the default rcParam be None? Currently the default label color will fall back to the rcParams['text.color'] but with this PR will default to 'black'

tacaswell added this to the v3.5.0 milestone Apr 28, 2021

Carloscerq commented Apr 28, 2021

Copy link
Copy Markdown
Contributor Author

Sure. I think that making a little change in the lib/matplotlib/legend.py it can work. I will give it a try.

QuLogic reviewed Apr 29, 2021
View reviewed changes
Comment thread doc/users/next_whats_new/rcparams_legend.rst Outdated
Comment thread doc/users/next_whats_new/rcparams_legend.rst Outdated
Comment thread doc/users/next_whats_new/rcparams_legend.rst Outdated
Comment thread doc/users/next_whats_new/rcparams_legend.rst Outdated
Comment thread doc/users/next_whats_new/rcparams_legend.rst Outdated
Comment thread doc/users/next_whats_new/rcparams_legend.rst Outdated
Comment thread lib/matplotlib/rcsetup.py Outdated
QuLogic approved these changes Apr 30, 2021
View reviewed changes
Comment thread doc/users/next_whats_new/rcparams_legend.rst Outdated
jklymak reviewed May 10, 2021
View reviewed changes
Comment thread lib/matplotlib/legend.py Outdated
if labelcolor is None:
pass
elif isinstance(labelcolor, str) and labelcolor in color_getters:
if mpl.rcParams['legend.labelcolor'] != 'none':

jklymak May 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The legend docstring should note that this rcparam exists....

tacaswell May 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should this be `

Suggested change
if mpl.rcParams['legend.labelcolor'] != 'none':
if mpl.rcParams['legend.labelcolor'] is not None:

along with similar changes to validator? There are a number of places where we use the string 'none' as a color to mean "this thing does not get a color" and None (the singleton) to mean "use the default color".

That said, currently doing

...
ax.legend(labelcolor='none')

does not result in an invisible label which is a bug in the implementation below (cycle of a 0 length thing is also 0 length, not infinite).

QuLogic May 14, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The rcParam can't be able to both be None and 'none', though, right? The fallback version would need a special value to work, like 'auto' or 'textcolor'?

tacaswell May 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

#20225 fixes the bug I noted above!

jklymak reviewed May 13, 2021
View reviewed changes
Comment thread lib/matplotlib/legend.py
'markerfacecolor' (or 'mfc'), or 'markeredgecolor' (or 'mec').

Otherly, labelcolor can be set globally using :rc:`legend.labelcolor`.

jklymak May 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry to be picky, bit this should go in as

labelcolor : str or list, default: :rc:`legend.labelcolor`.

Carloscerq May 13, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the review.
But when the rcParams[legend.labelcolor] is None(the default value(, should the docs say that it just uses the rcParams['text.color'] value?

jklymak May 13, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think that should be specified in the description of the parameter if it is not already

jklymak May 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is fine w/ me now. @tacaswell you have a block on this...

tacaswell requested changes May 14, 2021
View reviewed changes

tacaswell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks on the right path, but the default value should be None not 'none'.

Comment thread lib/matplotlib/legend.py Outdated
if labelcolor is None:
pass
elif isinstance(labelcolor, str) and labelcolor in color_getters:
if mpl.rcParams['legend.labelcolor'] != 'none':

tacaswell May 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should this be `

Suggested change
if mpl.rcParams['legend.labelcolor'] != 'none':
if mpl.rcParams['legend.labelcolor'] is not None:

along with similar changes to validator? There are a number of places where we use the string 'none' as a color to mean "this thing does not get a color" and None (the singleton) to mean "use the default color".

That said, currently doing

...
ax.legend(labelcolor='none')

does not result in an invisible label which is a bug in the implementation below (cycle of a 0 length thing is also 0 length, not infinite).

tacaswell mentioned this pull request May 14, 2021
Merged
3 tasks done
Carloscerq marked this pull request as draft May 16, 2021 17:52
Carloscerq marked this pull request as ready for review May 16, 2021 19:07
jklymak requested a review from tacaswell May 17, 2021 15:40
jklymak reviewed May 17, 2021
View reviewed changes
Comment thread doc/users/next_whats_new/rcparams_legend.rst Outdated
QuLogic reviewed May 17, 2021
View reviewed changes
Comment thread lib/matplotlib/legend.py Outdated

jklymak commented May 22, 2021

Copy link
Copy Markdown
Member

@tacaswell you are still blocking on this....

tacaswell merged commit 9b9c42d into matplotlib:master May 22, 2021

tacaswell commented May 22, 2021

Copy link
Copy Markdown
Member

Thanks @Carloscerq ! Congratulations on your first merged Matplotlib PR 🎉 hope we hear from you again!

tacaswell mentioned this pull request Jun 30, 2021
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

QuLogic QuLogic approved these changes
github-actions[bot] github-actions[bot] left review comments
tacaswell tacaswell approved these changes
jklymak jklymak approved these changes

Assignees

No one assigned

Labels

New feature topic: legend topic: rcparams

Projects

None yet

Milestone

v3.5.0

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL