New configure_subplots behaves badly on TkAgg backend #18299

Activity

1. dopplershift

   added

   GUI: tk

   on Aug 19, 2020
2. dopplershift

   added this to the v3.4.0 milestone on Aug 19, 2020
3. richardsheridan commented on Aug 19, 2020

   

   richardsheridan

   on Aug 19, 2020

   Last edited by richardsheridan

   ContributorAuthor

   More actions

   Testing updates:

   1. Modifying Gcf.destroy to allow figure managers that don't have a _cidgcf attribute and are not in the Gcf.figs dict is simple enough, however that risks regression of Fix RecursionError when closing nbAgg figures. #17025. It seems to me that figure managers should have the responsibility of destroying themselves, and then inform Gcf after the fact. This would possibly prevent some of the weird recursion bugs that keep cropping up? Does anyone know why this indirect strategy was chosen in the first place?
   2. The slider callbacks weren't working because the FigureCanvasBase.callbacks CallbackRegistry dropped them. We have to keep a reference to the output of plt.subplot_tool to keep them alive. (This took way too long to find :( ) Historically, this reference was dangled off the new canvas but in using plt.subplot_tool that becomes very awkward.

   Any suggestions on the design of (1) or the way to keep a reference for (2) would be appreciated, as these are design questions rather than hack-till-it-works issues.
4. tacaswell

   added

   Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

   on Aug 19, 2020
5. tacaswell commented on Aug 19, 2020

   

   tacaswell

   on Aug 19, 2020

   Member

   More actions

   For (2) that is an intentional design choice of how the callback system works motivated by preventing circular references.

   For (1), I'm not sure why we have this inverted logic, it has been than way as long as I have been working on the project (8yrs), I suspect the original motivation may be lost to time.
6. anntzer commented on Aug 20, 2020

   

   anntzer

   on Aug 20, 2020

   Contributor

   More actions

   Thank you for the careful analysis, and apologies for the disruption.

   For 1 it looks like the easy fix is

   diff --git i/lib/matplotlib/_pylab_helpers.py w/lib/matplotlib/_pylab_helpers.py
   index 26ba7a171..27904dd84 100644
   --- i/lib/matplotlib/_pylab_helpers.py
   +++ w/lib/matplotlib/_pylab_helpers.py
   @@ -53,18 +53,17 @@ class Gcf:
            It is recommended to pass a manager instance, to avoid confusion when
            two managers share the same number.
            """
   -        if all(hasattr(num, attr) for attr in ["num", "_cidgcf", "destroy"]):
   +        if all(hasattr(num, attr) for attr in ["num", "destroy"]):
                manager = num
                if cls.figs.get(manager.num) is manager:
                    cls.figs.pop(manager.num)
   -            else:
   -                return
            else:
                try:
                    manager = cls.figs.pop(num)
                except KeyError:
                    return
   -        manager.canvas.mpl_disconnect(manager._cidgcf)
   +        if hasattr(manager, "_cidgcf"):
   +            manager.canvas.mpl_disconnect(manager._cidgcf)
            manager.destroy()
            gc.collect(1)

   i.e. when passed an instance, always destroy it regardless of whether it's pyplot-managed or not (but I agree that long-term we should just let the figures destroy themselves and notify Gcf).

   For 2 it looks like restoring the attachment of the tool on the canvas is easy enough per

   diff --git i/lib/matplotlib/pyplot.py w/lib/matplotlib/pyplot.py
   index f6f9ff4c2..1befccebd 100644
   --- i/lib/matplotlib/pyplot.py
   +++ w/lib/matplotlib/pyplot.py
   @@ -1561 +1561,3 @@ def subplot_tool(targetfig=None):
   -    return SubplotTool(targetfig, tool_fig)
   +    tool = SubplotTool(targetfig, tool_fig)
   +    tool_fig.canvas.tool = tool
   +    return tool

   but I guess I'll take advantage of this to once again question whether weak references really make sense here; we are basically creating an artificial (strong) circular reference between the canvas and the tool (which, like any other cycle, needs the GC to be broken) to work around the fact that the more natural circular reference between the canvas and the callbacks doesn't work, because supposedly we don't want to rely on the GC to break these cycles... (The latter is more natural because they literally need to know each other to work.)
7. jklymak

   closed this as completedin #18306on Aug 24, 2020