Unifying the Figure getter/setter interface to match its constructor #21549

stanleyjs · Nov 5, 2021

PR Summary

Hi,
This is my first matplotlib PR. Should I base it in v3.5.x or base in main? This PR is based in main.

While building some convenience functions to automatically generate figures and/or update the parameters of passed figures, I noticed that figure.set(figsize=(w,h)) did not work, as there was no set_figsize function. I then noticed that subplotpars, figsize, and layout each lack getter/setter methods, and thus they cannot be set naively by calling .set(kwarg) or set_kwarg. This makes functions like the following difficult to write without having individual logic for every figure kwarg. It is better to have a standard interface of getters and setters.

def foo(fig=None,fig_kwargs):
    if fig is None:
        fig = plt.figure(**fig_kwargs)
    else:
         fig.set(**fig_kwargs)
    return fig

Edit: This PR also refactors how tight_layout and constrained_layout are set. It places all of the logic inside a set_layout function, which handles passing layout as a string (eg layout = "tight"), as a bool (eg tight_layout=True), and passing in padding parameters (eg layout = "constrained", constrained_layout={"param":"val"})

PR Checklist

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (run flake8 on changed files to check).
New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs should build without error).
Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

…nts. refactored layout setting. added ability to change subplotparams post hoc. added figsize getter/setter convenience function in order to match the constructor signature of figure. added tests.

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

anntzer · Nov 6, 2021

lib/matplotlib/figure.py

@@ -2684,6 +2888,60 @@ def get_size_inches(self):
        """
        return np.array(self.bbox_inches.p1)

+    def set_figsize(self, w, h=None):


This should probably just use cbook._define_aliases (see other uses in the library).

Thanks @anntzer. I made the requested change in the newest commit. Also updated the tests to match pep8.

jklymak · Nov 22, 2021

I'm not convinced by the motivation here. When do we want to pass a figure to a plotting method while at the same time wanting the user to be able to control layout parameters? It seems the only reason to pass a figure to a method is if we want to control the layout in our method. Can you justify this a little more?

Overall the new set_layout seems very long, and I'm not convinced we want to encourage set_layout after the figure is instantiated. In particular, colorbars need to know what layout is being used before they are created (for historical reasons, but they would be hard to remove at this point).

tacaswell · Nov 22, 2021

I think the motivation here is reasonable. We already moved to a single layout kwarg (preferred over tight_layout and constrained_layout). Coupled with #20426 , I think providing a way for users to after-the-fact change the layout engine is an obvious feature (think of the case of using xarray / pandas to make most of a figure and then tweaking it up).

@stanleyjs You have taken on an ambitious project for your first PR, anything extending (let alone changing) the API tends to result in very verbose discussions (because a lot of our day-to-day pain in related to APIs that in in 20/20 hind sight were not the best). Please bear with us while we work through the process.

jklymak · Nov 22, 2021

Yes but my point is that currently you cannot tweak it up after the fact if you have used a colorbar so an explicit setter will fail

stanleyjs · Dec 15, 2021

Hi just wanted to bump this and see what the timeline is moving forward.

A few things.

The primary motivation and concern of this PR was to increase uniformity in the API between the figure constructor and its actual setters/getters. In the current codebase, downstream figure instantiation / manipulation has to be done through special cases, as, for example, setting figsize post instantiation must be translated to set_size_inches. This is confusing.
I'll admit that set_layout is a little bit long. It had four requirements:
a) Accomodate all of the current API, which includes both modern layout and legacy tight_layout + constrained_layout.
b) Abstract the current instantiation code into its own method
c) Allow for subsequent modification of the layout parameters
d) Raise the correct set of warnings for undefined behavior.
As a result, the method suffers a bit and could be easily refined by removing support for the legacy attributes.
At least in my applications, which may not be matplotlib parlance, I find myself often setting constrained or tight_layout before and after making colorbars and legends
If set_layout is holding this PR up, I think we should at least push through the other new aliases.
Is it worth looking into how to make colorbars in a way that works independent of the order that the layout is set? I can look into this for a separate PR.

timhoffm · Dec 16, 2021

I will look into this from the API consistency point of view. But I'll take some time because I don't have much time for matplotlib currently. There are always tradeoffs between API improvements, compatibility with the existing API and redundant ways to do the same thing. Usually, you cannot have all.

jklymak · Dec 16, 2021

5. Is it worth looking into how to make colorbars in a way that works independent of the order that the layout is set? I can look into this for a separate PR.

I have looked into this extensively, and its certainly possible, but not in a back-compatible way.

Conversely you could use the current design, pull the colorbar out, hoist the parent axes to their original gridspec, and redo the layout (or vice versa), but that seems really brittle and not worth the bother.

I'm not convinced there is a great use case to toggle the two layout managers that would justify adding all the extra cruft needed to make this work, particularly given how poorly tight_layout works with colorbars in the first place. In my (very biased) opinion if you have colorbars you should be using constrained_layout because it was specifically designed to work around the issues with colorbars in tight_layout.

jklymak · Feb 1, 2022

There have been substantive changes to figure.py that cover these layout methods. I'm still not convinced this re-arrangement is a substantial improvement, but others may have a different opinion. It has acquired the need for a rebase. I'll leave active for others to comment on. I think we were waiting for @timhoffm to weigh in.

stanleyjs · Feb 7, 2022

@jklymak while I still believe that it doesn't make any sense for certain constructor arguments to not have a corresponding set_arg method, I think that is a separate thing from attempting to restructure the layout interface/construction. For that reason, I'm open to closing this, but I think a new, smaller PR would be reasonable to at least alias set_figsize to set_figsize_inches

timhoffm · May 19, 2022

a new, smaller PR would be reasonable to at least alias set_figsize to set_figsize_inches.

We currently have

Figure(figsize=...)
Figure.get_figwidth()
Figure.get_figheight()

So, yes I agree that we should have set_figsize() and get_figsize(). @stanleyjs sorry for the delay in reviewing. Are you willing to open a PR for that?

stanleyjs · Jun 2, 2022

@timhoffm yes. Sorry for the delay. I will try to get this on the list for this weekend.
Jay

timhoffm · Sep 8, 2022

@stanleyjs are you still interested in opening a PR?

tacaswell · Dec 4, 2022

Thank you for your work on this @stanleyjs ! I created #24617 writing up the more limited version of this PR and am going to close this. Working code is a good articulation of requirements, but I opted to open a new issue in favor of closing this PR to make it more likely for someone else to pick up this work.

Murad039 · May 7, 2023

hmm

Updated with original test from matplotlib#21549 written by @stanleyjs Co-Authored-By: Jay Stanley <stanleyjs@users.noreply.github.com>

stanleyjs added 6 commits November 5, 2021 15:48

Updated figure to have getters and setters for all constructor argume…

fc18909

…nts. refactored layout setting. added ability to change subplotparams post hoc. added figsize getter/setter convenience function in order to match the constructor signature of figure. added tests.

docs

2775180

added more test conditions for layout function

cc4f860

doc refinements, more tests

55d715c

pep8/flake8

781d0e8

doc formatting

09b931b

github-actions bot reviewed Nov 5, 2021

View reviewed changes

anntzer reviewed Nov 6, 2021

View reviewed changes

pep8/flake8 tests. cbook._define_aliases for figsize

1d324c4

tacaswell added this to the v3.6.0 milestone Nov 8, 2021

timhoffm added the API: consistency label Dec 16, 2021

timhoffm self-assigned this Dec 16, 2021

QuLogic modified the milestones: v3.6.0, v3.7.0 Aug 24, 2022

github-actions bot added the status: needs rebase label Oct 18, 2022

tacaswell mentioned this pull request Dec 4, 2022

[ENH]: Add setter/getter methods for all keyword parameters to Figure.__init__ #24617

Open

tacaswell closed this Dec 4, 2022

garveyz2 mentioned this pull request Dec 11, 2022

Addresses issue #24617 adding setters/getters for Figure #24696

Closed

5 tasks

dutta712 mentioned this pull request Jun 2, 2023

Getters setters #25901

Open

Lambxx mentioned this pull request Nov 2, 2023

Fixes : Add setter/getter methods for all keyword parameters to Figure.__init__ #24617 #27257

Open

2 tasks

Lambxx added a commit to Lambxx/matplotlib that referenced this pull request Nov 7, 2023

Update test_figure.py

04c0f04

Updated with original test from matplotlib#21549 written by @stanleyjs Co-Authored-By: Jay Stanley <stanleyjs@users.noreply.github.com>

zaheerhashmi mentioned this pull request Dec 16, 2023

Resolving: Add setter/getter methods for all keyword parameters to Figure.__init__ (Issue: #24617) #27531

Draft

5 tasks

AlexisMardas mentioned this pull request Mar 31, 2024

Add missing getter/setter methods to keywords of Figure.__init__ as described in #24617 #27997

Open

5 tasks

alexjvd4 mentioned this pull request Feb 17, 2025

Added figsize and layout getters and setters for Figure.__init__ #29632

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unifying the Figure getter/setter interface to match its constructor #21549

Unifying the Figure getter/setter interface to match its constructor #21549

stanleyjs commented Nov 5, 2021 •

edited

Loading

github-actions bot left a comment

anntzer Nov 6, 2021

stanleyjs Nov 8, 2021

jklymak commented Nov 22, 2021

tacaswell commented Nov 22, 2021

jklymak commented Nov 22, 2021

stanleyjs commented Dec 15, 2021

timhoffm commented Dec 16, 2021

jklymak commented Dec 16, 2021 •

edited

Loading

jklymak commented Feb 1, 2022

stanleyjs commented Feb 7, 2022

timhoffm commented May 19, 2022

stanleyjs commented Jun 2, 2022

timhoffm commented Sep 8, 2022

tacaswell commented Dec 4, 2022

Murad039 commented May 7, 2023

Search code, repositories, users, issues, pull requests...

Unifying the Figure getter/setter interface to match its constructor #21549

Unifying the Figure getter/setter interface to match its constructor #21549

Conversation

stanleyjs commented Nov 5, 2021 • edited Loading

PR Summary

PR Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

anntzer Nov 6, 2021

Choose a reason for hiding this comment

stanleyjs Nov 8, 2021

Choose a reason for hiding this comment

jklymak commented Nov 22, 2021

tacaswell commented Nov 22, 2021

jklymak commented Nov 22, 2021

stanleyjs commented Dec 15, 2021

timhoffm commented Dec 16, 2021

jklymak commented Dec 16, 2021 • edited Loading

jklymak commented Feb 1, 2022

stanleyjs commented Feb 7, 2022

timhoffm commented May 19, 2022

stanleyjs commented Jun 2, 2022

timhoffm commented Sep 8, 2022

tacaswell commented Dec 4, 2022

Murad039 commented May 7, 2023

stanleyjs commented Nov 5, 2021 •

edited

Loading

jklymak commented Dec 16, 2021 •

edited

Loading