8000 Fixed a bug where rcParams settings were being ignored for formatting axes labels by ayshih · Pull Request #25237 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Fixed a bug where rcParams settings were being ignored for formatting axes labels #25237

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 21, 2023
Merged

Fixed a bug where rcParams settings were being ignored for formatting axes labels #25237

merged 2 commits into from
Feb 21, 2023

Conversation

ayshih
Copy link
Contributor
@ayshih ayshih commented Feb 17, 2023 •
edited
Loading

PR Summary

Matplotlib 3.7.0 has a bug where rcParams settings are being ignored for formatting axes labels. This bug was introduced with #21253, specifically the use of the Text._reset_visual_defaults() method to reset the formatting of an axis label, including during the initialization of an axis. Since the axis label is just a Text, the color gets reset to text.color instead of axes.labelcolor, and axes.labelsize and axes.labelweight are ignored. (This bug is not readily apparent when text.color and axes.labelcolor happen to be the same, as is common for many styles.)

Example script:

import matplotlib as mpl
import matplotlib.pyplot as plt

with mpl.rc_context({'axes.labelcolor': 'red', 'axes.labelsize': 20, 'axes.labelweight': 'bold'}):
    plt.plot([1, 2, 1])
    plt.xlabel('xlabel')
    plt.show()

Output on Matplotlib 3.7.0:
Figure_1

Output with this PR:
Figure_2

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/ 10000 next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

github-actions[bot]
github-actions bot reviewed Feb 17, 2023
View reviewed changes
Copy link
@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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.

@dstansby dstansby added this to the v3.7.1 milestone Feb 17, 2023
jklymak
jklymak reviewed Feb 17, 2023
View reviewed changes
lib/matplotlib/axis.py
@@ -879,6 +879,9 @@ def clear(self):
- registered callbacks
"""
self.label._reset_visual_defaults()
self.label.set_color(mpl.rcParams['axes.labelcolor'])
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this all happen in _reset_visual_defaults?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self.label is simply a Text instance, so self.label._reset_visual_defaults() looks, for example, at the text.color rcParams instead of the axes.labelcolor rcParams.

self.label._reset_visual_defaults() does accept input arguments to override text defaults (although arguably that that doesn't make complete sense given the name of the method). So, for color in specific, one could call self.label._reset_visual_defaults(color=mpl.rcParams['axes.labelcolor']). For axes.labelsize and axes.labelweight, it takes a little more effort because those have to be passed in through the fontproperties keyword argument.

Ultimately, I decided that it was overly complicated to pass in those arguments to _reset_visual_defaults() or to modify that method for self.label to be aware of the defaults for an axis label rather than the defaults for text. The approach that I took in this PR felt more straightforward.

I have added a clarifying comment for future maintainers.

jklymak
jklymak reviewed Feb 20, 2023
View reviewed changes
Copy link
Member
@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Looks good, but I think you need to wrap the test in a style context, rather than set the parameter values as those will persist. However, I could be mistaken in how the decorator works....

lib/matplotlib/tests/test_axes.py

@mpl.style.context('default')
def test_rc_axes_label_formatting():
mpl.rcParams['axes.labelcolor'] = 'red'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mpl.rcParams['axes.labelcolor'] = 'red'
mpl.rcParams['axes.labelcolor'] = 'red'

I think these should be wrapped in an rcContext, otherwise they apply for the rest of the test file, which could be confusing for future tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it should be fine because the matplotlib.style.context decorator already wraps everything in an rc_context:

matplotlib/lib/matplotlib/style/core.py

Line 220 in f726d2c

with mpl.rc_context():

Admittedly, I have not tested it.

@jklymak jklymak merged commit 4de75b7 into matplotlib:main Feb 21, 2023
@jklymak
Copy link
Member
jklymak commented Feb 21, 2023

Thanks @ayshih I squashed your commits. Congratulations on your first PR to Matplotlib!

@meeseeksmachine meeseeksmachine mentioned this pull request Feb 21, 2023
Merged
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Feb 21, 2023
@ayshih @meeseeksmachine
Backport PR matplotlib#25237: Fixed a bug where rcParams settings wer…
acfd5b1 
…e being ignored for formatting axes labels
@ayshih
Copy link
Contributor Author
ayshih commented Feb 21, 2023

Great, thanks!

ksunden added a commit that referenced this pull request Feb 21, 2023
@ksunden
Merge pull request #25276 from meeseeksmachine/auto-backport-of-pr-25…
c70ddbc 
…237-on-v3.7.x

Backport PR #25237 on branch v3.7.x (Fixed a bug where rcParams settings were being ignored for formatting axes labels)
@ayshih ayshih deleted the axes_label branch February 22, 2023 04:01
@ayshih ayshih mentioned this pull request Feb 22, 2023
Merged
@rcomer rcomer mentioned this pull request Feb 23, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@jklymak jklymak jklymak approved these changes

@dstansby dstansby dstansby approved these changes

Assignees
No one assigned
Labels
topic: rcparams topic: ticks axis labels
Projects
None yet
Milestone
v3.7.1
Development

Successfully merging this pull request may close these issues.
4 participants
@ayshih @jklymak @dstansby @QuLogic
0