8000 Group figure.subplot.* rc to a single rcParam. by anntzer · Pull Request #11231 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Group figure.subplot.* rc to a single rcParam. #11231

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
Closed
wants to merge 1 commit into from
Closed

Group figure.subplot.* rc to a single rcParam. #11231

wants to merge 1 commit into from

Conversation

anntzer
Copy link
Contributor
@anntzer anntzer commented May 11, 2018 •
edited
Loading

The idea is to make it easier to restore a figure's subplotparams to the
rc-provided defaults. Specifically, this can now be done with

fig.subplots_adjust(**rcParams["figure.subplot"])

or

fig.subplots_adjust(**dict(rcParams["figure.subplot"], left=...))

if some values need to be overridden.

Also make Figure.clf() restore the subplotsparams to these rc-provided
defaults (that is the original motivation for the change).

Note that this PR runs into a limitation of the rcParams API: it would
be nicer if the validators took the rcParams as parameters (e.g.
implicitly by being methods of the RcParams class), so that they can
actually depend on another key in the same instance.

Also, the tests now restore original rcParams using dict.update to
avoid revalidating them and running into the warnings.
(rc_context.__exit__ does the same.)

Alternative to #11086.

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

@ImportanceOfBeingErnest
Copy link
Member

This PR seems to restrict the subplot params into the the range [0,1]. This not useful.
All you have to check for is that left is smaller than right and bottom is smaller than top.
In addition you may check for left==right, but that may need to be allowed too, and it may be a fall-back in tight_layout.

I don't like figure.subplot, because it sound so general... like "Hell yes, I want a subplot, so let's set it to True". But I suppose it's the natural name, given the current params.

@anntzer
Copy link
Contributor Author
anntzer commented May 11, 2018

I just copied the name from figure.subplot.foo, don't really like it either, happy to change it for something better.
The 0 < ... < 1 restriction comes from the rcParams validator, will drop it, but note (as a side point) the benefit of having a single validation point for the rcParams...

@anntzer anntzer force-pushed the single-subplot-adjust branch from f04d697 to ebb16de Compare May 11, 2018 15:05
@ImportanceOfBeingErnest
Copy link
Member

Uh, so maybe that validator should change. I agree that it doesn't make sense to be able to set values larger 1 only via the api but not via rc.

Resons to keep it as it is:

  • You may simply want to have >100% space between subplots (e.g. for small plots with large captions).
  • When using the bbox_inches="tight" saving option, you may use some negative margin to get a desired output.
  • When combining subplots with 3d axes you might want to fiddle around with the subplot_params to get a 2d and 3d plot nicely aligned.
  • When creating a wide figure with an image and a colorbar you may use left < 0 to center the image. When creating a subplot grid with colorbars you may use right > 1 to reduce space to the right of the colorbar.
  • When creating a wedged polar plot, you may need out of range params to get the plot tight into the figure (Wedged polar plot's position/scaling leaves too much white space. #10264)

The latter are mainly workarounds, but as long as such problems do not have a good solution (and there will always such special cases I figure) it would be a pitty to artificially limit the available options to solve them quickly.

@anntzer anntzer force-pushed the single-subplot-adjust branch 2 times, most recently from 5955f3a to 80b1a8e Compare May 11, 2018 16:09
@anntzer
Copy link
Contributor Author
anntzer commented May 11, 2018

Oh I see, the validators for hspace and wspace were just completely wrong too... killed them as well.

@anntzer anntzer force-pushed the single-subplot-adjust branch 3 times, most recently from 6398d16 to 4e16522 Compare May 11, 2018 22:19
@anntzer
Group figure.subplot.* rc to a single rcParam.
8ce5600 
The idea is to make it easier to restore a figure's subplotparams to the
rc-provided defaults.  Specifically, this can now be done with

    fig.subplots_adjust(**rcParams["figure.subplot"])

or

    fig.subplots_adjust(**dict(rcParams["figure.subplot"], left=...))

if some values need to be overridden.

Also make Figure.clf() restore the subplotsparams to these rc-provided
defaults (that is the original motivation for the change).

Note that this PR runs into a limitation of the rcParams API: it would
be nicer if the validators took the rcParams as parameters (e.g.
implicitly by being methods of the RcParams class), so that they can
actually depend on another key in the same instance.
@anntzer anntzer force-pushed the single-subplot-adjust branch from 4e16522 to 8ce5600 Compare May 11, 2018 23:57
@anntzer anntzer mentioned this pull request Jun 26, 2018
Closed
@jklymak jklymak marked this pull request as draft April 15, 2021 17:49
@anntzer anntzer mentioned this pull request Feb 17, 2023
Merged
5 tasks
@github-actions
Copy link
github-actions bot commented May 6, 2023

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label May 6, 2023
@anntzer anntzer mentioned this pull request May 9, 2023
Merged
5 tasks
@anntzer
Copy link
Contributor Author
anntzer commented May 9, 2023

Just for tracking purposes, the surrounding machinery has mostly all been pr'd separately; the only question remaining is whether we want to group the rcparams into a single dict or not. (I still think this would be a good idea -- probably easiest would be to just start a new pr from scratch.)

@github-actions github-actions bot removed the status: inactive Marked by the “Stale” Github Action label May 10, 2023
@timhoffm
Copy link
Member

the only question remaining is whether we want to group the rcparams into a single dict or not.

I think we should not do this right now. While a single dict would make some things easier, other things get hader. You'll have to write rcParams["figure.subplot"]["wspace"] instead of rcParams["figure.subplot.wspace"], which is more cumbersome and an API change. The dict-base rcParams is not suited to make the transition easy and it also lacks features to support all use cases well.

I suggest to defer this to the redesign of the config system #24585. In particular. we can build a proper hierarchial interface there, which supports both rcParams["figure.subplot.wspace"] and config["figure.subplot"]. With a bit of careful design, the latter could be a dict or dict-like.

@anntzer anntzer closed this May 10, 2023
@anntzer anntzer deleted the single-subplot-adjust branch May 10, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
status: needs rebase topic: rcparams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anntzer @ImportanceOfBeingErnest @timhoffm @jklymak
0