Keep track of axes in interactive navigation. #9359
Conversation
|
Did you try the second commit (which should avoid some bad memory leaks)? |
|
@anntzer no, I was too quick to pick that up. And I wouldn't have noticed leaks anyway. But if you need me to try it I can. I'm also happy to try the finished PR. |
|
Please do. |
|
Looking at this again I actually think the semantics of the home button are not very well defined when you start dynamically adding and removing axes (what do you do if the original axes are not there anymore?). This PR takes the view that we just don't do anything if an axes had been removed, and leave access that were not present as is (so one should strive to reuse axes if you want to keep the navigation information available). The old approach was to match axes by their index, which seems definitely more brittle once you start swapping axes around... |
|
@anntzer I will try to test the patch within the next couple of hours -- tied up at the moment. I didn't write the original code I'm working with but I don't think it is dynamically adding/removing axes. Looks like it just resets the x/y limits and the labels and reuses everything. I assume this was an attempt to be more efficient -- maybe that doesn't actually pay. When you load a new plot it should be as if you discarded and created a new plot object from the user's point of view, so the semantics of the Home, Back, etc. buttons seem to be well defined. The reported bug appears when using the same axes updated in this way to plot a new set of curves. I dumped the value of |
|
The problem is if you started with just one axes and then dynamically add another one (but keep the first one too): do we drop the nav info for the first axes (i.e. is home defined by where the first axes actually started, or by where it is at the time the second axes is added)? I don't have a very strong opinion there (other than considering that matching by index is definitely brittle in this case). @tacaswell want to comment? |
37fe37a to
cffeef0
Compare
|
I confirm this also fixed the problem I had as noted in #9358. |
|
This is milestoned for the next bug fix release which will be tagged when everything is merged. This PR is currently waiting for review. @2sn Could you do a careful code review of these changes? That will help expedite the process. |
| @@ -2799,19 +2797,24 @@ def __init__(self, canvas): | |||
| self.set_history_buttons() | |||
|
|
|||
| @partial(canvas.mpl_connect, 'draw_event') | |||
There was a problem hiding this comment.
So we're processing the navigation stack on every draw event? I'm concerned about potential performance of all the extra set and stuff. I realize we're already slow, but kinda hoping to not make things worse.
There was a problem hiding this comment.
Alright @tacaswell convinced me that this isn't going to be the slowest part.
Backport PR #9359 on branch v2.1.x
xref #9358.
@DeadParrot can you check that this patch fixes the issue for you?
I believe the old (2.0) code only "worked" if the axes you delay-added happened to have the same limits as the original ones (otherwise where did the info for the new axes get pushed?).
May want to switch to using weakrefs as preventing axes from being gc'd can be quite expensive...
EDIT: second commit switches to weakrefs.
PR Summary
PR Checklist