| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
@jklymak Would you be able to review this pr? Thanks |
Sorry, something went wrong.
|
Why add an additional flag to update rather than extracting the rcparam values and passing them in clear (where we have to add a call to update anyway)? I am not against the current implementation, but am curious why a more minimal approach would not work? |
Sorry, something went wrong.
@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 clear() and subplots_adjust(...). If we would only modify the clear(), then I fully agree we could do without the flag. If we do add a parameter to subplots_adjust, then the flag in update avoids a bit of duplicated code in clear and subplots_adjust. I am fine with leaving the PR as is, or removing the flag in both update and subplots_adjust, or removing the flag in update while generating the new parameters in both clear and subplots_adjust. |
Sorry, something went wrong.
Co-authored-by: Kyle Sunden <git@ksunden.space>
| 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.
Why not simply define __repr__?
Sorry, something went wrong.
There was a problem hiding this comment.
__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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
…b into clf_subplots_adjust
There was a problem hiding this comment.
Only minor changes left.
Sorry, something went wrong.
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Co-authored-by: Jody Klymak <jklymak@gmail.com>
|
pre-commit.ci autofix |
Sorry, something went wrong.
for more information, see https://pre-commit.ci
| Back | FazBrowse Home | New Git URL |
PR summary
Fixes #11059. This PR is a continuation of #11086 by @fredrik-1
PR checklist