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

Make GridSpec.update docstring match behavior. #13105

Merged
merged 1 commit into from Jan 15, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 4, 2019

The sentence "If any kwarg is None, default to the current value, if
set, otherwise to rc." is clearly wrong given the lines just below,
which overwrite the current value unconditonally:

for k, v in kwargs.items():
    if k in self._AllowedKeys:
        setattr(self, k, v)

This can also be checked with e.g.

from matplotlib.gridspec import GridSpec
from matplotlib import pyplot as plt
gs = GridSpec(1, 1, left=.5)
gs.update(left=None)
plt.figure().add_subplot(gs[0, 0])
plt.show()

where the update reset left to the rc value.

Given that the behavior has been like that ever since gridspec was added
to matplotlib, it seems better to update the docstring.

@jklymak In the same module there's also

    def get_subplot_params(self, figure=None, fig=None):
        """
        Return a dictionary of subplot layout parameters. The default
        parameters are from rcParams unless a figure attribute is set.
        """

but here too the implementation clearly always drops the figure attributes; IOW it always returns a copy of self completely ignoring figure. Given that you last touched this with the constrainedlayout work I don't know what the best approach is, but perhaps we can just get rid of that method...

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

The sentence "If any kwarg is None, default to the current value, if
set, otherwise to rc." is clearly wrong given the lines just below,
which overwrite the current value unconditonally:

    for k, v in kwargs.items():
        if k in self._AllowedKeys:
            setattr(self, k, v)

This can also be checked with e.g.

    from matplotlib.gridspec import GridSpec
    from matplotlib import pyplot as plt
    gs = GridSpec(1, 1, left=.5)
    gs.update(left=None)
    plt.figure().add_subplot(gs[0, 0])
    plt.show()

where the update reset left to the rc value.

Given that the behavior has been like that ever since gridspec was added
to matplotlib, it seems better to update the docstring.
@jklymak
Copy link
Member

jklymak commented Jan 4, 2019

@anntzer, I think all I changed was fig to figure in that call. My memory is fuzzy about what that function does exactly. Happy to see someone simplify things - there are lots of tests of constrained_layout that will fail if something goes awry.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 5, 2019

Re: get_subplot_params: GridSpec.get_subplot_params suffers from the issue above, but GridSpecFromSubplotSpec.get_subplot_params does check for None attributes (but isn't documented to do so).
What a mess, as usual.

@jklymak jklymak merged commit f99ff7e into matplotlib:master Jan 15, 2019
@anntzer anntzer deleted the gridspec-docstring branch January 15, 2019 18:22
@QuLogic QuLogic added this to the v3.1 milestone Jan 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants