FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix behaviour of Figure.clear() for SubplotParams by eendebakpt · Pull Request #27183 · matplotlib/matplotlib · GitHub

Repository navigation

Fix behaviour of Figure.clear() for SubplotParams - #27183

Merged
QuLogic merged 43 commits into
matplotlib:mainfrom
eendebakpt:clf_subplots_adjust
Apr 23, 2025
Merged

QuLogic merged 43 commits into
matplotlib:mainfrom
eendebakpt:clf_subplots_adjust

Conversation

eendebakpt commented Oct 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR summary

  • Update Figure.clear() to reset the properties of Figure.subplotpars.
  • Add representation method for matplotlib.gridspec.SubplotParams
  • Add rc_default parameter to the SubplotParams.update

Fixes #11059. This PR is a continuation of #11086 by @fredrik-1

PR checklist

ksunden marked this pull request as draft October 24, 2023 20:55
eendebakpt changed the title Draft: Fix behaviour of Figure.clear() for SubplotParams Fix behaviour of Figure.clear() for SubplotParams Oct 25, 2023
eendebakpt marked this pull request as ready for review October 25, 2023 09:27

Copy link
Copy Markdown
Contributor Author

@jklymak Would you be able to review this pr? Thanks

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor Author

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?

@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.

github-actions Bot added topic: geometry manager LayoutEngine, Constrained layout, Tight layout topic: pyplot API topic: figures and subfigures Documentation: user guide files in galleries/users_explain or doc/users and removed status: needs rebase labels Jan 16, 2024

Copy link
Copy Markdown
Contributor Author

@tacaswell Gentle ping

eendebakpt requested a review from ksunden January 24, 2024 10:36
github-actions Bot removed Documentation: user guide files in galleries/users_explain or doc/users status: needs rebase labels Feb 27, 2024
Comment thread lib/matplotlib/gridspec.py Outdated
Comment thread lib/matplotlib/gridspec.py Outdated
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:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why not simply define __repr__?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

__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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread lib/matplotlib/gridspec.py Outdated
Comment thread lib/matplotlib/gridspec.py Outdated
Comment thread lib/matplotlib/gridspec.pyi Outdated
Comment thread lib/matplotlib/tests/test_figure.py Outdated
Comment thread lib/matplotlib/tests/test_gridspec.py Outdated
Comment thread lib/matplotlib/tests/test_gridspec.py Outdated
Comment thread lib/matplotlib/tests/test_gridspec.py Outdated
Comment thread lib/matplotlib/tests/test_gridspec.py Outdated

timhoffm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Only minor changes left.

Comment thread lib/matplotlib/tests/test_gridspec.py Outdated
Comment thread lib/matplotlib/tests/test_gridspec.py Outdated
eendebakpt and others added 2 commits April 22, 2025 10:42
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Comment thread lib/matplotlib/gridspec.py Outdated
Comment thread lib/matplotlib/gridspec.py Outdated
eendebakpt and others added 3 commits April 22, 2025 20:22
Co-authored-by: Jody Klymak <jklymak@gmail.com>
Co-authored-by: Jody Klymak <jklymak@gmail.com>
Co-authored-by: Jody Klymak <jklymak@gmail.com>

jklymak commented Apr 22, 2025

Copy link
Copy Markdown
Member

You have a stray end of file issue that fails CI.

jklymak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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>

QuLogic commented Apr 23, 2025

Copy link
Copy Markdown
Member

pre-commit.ci autofix

QuLogic merged commit 00d5cb6 into matplotlib:main Apr 23, 2025
QuLogic mentioned this pull request May 7, 2026
2 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: figures and subfigures topic: geometry manager LayoutEngine, Constrained layout, Tight layout

Projects

None yet

Development

Successfully merging this pull request may close these issues.

figure.clf() and subplots_adjust

7 participants


Back | FazBrowse Home | New Git URL