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

plt.close() not firing close_events on non-interactive backends #18609

Open
drammock opened this issue Sep 29, 2020 · 7 comments
Open

plt.close() not firing close_events on non-interactive backends #18609

drammock opened this issue Sep 29, 2020 · 7 comments

Comments

@drammock
Copy link

drammock commented Sep 29, 2020

Bug report

Bug summary

When using a non-interactive backend, calling plt.close() does not trigger close events. This is a problem for testing / continuous integration; code that works interactively to manage multiple inter-related figures does not work on the CI server.

Code for reproduction

import matplotlib
matplotlib.use('Agg')  # also occurs on SVG, PDF...
import matplotlib.pyplot as plt


def close_callback(event):
    print('close callback was called')


fig = plt.figure()
cid = fig.canvas.mpl_connect('close_event', close_callback)
plt.close(fig)

Actual outcome

No console output with backends agg, svg, and pdf at least (didn't try every single backend).

Expected outcome

With e.g., Qt5Agg backend there is (correctly) a line of console output saying close callback was called.

Matplotlib version

  • Operating system: Linux
  • Matplotlib version: 3.3.1 (via conda-forge). Also happens as far back as version 2.1.2
  • Matplotlib backend: Agg
  • Python version: 3.8.5
  • Jupyter version (if applicable): n/a
  • Other libraries: n/a

A less-minimal example that more closely approximates my use case is here: https://stackoverflow.com/q/64105454/1664024

@anntzer
Copy link
Contributor

anntzer commented Sep 30, 2020

To be honest I'm not really sure the concept of "closing" a non-interactive figure even makes sense. Nothing really changes from the point of view of the figure after you close it, if you keep a pointer to it, e.g.

import matplotlib as mpl; mpl.use("agg")
from pylab import *
f = gcf()
plot()
close("all")
f.savefig("/tmp/test.png")

saves a figure just fine.
The only thing that really happens is that the figure is no longer registered with pyplot, but that's something you can check by inspecting e.g. plt.get_fignums().

Or we could possibly really say close_event really means "not registered with pyplot anymore" (and emit it in Gcf.destroy) but then this breaks the case of FigureManagers being used independently of pyplot... (Note that right now FigureCanvas used standalone (without FigureManagers) already don't send out close_events (except on tk (edit: and gtk3)), and pyplotless embedding using FigureManagers rather than FigureCanvas is relatively rarer (I think), so I think that's a option to consider... or we should fix that to keep animations happy?)

@drammock
Copy link
Author

drammock commented Sep 30, 2020

To be honest I'm not really sure the concept of "closing" a non-interactive figure even makes sense. Nothing really changes from the point of view of the figure after you close it, if you keep a pointer to it

I don't disagree. But since it doesn't raise an error or warning to call plt.close() when a non-interactive backend is in use, wouldn't it make sense that any associated CloseEvents were emitted? Put another way, if you want to take the hard line that "closing a non-interactive figure doesn't make sense" then plt.close() probably should raise an error on non-interactive backends (to be clear, I'm not advocating for taking that hard line; I think it's more sensible to just emit the close events).

To give a bit more context/motivation: my close event listeners are designed to clean up lingering pointers to the figure being closed. There's a parent figure with data, and a few different child figures for changing various settings that affect the data display. The parent figure has pointers to the child figs stored as attributes. When the child figure is closed I want to reset all those attributes to None so that the child figure can be garbage collected. Doing that cleanup in a CloseEvent listener seemed like the obviously right way to do it.

Of course, doing all that with a non-interactive backend is not a typical use case, but (1) having it work the way I suggested makes testing easier by increasing consistency across backends, and (2) so far I don't see a downside to doing it that way (is there a downside I'm not seeing?).

@dopplershift
Copy link
Contributor

dopplershift commented Sep 30, 2020

I agree that having close() run without error but not emit any CloseEvents is a bad middle ground for us to be sitting in.

@larsoner
Copy link
Contributor

larsoner commented Sep 30, 2020

Nothing really changes from the point of view of the figure after you close it, if you keep a pointer to it

To me this seems like an important and relevant "if". In our case (and in other scripts I've written), we don't keep a pointer, so it should get GC'ed after calling plt.close. So this action has a meaningful/valuable effect even when using a non-interactive backend. Since it has some effect, it doesn't seem like it should raise an error to close it, and it seems like it should emit a closeEvent.

@anntzer
Copy link
Contributor

anntzer commented Sep 30, 2020

close_event was originally added in 7fa8ac (and followup PRs) by @dopplershift, I guess to support animations? The only use case in the library is to disconnect the animation timers/callbacks when the figure is closed.

I guess we can tweak the semantics to whatever we want (I don't have a very strong opinion there), but they should hopefully be consistent across use cases. Currently, close_event is not emitted at all for the case of pyplotless canvas embedding in Qt or wx GUIs, because they handle it at the Manager (Qt)/Frame (wx, equiv. to MainWindow) level. This can easily be checked by adding fig.canvas.mpl_connect("close_event", print) to any of the Qt or wx embedding examples. Conversely, Gtk3 and Tk handle that at the canvas level, so handle the embedding case fine. I guess that means that animations + Qt/wx pyplotless embedding may have problems if the animation is still running when the widget is closed? On the other hand it's not even really clear what close_event should mean in an embedding case, because is e.g. temporarily hiding a widget closing it? If not, then perhaps close_event should really be triggered when the canvas is about to be irreversibly destroyed -- but close("all") doesn't do that for the non-interactive case. Alternatively, we could just say that close_event is really only meant to interact with the pyplot case (i.e. it really means "this is getting deregistered from pyplot"), not handle the embedding case at all -- but does that mean we need to look at the animation + embedding case? On the other hand perhaps that case was already not working before, as noted above...

@larsoner I guess if you really want to track object deletion you can use https://docs.python.org/3/library/weakref.html#weakref.finalize.

@dopplershift
Copy link
Contributor

dopplershift commented Sep 30, 2020

Wow, I had no idea that I'm the reason we're in this middle ground 😆 (I guess it was 10 years ago.)

@anntzer
Copy link
Contributor

anntzer commented Oct 5, 2020

Discussion summary: Add a Figure.close() method which only emits a close_event. Have plt.close() call fig.close() (thus emitting the event). In the pyplotless embedding case, document that it is the responsibility of the embedder to call fig.close() to emit the event.

We may also want to revisit the ability of figures being revived after being close()d.

Personal comments: we may additionally want to add a flag checking whether fig.close has been called, and if not, then do so when the canvas gets gc'ed (more or less like what happens right now).

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

6 participants