8000 Prepare to change the default pad for AxesDivider.append_axes. by anntzer · Pull Request #14173 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Prepare to change the default pad for AxesDivider.append_axes. #14173

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 1 commit into from
May 12, 2019
Merged

Prepare to change the default pad for AxesDivider.append_axes. #14173

merged 1 commit into from
May 12, 2019

Conversation

anntzer
Copy link
Contributor
@anntzer anntzer commented May 8, 2019

Using a nonzero default (configurable from the rcParams and identical to
what plt.subplots() use) should make this method a bit more
user-friendly (note that none of the uses of append_axes throughout
the codebase leaves pad to its current default of zero).

Inspired by #13775, which also shows the utility of axes_divider.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 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

@anntzer anntzer added this to the v3.2.0 milestone May 8, 2019
@anntzer
Prepare to change the default pad for AxesDivider.append_axes.
e3d0cc2 
Using a nonzero default (configurable from the rcParams and identical to
what `plt.subplots()` use) should make this method a bit more
user-friendly (note that *none* of the uses of `append_axes` throughout
the codebase leaves `pad` to its current default of zero).
@anntzer anntzer force-pushed the append_axes_default_pad branch from c4b37e5 to e3d0cc2 Compare May 8, 2019 22:57
jklymak
jklymak approved these changes May 9, 2019
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.

These seems good, though I'm quite concerned with our pads being relative to axes sizes that we don't explicitly know a-priori. In hind sight I think it'd have been better if these were in physical units.

@anntzer
Copy link
Contributor Author
anntzer commented May 9, 2019

But if you want to change that you'll have to do it on subplots(), not on axes_divider? (The point of this PR is just to make AxesDivider (and more generally mpl_toolkits) more consistent with the main library.)

@jklymak
Copy link
Member
jklymak commented May 9, 2019

It was just a comment, not something for this PR...

@timhoffm
Copy link
Member
timhoffm commented May 9, 2019 •
edited
Loading

Should we switch the default value from None to 0 as part of the PR?

In both cases, the current plot is not affected. Will just change when the meaning of None is changed to using rcParams.

The difference is in the warnings

Keep None:

  • A user who is not passing pad will receive a deprecation warning.
  • He has to explicitly set it to 0 to silence the warning. If he does so, he will not be notified by the code when the new default is introduced.

Switch to 0:

  • Deprecation warnings will only occur if the user explicitly passes None. We want to notify this case at least.
  • There will be no notification of the default change.

Essentially, it boils down to the intention of the user for using the default:

  • If "I don't care to configure and trust matplotlib to create a good plot with standard values" we should switch to 0 - because that will not bother the user with the deprecation warning.
  • If "I want 0, which happens to be the default, so I don't have to specify it." we should keep None.

The by-default warning is good to inform users that the plot appearance will slightly change, but it's bad because it might nag users who don't care.

@anntzer
Copy link
Contributor Author
anntzer commented May 10, 2019

Well, Matplotlib's standard policy for this kind of changes has been to keep the default to None for now and nag all users (cf. the stem() LineCollection debate).
I think it would specifically benefit mpl_toolkits if it could evolve a bit faster, but I don't really want to fight for a policy change as part of this PR.

@timhoffm
Copy link
Member
timhoffm commented May 11, 2019 •
edited
Loading

There's a difference to the stem/LineCollection debate. There, one can set the future default value to remove the deprecation warning. This is not possible here because we don't have specified a new default yet. People are forced to use the old default.

In the future

stem(...)
stem(..., use_linecollection=True)

will be the same, but

new_horizontal(...)
new_horizontal(..., pad=0)

won't. That's at least something we (and users) should be aware of when we ask them to hard-code something.

@timhoffm
Copy link
Member

Would it be an option to use None as a default (which now resolves to 0 and later to something else)?

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

Well, they can hardcode rcParams[...] in their code right now if they want something that works both before and after.
Right now the default is already None (for append_axes, which is the "main" public API; new_vertical and new_horizontal are clearly just helpers).

@timhoffm
Copy link
Member
timhoffm commented May 12, 2019 •
edited
Loading

Sorry, forget my comment on None. I was mixing things up there.

What actually would work is introducing a default sentinel if people
want to explicitly silence the warning but keep using whatever default.

USE_DEFAULT_PAD = object()

def new_horizontal(self, size, pad=None, pack_start=False, **kwargs):
    ...
    if pad is None:
        cbook.warn_deprecated(
            "3.2", message="In a future version, 'pad' will default to "
            "rcParams['figure.subplot.wspace'].  To keep the current "
            "behavior for all time set pad=0. To silence this warning "
            "and keep using the default now and in the future set "
            "pad=USE_DEFAULT_PAD. You can also ignore this warning "
            "in which case you will automatically use the new defaults "
            "when they are introduced.")
    elif pad is USE_DEFAULT_PAD:
        pad = None

This is fully compatible with the simple behavior: People who do nothing will experince the same deprecation warning as in your code. But for those who want to get rid of the warning but still use the the default, there's an official way.

Alternatively, maybe a bit simpler with pad='default' instead of the sentinel.

@anntzer
Copy link
Contributor Author
anntzer commented May 12, 2019

With the PR as it is, if someone wants to keep the old default and silence the warning they can just pass pad=0, if someone wants to use the future new default and silence the warning they can pass pad=rcParams[...]. USE_DEFAULT_PAD would only help if someone wants to use the old default with old matplotlibs (and even then, only with mpl3.2/3.3) and the new default with mpl3.4 (and then, only until we deprecate and remove USE_DEFAULT_PAD, which will be a mess in itself because it's not a callable so can't trigger (easily) a deprecation on access); overall that seems more complex than it is worth.

@timhoffm
Copy link
Member

Agreed, accepting as is then.

@timhoffm timhoffm merged commit 706051a into matplotlib:master May 12, 2019
@anntzer anntzer deleted the append_axes_default_pad branch May 13, 2019 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
topic: mpl_toolkit
Projects
None yet
Milestone
v3.2.0
Development

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