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

Clarify tutorial "Customizing Matplotlib with style sheets and rcParams" #20812

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

@andthum
Copy link

@andthum andthum commented Aug 9, 2021

PR Summary

Closes #20800

The tutorial Customizing Matplotlib with style sheets and rcParams was not clear about what rcParams can be set in style sheets. The blacklist of rcParams that cannot be set in style sheets is now documented in the docstring of matplotlib.style.use and the tutorial links to this blacklist.

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).
Copy link

@github-actions github-actions bot left a comment

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.

Copy link
Member

@tacaswell tacaswell left a comment

Thank you for working on this! I left some comments that would be good to address but are not deal-breakers.

Good to merge once the docs build cleanly!

@@ -104,15 +120,10 @@
plt.show()

###############################################################################
# .. _matplotlib-rcparams:

This comment has been minimized.

@tacaswell

tacaswell Aug 9, 2021
Member

I think I am 👍🏻 on this re-structuring to keep 3 headings (to match the 3 numbered things at the top). I am 50% sure if you can put more than one target on a section heading, but if not the 3 places we reference it will need to be updated.

This comment has been minimized.

@andthum

andthum Aug 10, 2021
Author

I've updated the 3 places to reference now customizing-with-dynamic-rc-settings

#
# .. _matplotlibrc-sample:
#
# A sample matplotlibrc file
# ~~~~~~~~~~~~~~~~~~~~~~~~~~
# --------------------------

This comment has been minimized.

@tacaswell

tacaswell Aug 9, 2021
Member

While you are here change this section heading to point out that is the default defaults?

This comment has been minimized.

@andthum

andthum Aug 10, 2021
Author

Renamed section to The default matplotlibrc file

aesthetics of ggplot_ (a popular plotting package for R_). To use this style,
just add:
aesthetics of ggplot_ (a popular plotting package for R_). To use this
style, just add:

This comment has been minimized.

@tacaswell

tacaswell Aug 9, 2021
Member

Suggested change
style, just add:
style, add:

As long as you are touching this line lets remove "just" which is at best a filler word at worst alienate readers who do not find the following code simple!

This comment has been minimized.

@tacaswell tacaswell added this to the v3.5.0 milestone Aug 9, 2021
lib/matplotlib/style/core.py Outdated Show resolved Hide resolved
andthum added 3 commits Aug 9, 2021
Clarify that only the rcParam that is not related to style is ignored
instead of the entire style sheet.
  * Include `STYLE_BLACKLIST` in the documentation of
    `matplotlib.style.use` in alphabetical order.
  * Print list of rcParams in `matplotlib.RcParams` in alphabetical
    order.
  * Fix broken links to `.style.available` in
    `lib/matplotlib/style/core.py`.
@andthum andthum force-pushed the andthum:style-sheet-docs branch from f3af5c6 to 08bb99d Aug 10, 2021
3. :ref:`Changing your matplotlibrc file<customizing-with-matplotlibrc-files>`.
Style sheets take precedence over :file:`matplotlibrc` files and setting
rcParams at runtime takes precedence over style sheets.

This comment has been minimized.

@andthum

andthum Aug 10, 2021
Author

Newly added.

@docstring.Substitution(
"\n".join(
sorted(
set(map("- {}".format, map(str.strip, STYLE_BLACKLIST))),

This comment has been minimized.

@QuLogic

QuLogic Aug 11, 2021
Member

STYLE_BLACKLIST is a set already, no need to wrap it.

Suggested change
set(map("- {}".format, map(str.strip, STYLE_BLACKLIST))),
map("- {}".format, map(str.strip, STYLE_BLACKLIST)),

This comment has been minimized.

@andthum

andthum Aug 11, 2021
Author

I used the set to remove possible duplicates in STYLE_BLACKLIST. It's just a security layer in case someone puts a duplicate entry in STYLE_BLACKLIST by accident, although this is very unlikely to happen. So, I will remove it.

Edit:
The map(str.strip, STYLE_BLACKLIST) is also just a security layer to remove white space in front or at the end of a blacklisted style parameter. So, I guess this should be removed as well.

This comment has been minimized.

@tacaswell

tacaswell Aug 11, 2021
Member

STYLE_BLACKLIST is already a set literal when it is defined

STYLE_BLACKLIST = {
'interactive', 'backend', 'webagg.port', 'webagg.address',
'webagg.port_retries', 'webagg.open_in_browser', 'backend_fallback',
'toolbar', 'timezone', 'datapath', 'figure.max_open_warning',
'figure.raise_window', 'savefig.directory', 'tk.window_focus',
'docstring.hardcopy', 'date.epoch'}

and sorted will happily iterate over the map without having to fully materialize it first. I also agree it is reasonable to expect that it will contain only valid rcparams (which at this point none of them contain whitespace!) and not worry about doing any cleaning.

That said, I am 👍 on the concern of covering surprising inputs graceful!

1. :ref:`Using style sheets<customizing-with-style-sheets>`.
2. :ref:`Setting rcParams at runtime<customizing-with-dynamic-rc-settings>`.
3. :ref:`Changing your matplotlibrc file<customizing-with-matplotlibrc-files>`.
Style sheets take precedence over :file:`matplotlibrc` files and setting
rcParams at runtime takes precedence over style sheets.
Comment on lines +11 to +16

This comment has been minimized.

@QuLogic

QuLogic Aug 11, 2021
Member

These should probably be listed in priority order.

This comment has been minimized.

@andthum

andthum Aug 11, 2021
Author

I was also thinking about this, but finally mirrored the order of the tutorial sections. If you prefer, I can list the items in priority order. When doing this, should I also change the section order accordingly or keep it as is?

@docstring.Substitution("\n".join(map("- {}".format, rcsetup._validators)))
@docstring.Substitution(
"\n".join(
sorted(

This comment has been minimized.

@andthum

andthum Aug 11, 2021
Author

Is it desirable to sort the list of valid rcParams alphabetically or should I remove the sorting? (Same for STYLE_BLACKLIST)

This comment has been minimized.

@tacaswell

tacaswell Aug 11, 2021
Member

I am weakly 👍🏻 on sorting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants