-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix behaviour of Figure.clear() for SubplotParams #27183
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
Conversation
f76c23f
to
adebaa3
Compare
@jklymak Would you be able to review this pr? Thanks |
Why add an additional flag to I am not against the current implementation, but am curious why a more minimal approach would not work? |
@tacaswell This is a continuation of #11086 and I followed the implementation there. Looking at the code right now: the new flag is used both in I am fine with leaving the PR as is, or removing the flag in both |
@tacaswell Gentle ping |
Co-authored-by: Kyle Sunden <git@ksunden.space>
lib/matplotlib/gridspec.py
Outdated
The height of the padding between subplots, | ||
as a fraction of the average Axes height. | ||
""" | ||
for key in ["left", "bottom", "right", "top", "wspace", "hspace"]: | ||
setattr(self, key, mpl.rcParams[f"figure.subplot.{key}"]) | ||
self.update(left, bottom, right, top, wspace, hspace) | ||
|
||
def _repr_pretty_(self, p: Any, cycle: bool) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not simply define __repr__
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__repr__
ends up at many places (logging, inside other repr of containers) and not everybody likes these kind of changes. The _repr_pretty_
is mainly used in interactive environments (spyder, jypyter noteobook, etc.) and visible to humans.
I do not think this change belongs in this PR though, so I removed it. Let me know if you want to to open a separate PR for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's rare that we/users ever access this, so I don't really care. But I think if we do this, we can afford to do __repr__
. We don't have precedence for _repr_pretty_
in the code base. Take or leave a separate PR.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
…b into clf_subplots_adjust
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor changes left.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Jody Klymak <jklymak@gmail.com>
Co-authored-by: Jody Klymak <jklymak@gmail.com>
Co-authored-by: Jody Klymak <jklymak@gmail.com>
You have a stray end of file issue that fails CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small suggestion to the what's new (at the very least the line was too long).
If someone merges this, it should be squash-merged.
Co-authored-by: Jody Klymak <jklymak@gmail.com>
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
PR summary
Figure.clear()
to reset the properties ofFigure.subplotpars
.matplotlib.gridspec.SubplotParams
rc_default
parameter to theSubplotParams.update
Fixes #11059. This PR is a continuation of #11086 by @fredrik-1
PR checklist