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

Allow the user to specify a figure to reuse when creating a FacetGrid #2830

Open
Erotemic opened this issue Jun 2, 2022 · 10 comments
Open

Comments

@Erotemic
Copy link

Erotemic commented Jun 2, 2022

Currently FacetGrid and its consumers relplot and catplot will always create a new figure when you call them. In my workflow, I often like to create the figures or axes beforehand and then pass these as arguments to seaborn functions. This works with axes level plots line lineplot and scatterplot, where you can pass the ax parameter, and if you don't it uses plt.gca().

I would like to propose that FacetGrid should take a parameter fig=None, and if specified it uses it, otherwise it uses the existing code as such:

if fig is None:
        with mpl.rc_context({"figure.autolayout": False}):
            fig = plt.figure(figsize=figsize)

That would allow me to force specific plots to be associated with specific figure numbers, which helps me keep track of things as I work. I can make this PR if there is interest in this feature. I made a draft here: #2831 and I will work on the remaining bullet points if there is interest.

@jhncls
Copy link

jhncls commented Jun 2, 2022

How does this option work together with the height= and aspect= parameters? How are a variable number of columns and rows handled? Will pre-existing subplots and other elements be erased? How will the extra space for the figure legend be created?

@mwaskom
Copy link
Owner

mwaskom commented Jun 2, 2022

This has definitely come up before, I think I have articulated some thoughts. Basically, making this possible is straightforward; your approach is the right one. The blocker has always been thinking through all the edge cases that would result from enabling this, given that FacetGrid was written with the assumption that it would always own its figure.

@jhncls hits on several of the important points. I assume that the main usecase here is when working with a subfigure, (I am not sure I see a need for it otherwise, except maybe to use a figure that is not plugged intopyplot, e.g. in a GUI interface)?

The main thing is the fact that FacetGrid and PairGrid put the legend outside the space of subplots and resize the figure that they are in to make space for it They also call tight_layout, using a bounding box that assumes the grid is the only thing in the figure. Neither operation would work well in a subfigure context.

The edge cases around the size parameters in the constructor would take some care — you'd want to fire a helpful warning if they are used, but they current have non-null default values. This adds some complexity, although I think the API decisions are reasonably straightforward.

There may be other considerations I am not immediately thinking of.

Another thing to note: this would need to be rolled out to the other Grid objects as well, and the figure-level functions that use them should probably get the fig parameter as well. So it would be a not-insubstantial amount of work.

@Erotemic
Copy link
Author

Erotemic commented Jun 2, 2022

I'll take a look at these cases and try to formulate a coherent plan. I think the general strategy is going to be: if the user is specifying the figure, they are responsible for setting it up in a "clean" way, and if they don't then results may not be expected (although I agree it's a good idea to fire off a warning if we can detect these cases).

For all API decisions, the defaults when fig=None will be functionally unchanged.

For height and aspect they could either:

  • continue working as is, and the user's figure is resized unless they specifically disable them
  • change them to an "auto" default, which has different behavior based on if fig is specified or not.

I'm in favor of the "auto" approach, and if they are specified as numbers they are always applied. I think the same approach will work with calling "tight_layout".

The legend issue is a tricky one that I will have to think about, but I think it can be dealt with in a sensible way.

I assume that the main usecase here is when working with a subfigure, (I am not sure I see a need for it otherwise, except maybe to use a figure that is not plugged intopyplot, e.g. in a GUI interface)?

That is a use case. But I simply want to control the number of figures on my screen by assigning them all numbers. I don't want FacetGrid to always pop open a new figure. I want it to reuse an existing one. I don't think I care if it clears it. I might care if it is resized, but it probably isn't that big of a deal.

Another thing to note: this would need to be rolled out to the other Grid objects as well, and the figure-level functions that use them should probably get the fig parameter as well. So it would be a not-insubstantial amount of work.

I'll probably work on this on and off. I think I care enough about it where I'm not going to abandon it, but I'm also more interested in just having the feature rather than working on implementing it. So if anyone wants to get to it before I do that would be helpful.

@mwaskom
Copy link
Owner

mwaskom commented Jun 2, 2022

if the user is specifying the figure, they are responsible for setting it up in a "clean" way, and if they don't then results may not be expected

Easy enough to say, but you're not going to be the one dealing with the bug reports ...

@Erotemic
Copy link
Author

Erotemic commented Jun 3, 2022

but you're not going to be the one dealing with the bug reports ...

You can always @ me 😉

More seriously, I do think it's the right approach and I think we can set this up in a way to minimize foot-shooting.

@mwaskom
Copy link
Owner

mwaskom commented Jun 4, 2022

To be clear — I am in favor of adding this functionality (and it already exists in the new objects interface). I just expect it to take some careful thought about how to handle all the edge cases.

@davidgilbertson
Copy link
Contributor

@Erotemic I'm curious, would it suit your use case if you could pass in something like fig_kws to FacetGrid (and the figure-level plot functions)? That allows you to specify the num and clear args. I logged this a few days ago (#2868), before seeing your issue just now.

Does that approach strike a good balance between allowing the user some control over the figure creation, without opening the door to likely problems?

@Erotemic
Copy link
Author

Yes, that would likely be sufficient.

@davidgilbertson
Copy link
Contributor

@Erotemic the below is a horribly hacky workaround, but it will allow you to open one figure window, and the re-execute your code and have it re-use that same figure window.

It hijacks the call from Seaborn into plt.figure() and adds in num and clear properties. You could alternatively just have it return a figure you've already created (after setting the figsize that Seaborn passes through).

if "figure_original" not in locals():
    figure_original = plt.figure

    def figure_new(**kwargs):
        import inspect

        if "seaborn" in inspect.stack()[1].filename and "num" not in kwargs:
            print("Seaborn calling plt.figure()")
            kwargs["num"] = "Seaborn Figure"
            kwargs["clear"] = True
            
        return figure_original(**kwargs)

    plt.figure = figure_new

@Erotemic
Copy link
Author

Erotemic commented Aug 31, 2024

I just started using self.sns.jointplot ran into this issue, started writing up an entire new issue and though to myself: hmm, this feels familiar, and then I remembered that I made this issue 2 years ago. I didn't see it in my search-for-similar-questions because I'm hitting it in the context of seaborn.jointplot and JointGrid now.

Man... I just want to control the figure before I send it to seaborn. I really don't like that monkey patch. I want my tools to interoperate with others, and if I monkey patch pyplot something is going to break.

But yeah, just passing fig_kws would be enough. I'd be happy to submit the PR, I just need to know that it would be accepted.

I made a slightly longer version of the monkey patch that uses a context manager to clean up after itself that I'm going to try in the meantime:

class MonkeyPatchPyPlotFigureContext:
    """
    😢 🙈 😭

    Forces all calls of plt.figure to return a specific figure in this context.

    References:
        ..[Seaborn2830] https://github.com/mwaskom/seaborn/issues/2830

    CommandLine:
        TEST_MONKEY=1 xdoctest -m kwcoco.cli.coco_visual_stats MonkeyPatchPyPlotFigureContext

    Example:
        >>> # xdoctest: +REQUIRES(env:TEST_MONKEY)
        >>> from kwcoco.cli.coco_visual_stats import *  # NOQA
        >>> import matplotlib.pyplot as plt
        >>> func1 = plt.figure
        >>> self = MonkeyPatchPyPlotFigureContext('mockfig')
        >>> with self:
        >>>     func2 = plt.figure
        >>> func3 = plt.figure
        >>> print(f'func1={func1}')
        >>> print(f'func2={func2}')
        >>> print(f'func3={func3}')
        >>> assert func1 is func3
        >>> assert func1 is not func2
    """
    def __init__(self, fig):
        from matplotlib import pyplot as plt
        self.fig = fig
        self.plt = plt
        self._monkey_attrname = '__monkey_for_seaborn_issue_2830__'
        self._orig_figure = None

    def figure(self, *args, **kwargs):
        """
        Our hacked version of the figure function
        """
        return self.fig

    def _getmonkey(self):
        """
        Check if there is a monkey attached to pyplot
        """
        return getattr(self.plt, self._monkey_attrname, None)

    def _setmonkey(self):
        """
        We are the monkey now
        """
        assert self._getmonkey() is None
        assert self._orig_figure is None
        # TODO: make thread safe?
        setattr(self.plt, self._monkey_attrname, 'setting-monkey')
        self._orig_figure = self.plt.figure
        self.plt.figure = self.figure
        setattr(self.plt, self._monkey_attrname, self)

    def _delmonkey(self):
        """
        Get outta here monkey
        """
        assert self._getmonkey() is not None
        assert self._orig_figure is not None
        setattr(self.plt, self._monkey_attrname, 'removing-monkey')
        self.plt.figure = self._orig_figure
        setattr(self.plt, self._monkey_attrname, None)

    def __enter__(self):
        current_monkey = self._getmonkey()
        if current_monkey is None:
            self._setmonkey()
        else:
            raise NotImplementedError('no reentrancy for now')

    def __exit__(self, ex_type, ex_value, ex_traceback):
        if ex_traceback is not None:
            return False
        self._delmonkey()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants