GeezerVison

This is a relatively large behaviour change. Perhaps for the better, and I'd definitely support this as a default. However, is there a way for users to get the old behaviour and still keep the axes transparencies as they were? It seems to me that we need a manual toggle for all of this?

Perhaps a contrived example, but it's possible I make a plot and know that a useful portion of the main axes is covered by the inset and hence zoom over the inset expecting the main axes to re-center. It is also quite likely that an inset is showing a fixed portion of the main axes and I don't want to allow a zoom on it, in which case the zoom may as well pass through.

I'm not particulalry opposed to this solution, but want to make sure it is though through.

@jklymak of course... I tried to give this as much thought as i could 😄

So the old behaviour would mean that irrespective of background and zorder, you always trigger all axes... not sure anyone really wants to go back to that.

• If I really want the inset-axes to have a background AND trigger other axes (whether they are below or not), using sharex=ax and/or sharey=ax would be the natural way to achieve this.

• correctly connecting the zoom of an inset-axes to a parent axes is a bit more difficult...

  • you'd need to ensure that the inset-axes has the same projection
  • you'd have to identify the clicked point in data-coordinates and adjust the zoom and pan on the other axes accordingly
    Note: the current behaviour does not zoom the inset-axes and the axes below in the desired way! they just both take the position of the click-event in the axes and zoom accordingly...

• NOT triggering an axes and just passing the zoom events to axes below is not possible in the current behaviour and it is still not possible. (I guess implementing it would indeed require an additional flag for axes... such as disable_zoom or disable_pan or something like that)

I guess I'm in favour of a reasonable default as proposed here, but also an explicit state on the Axes that can be queried and set. Probably pan needs to be included?

Certainly I don't think a zoom in one axes need be tied to anything on the other axes. We just encouraged that functionality in an external package for now.

@jklymak
... I've once tried to implement a shared zoom functionality for cartopy myself, but I stopped since it can get quite messy with geo-axes at different projections...

you mean a state-variable for disabling zoom&pan on an axes right? I could add it, but I'd need someone to point me to the right location for that. (also I don't know what your naming-conventions for such a variable would be) Maybe it's better to open an issue and do this in another pull-request?

My personal opinion is we should figure out the api for allowing the user to control the pass through before we change the behaviour in a way the user can't get back to the previous behaviour. We would only do that if we were sure the old behaviour was definitely wrong.

So I think the API would be:

axes._captures_navigation

• None: default as proposed here, and sentinel that the user hasn't explicitly set this.
• True: stop further propagation down axes list.
• False: allow event to continue down the stack (current behaviour).

Then we just need a set/get_captures_navigation pair of methods.

This would live on axes/_base.py

@jklymak
Hey,
I'd be willing to implement this, but I have 2 general points here that I think might be worth considering beforehand:
(I'm not a matplotlib dev but I hope my opinion is welcome...)

• First, if a new attribute is introduced, I'd suggest that it also directly allows to completely disable navigation on an axis (and then automatically forward the events to axes below)

  • maybe a more general ax._navigation attribute that accepts values like: [None, "forward", "capture", "disable"] ?

• Second, I'm not sure if the "continue down the stack" option is really useful at all (not even for transparent axes)
  (for twinned-axes yes, but they can always be handled explicitly)
  Note... events are not "continued" with respect to data-coordinates, the "continuation" only propagates the mouse-position to the other axes!

  ... in other words... here's what the "continue down the stack" option does with axes that are not the same size:

  • when you try to zoom-in on a rectangle in an inset-axes, you simultaneously zoom-in to the very same rectangle on all other axes (which is an area effectively located behind the inset-axes !)
  • the effective size of the zoomed-in area in the "continued-axis" however is dependent on the relative size between the "continued-axis" and the axis the user selected as basis for his mouse-movement (which intuitively is always the topmost axes)
    • the very same scaling issue happens also if you execute a "pan" on the inset-axes

... here's what I mean as a gif:

import matplotlib.pyplot as plt
import numpy as np
data = np.mgrid[0:30, 0:30]

f, ax = plt.subplots()
ax2 = f.add_axes((.25,.25,.5,.5))
ax.scatter(*data, c="C0")
ax2.scatter(*data, c="C1")

Note... events are not "continued" with respect to data-coordinates, the "continuation" only propagates the mouse-position to the other axes!

Thats understood, but replacing an explicit (though perhaps undesirable) behaviour with an implicit behaviour depending on murky heuristics is how we get into trouble unless we allow the user an escape hatch to specify things explicitly. Given that no one has complained about the existing behaviour until now (that I know of) makes me a bit skeptical that we can just discount the old behaviour as a bug.

Just a quick drive-by comment: there's already ax.set_navigate(False), which can disable pan/zoom on an Axes.

@anntzer thx, don't know how I've missed that... (I've indicated the function in the See Also section of the ax.set_captures_navigation doc)

@jklymak OK, I get your concerns here... I've now implemented the API as suggested!

Note however that there's not yet a way to get back to the current behavior globally... so at the moment, if you really need the old behavior, you'd have to set the navigation-capture mode for each axes individually...

Should axes.captures_navigation=None be ingested into rcParams so that users can override the global default?

Should axes.captures_navigation=None be ingested into rcParams so that users can override the global default?

I wouldn't unless there is an outcry. To be clear, I support the proposed default behaviour, and think it is what most people would prefer, so I think its OK to ask the few folks who may object to set the state manually on each axes.

@jklymak OK, sounds good to me! ...then I think from my side I did what I can to push this forward...
(I have no clue why the docs are failing now, but it looks more like a CI related issue than something connected to the changes here)

Yes, if you rebase, the docs should build again. Setuptools broke us.

BTW: this definitely needs an API behaviour change note. /docs/api/next_api/behavior

BTW: this definitely needs an API behaviour change note. /docs/api/next_api/behavior

@jklymak I added a note to /next_api_changes/behavior/22347-RQ.rst

hey, before this gets lost in time...
Anything else you'd need from my side to merge this? (I tried another rebase but somehow the docs still don't build)

Try getting rid of leading spaces on lines 9 and 10. That should solve the doc problem. In the release note, that is.

(For the PR itself, I cannot really say.)

I believe the doc failure is that you need to add your new functions to doc/api/axes_api.rst.

hey all,
so I did another rebase and implemented all suggestions but to be honest, I really have no clue why the docs still fail...
would be nice if anyone from your side could have a look at this because I think aside of the docs it's ready

ok... now I'm puzzled... the branch is on top of main and contains a single commit with all changes.
However, tests fail with AttributeError: 'Axes' object has no attribute 'get_capture_navigation_events which is clearly not true if you look at the changes in axes/_base.py... I tried closing+reopen of the PR to re-trigger the tests but it didn't help...

Hey all, before this gets lost in time, I'll give it another try...

• What's still missing to get this merged?
  • are there general concerns on the implementation?
  • functionalities not working as expected?

From my side, the tests are now quite general and all works as expected.

Sorry this is dragging @raphaelquast - your perseverance is very much appreciated. I think you still have some actionable comments from Greg above. He is usually great about re-reviews, so feel free to ping!

Hey @jklymak,
Thanks for your follow up on this one! (and @greglucas, thanks for your comments!)
It's not easy to keep track on such long-term PRs but I think we're getting close...

I got a bit carried away by other tasks recently, but I'll do my best to find some time to finalize this as soon as possible.
(The good thing is, I do have a new motivation to fix this since the upcoming eomaps v7 release will allow zoomable inset-maps.)

To summarize, the remaining steps are:

Someone has to decide this once and for all:

• agree on default arguments for get_capture_navigation_events ("auto" or None?)
  • None seems to be the commonly used default, but in this case it also somewhat hides the fact that there is a special capture logic applied by default (e.g. based on the background patch visibility)
  • auto would be at least a little bit more descriptive (to avoid confusions with False)

In the end I think this is a very minor problem since almost nobody will tinker with this option anyways...
My preference would be to stick to "auto" but I understand the need for consistency, so if you prefer None, I have no objections.

TODOs:

• refactor code to avoid duplications (e.g. handling of pan/zoom events is quite similar)
• simplify example

I would lean to "auto" here, but we should put this on the agenda for next week.

Sorry for only drive-by reviewing this, but per #22347 (comment) is this distinct enough from set_navigate?

@jklymak At the moment, ax.set_navigate turns off navigation completely and ax.set_capture_navigation_events handles relaying of navigation events to axes below.

It would be possible (and maybe actually preferable?) to supercharge ax.set_navigate with more input options instead of introducing a new method... something like:

• True: use "auto" capture behavior
• "capture": capture navigation events
• "relay" or "forward"? : relay navigation events
• False: turn navigation off

Would that give you all the flexibility you need, solve the simplest case, and be relatively easy to explain to users? If so, maybe that is a good way to go forward?

Edit: just to be clear, not blocking on this, just wanted to make sure it had been considered.

OK, I invested some time to make the axes-trigger logic easier to follow and I simplified the example.
With these changes I think the PR is ready for a new review.

• tests are currently failing due to issues with the codecov upload

Remaining open points for discussion:

• Is this distinct enough from set_navigate?

  @jklymak After giving this a bit of thought, I think yes... To clarify:

  Supercharging ax.set_navigate with additional arguments actually has an unwanted side-effect:

  It would mean that ax.get_navigate no longer returns a bool.
  While this is most probably a very minor issue, it would break any code that uses if ax.get_navigate() is True.
  Therefore I stayed with using ax.set_capture_navigation_events so far.

• Use auto or None as default?
  auto is currently used as default argument for ax.set_capture_navigation_events.
  This choice was made since it more clearly indicates that there is a special logic used by default.

Finally, I realized the more general reason why some of the tests were failing at one stage:

• shared axes (e.g. ax.sharex()) are triggered automatically with the parent
• twinned axes (e.g. ax.twinx()) are NOT -> (e.g. also twinned axes of shared axes are not triggered automatically)

At the moment I've implemented explicit triggers of any relevant twinned-axes to handle this,
but I think this should actually be resolved more generally by reviewing why twinned axes are not handled
in the same way as shared axes. (I mean those lines)

However, I'd prefer not to tackle this in this PR to avoid any more confusion.
(I tried to be as clear as possible in the comments to aid any follow-up PR)

Two comments from today's call:

1. change the same of set_capture_events to set_forward_events and invert the logic. The docs explaining this start with the word "forward" so it is a hint!
2. could you write an "unrolled" version of example (in addition to kitchen sink example)? The kitchen sink example is good for testing but not good for understanding.

Hey, thanks for the follow-up on this!
(And sorry for not being able to join the meeting...)

I can try to implement this in the next few days, but I'll be afk for all of August,
so not sure I manage to find enough time to finalize this beforehand.

1. OK I can do that! ✔️ Done.
2. Yes, I can do that as well, but I need some clarifications first...
   • Which one is the kitchen sink example now?
     The current (stripped down) one or the initial (extensive but a bit confusing) one?
   • Not really sure what you mean by an "unrolled" example?

First, thanks @greglucas for the thorough review! I've implemented all minor comments, but the final major one actually went down a rabbit-hole and I'm slowly starting to understand the initial comment of @timhoffm when I first posted the issue more than a year ago 😄

"So this might be a bit more difficult to clean up."

OK, now to the actual problem:

One major issue is that right-click zoom (i.e. zoom out) does not work on the lower two axes of your example. I get: ValueError: Axis limits cannot be NaN or Inf (I also only get this in the "y-direction" when holding "x" everything seems to work as expected, but holding "y" throws the ValueError

It turns out the origin of this is hidden a bit deeper in the code... To clarify:

• The error is triggered when trying to zoom twinned axes of shared axes.
• The reason why this is not a problem in the current mpl version is because twinned axes of shared axes are completely ignored by pan/zoom events.

I have implemented explicit triggers for all twin-axes (see here), which revealed that there's a bit more to fix to get this working properly...

My guess is that things go wrong because the event-position (event.x, event.y) is used to evaluate the zoom rectangle which is problematic if we want to zoom a twin-axes of a shared axes that is located somewhere else...)
(pan-events transform to data-coordinates first which seems to fix this (as long as the transformed values are still finite) (see here)

As I see it there are 2 ways to solve this:

• handle zoom events in data-coordinates (might cause problems in non-rectilinear projections??)
  (I'd consider this more a "fix" than a solution...)
• make sure that only the axes that is clicked evaluates the pan/zoom event and all other axes that share x-y with it simply
  inherit the new extent (and not trigger the whole pan/zoom machinery at all)

The question also remains if this should be part of this PR or if we let twin-axes of shared axes remain ignored for now and postpone it until someone actually complains about it...

• e.g. simply removing those lines would do the trick

make sure that only the axes that is clicked evaluates the pan/zoom event and all other axes that share x-y with it simply
inherit the new extent (and not trigger the whole pan/zoom machinery at all)

I haven't given this a lot of thought, but this seems like a good approach. If I have located an axes somewhere else and want to "share" limits, then I wouldn't expect a whole new pan/zoom trigger, because the event may not be contained in that axes, so sending a set_xlim() etc... seems to make sense to me.

One thing to think about here is if it would make sense to do something along the lines of with callbacks.blocked(): to block some of the update triggers from firing when you are currently working a pan/zoom event. We do this in a few other places with the norms so that we don't trigger infinite update loops.

Did you mean to close this?

@ksunden NO definitely not.. I actually wanted to finally finish it by implementing the last minor changes and merging all to 1 commit.

However, in the process I somehow managed to mixup histories (still not really sure how that happened) and now GitHub claims that the branch-histories are unrelated and automatically closed the PR.

@ksunden
A final comment on this since I've investigated what went wrong while I've tried to finalize and cleanup this PR...

Apparently, I ran into an GitHub issue (isaacs/github#361) which caused this PR to be closed and also prevents re-opening this PR.

The events that led to this are the following

• I accidentally force-pushed unrelated history resulting in an automatic close of the PR on GitHub
• Once I realized, I undid all changes and force-pushed back to a clean state
• Now the GitHub issue triggers which prevens force-pushed PRs to be re-opened once they have been closed.
• As a consequence, there's now Correctly treat pan/zoom events of overlapping axes. #27148 which is a new PR for the exact same branch (cause somehow GitHub is fine with a new PR but not with re-opening the old one)

Really sorry about this... I actually just wanted to finalize this in a clean way and messed up quite a bit 😅

Not sure how we want to proceed with the duplicated PRs now... aside of that, this PR would be ready for a final review!