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

xlim_changed not emitted on shared axis #15785

Closed
abg3259 opened this issue Nov 28, 2019 · 14 comments · Fixed by #26011
Closed

xlim_changed not emitted on shared axis #15785

abg3259 opened this issue Nov 28, 2019 · 14 comments · Fixed by #26011
Labels
Good first issue Open a pull request against these issues if there are no active ones! status: has patch patch suggested, PR still needed topic: axes
Milestone

Comments

@abg3259
Copy link

abg3259 commented Nov 28, 2019

Bug report

Bug summary

When an axis is shared with another its registered "xlim_changed" callbacks does not get called when the change is induced by a shared axis (via sharex=).

In _base.py the set_xlim for sibling axis are called with emit=False:

matplotlib/lib/matplotlib/axes/_base.py:

/.../
def set_xlim(...)
/.../
        if emit:
            self.callbacks.process('xlim_changed', self)
            # Call all of the other x-axes that are shared with this one
            for other in self._shared_x_axes.get_siblings(self):
                if other is not self:
                    other.set_xlim(self.viewLim.intervalx,
                                   emit=False, auto=auto)

I'm very new to matplotlib, so perhaps there is a good reason for this? emit=False seems to disable both continued "inheritance" of axis (why?) and triggering of change callbacks (looking at the code above).

It seems like one would at least want to trigger the xlim_changed callbacks as they would be intended to react to any change in axis limits.

Edit: Setting emit=True seems to introduce a recursion issue (not sure why but as inheritance seems to be passed along anyway it doesn't really matter). Moving the callback call to outside of the "if emit:"-statement seems to solve the issue as far as I can see when trying it out. Any reason to keep it inside the if-statement?

@bmcfee
Copy link
Contributor

bmcfee commented May 10, 2021

I'm also seeing this behavior on matplotlib 3.4.1. Working from the resampling data example, I've been developing an adaptive waveform plotter in this PR (code included there). The specific quirks that I'm seeing are as follows:

  • Create two axes with shared x axis (eg, fig, (ax0, ax1) = plt.subplots(nrows=2, sharex=True)), and set an axis callback on ax0 for xlim_changed. If the xlim changes on ax1, which does not directly have the callback set, the axes still update appropriately but the callback is never triggered.
  • Possibly related: if the callback is set on ax0 first, and some time later we draw on ax1, the callback never triggers even if we directly set the xlims on ax0.

Note: if I create the shared axes, draw on ax1 first and set the callback on ax0 last, everything works as expected. So I don't think there's any fundamental incompatibility here. It does seem like some data structure is being either ignored or clobbered though.

@jklymak
Copy link
Member

jklymak commented May 10, 2021

A short self-contained example would be very helpful here! Thanks

@bmcfee
Copy link
Contributor

bmcfee commented May 10, 2021

"short" is relative here :) There is a full setup in the linked PR, but here's something hopefully a little more streamlined:

import numpy as np
import matplotlib.pyplot as plt

# From https://matplotlib.org/stable/gallery/event_handling/resample.html
# A class that will downsample the data and recompute when zoomed.
class DataDisplayDownsampler:
    def __init__(self, xdata, ydata):
        self.origYData = ydata
        self.origXData = xdata
        self.max_points = 50
        self.delta = xdata[-1] - xdata[0]

    def downsample(self, xstart, xend):
        # get the points in the view range
        mask = (self.origXData > xstart) & (self.origXData < xend)
        # dilate the mask by one to catch the points just outside
        # of the view range to not truncate the line
        mask = np.convolve([1, 1, 1], mask, mode='same').astype(bool)
        # sort out how many points to drop
        ratio = max(np.sum(mask) // self.max_points, 1)

        # mask data
        xdata = self.origXData[mask]
        ydata = self.origYData[mask]

        # downsample data
        xdata = xdata[::ratio]
        ydata = ydata[::ratio]

        print("using {} of {} visible points".format(len(ydata), np.sum(mask)))

        return xdata, ydata

    def update(self, ax):
        # Update the line
        lims = ax.viewLim
        if abs(lims.width - self.delta) > 1e-8:
            self.delta = lims.width
            xstart, xend = lims.intervalx
            self.line.set_data(*self.downsample(xstart, xend))
            ax.figure.canvas.draw_idle()


# Create a signal
xdata = np.linspace(16, 365, (365-16)*4)
ydata = np.sin(2*np.pi*xdata/153) + np.cos(2*np.pi*xdata/127)


# --- This does not work: ax1 drawn after ax0 kills callbacks
d = DataDisplayDownsampler(xdata, ydata)
fig, (ax0, ax1) = plt.subplots(nrows=2, sharex=True)

# Hook up the line
d.line, = ax0.plot(xdata, ydata, 'o-')
ax0.set_autoscale_on(False)  # Otherwise, infinite loop

# Connect for changing the view limits
ax0.callbacks.connect('xlim_changed', d.update)
ax0.set_xlim(16, 365)

ax1.plot(xdata, -ydata)
plt.show()


# --- This does work: ax0 drawn after ax1
# --- Note: only works if axis limits are controlled via ax0, not ax1
# Create a signal
xdata = np.linspace(16, 365, (365-16)*4)
ydata = np.sin(2*np.pi*xdata/153) + np.cos(2*np.pi*xdata/127)

d = DataDisplayDownsampler(xdata, ydata)

fig, (ax0, ax1) = plt.subplots(nrows=2, sharex=True)

ax1.plot(xdata, -ydata)

# Hook up the line
d.line, = ax0.plot(xdata, ydata, 'o-')
ax0.set_autoscale_on(False)  # Otherwise, infinite loop

# Connect for changing the view limits
ax0.callbacks.connect('xlim_changed', d.update)
ax0.set_xlim(16, 365)


plt.show()

In neither case does panning/zooming/setting limits on ax1 do the right thing.

@jklymak
Copy link
Member

jklymak commented May 10, 2021

Thats not bad ;-)

@jklymak
Copy link
Member

jklymak commented May 10, 2021

The problem is that we do

other.set_xlim(self.viewLim.intervalx, emit=False, auto=auto)

which doesn't do the ax0.callbacks.process('xlim_changed', self)

If we don't do this, it continues to emit to the shared axes and we get an infinite recursion.

Something like

diff --git a/lib/matplotlib/axes/_base.py b/lib/matplotlib/axes/_base.py
index 9898c7c75..0c1941efb 100644
--- a/lib/matplotlib/axes/_base.py
+++ b/lib/matplotlib/axes/_base.py
@@ -3742,10 +3742,11 @@ class _AxesBase(martist.Artist):
             # Call all of the other x-axes that are shared with this one
             for other in self._shared_x_axes.get_siblings(self):
                 if other is not self:
-                    other.set_xlim(self.viewLim.intervalx,
-                                   emit=False, auto=auto)
-                    if other.figure != self.figure:
-                        other.figure.canvas.draw_idle()
+                    if not np.allclose(other.viewLim.intervalx, self.viewLim.intervalx):
+                        other.set_xlim(self.viewLim.intervalx,
+                                    emit=True, auto=auto)
+                        if other.figure != self.figure:
+                            other.figure.canvas.draw_idle()

Fixes the problem (plus we'd need the same for yaxis). However, I'm not really expert enough on how sharing is supposed to work versus the callbacks to know if this is right or the best. @anntzer or @efiring last touched this part of the code I think.

@anntzer
Copy link
Contributor

anntzer commented May 10, 2021

I think I would prefer something like

diff --git i/lib/matplotlib/axes/_base.py w/lib/matplotlib/axes/_base.py
index 9898c7c75..1116d120f 100644
--- i/lib/matplotlib/axes/_base.py
+++ w/lib/matplotlib/axes/_base.py
@@ -541,6 +541,11 @@ class _process_plot_var_args:
             return [l[0] for l in result]
 
 
+import dataclasses
+_NoRecursionMarker = dataclasses.make_dataclass(
+    "_NoRecursionMarker", ["event_src"])
+
+
 @cbook._define_aliases({"facecolor": ["fc"]})
 class _AxesBase(martist.Artist):
     name = "rectilinear"
@@ -3737,13 +3742,18 @@ class _AxesBase(martist.Artist):
         if auto is not None:
             self._autoscaleXon = bool(auto)
 
-        if emit:
+        if emit and emit != _NoRecursionMarker(self):
             self.callbacks.process('xlim_changed', self)
             # Call all of the other x-axes that are shared with this one
             for other in self._shared_x_axes.get_siblings(self):
                 if other is not self:
+                    # Undocumented internal feature: emit can be set to
+                    # _NoRecursionMarker(self) which is treated as True, but
+                    # avoids infinite recursion.
+                    if not isinstance(emit, _NoRecursionMarker):
+                        emit = _NoRecursionMarker(self)
                     other.set_xlim(self.viewLim.intervalx,
-                                   emit=False, auto=auto)
+                                   emit=emit, auto=auto)
                     if other.figure != self.figure:
                         other.figure.canvas.draw_idle()
         self.stale = True

to more explicitly block infinite recursion, but other than that the basic idea seems fine to me.

@bmcfee
Copy link
Contributor

bmcfee commented May 11, 2021

I'm not sure if this is related, but I'm seeing a similar issue if I try to run the same example code multiple times on one ax. As far as I can tell from reading https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/cbook/__init__.py , it should support multiple callbacks on the same signal (or am I misunderstanding?), but the above example when run twice only issues the second callback.

If you think this is unrelated, I can open a separate issue for it.

@jklymak jklymak added this to the v3.5.0 milestone May 11, 2021
@anntzer
Copy link
Contributor

anntzer commented May 11, 2021

I'm not exactly sure what you mean, but note that CallbackRegistry currently drops duplicate callbacks (connecting a same callback a second time to the same signal results in it being dropped and the original cid is returned). I actually think that's a pretty unhelpful behavior and would be happy to see it deprecated (that can just go through a normal deprecation cycle), but that would be a separate issue.

@bmcfee
Copy link
Contributor

bmcfee commented May 11, 2021

Ah, I see. Thanks @anntzer for the clarification.

@tacaswell tacaswell added Good first issue Open a pull request against these issues if there are no active ones! status: has patch patch suggested, PR still needed labels May 14, 2021
@tacaswell
Copy link
Member

I am 👍 on @anntzer 's solution here.

Marking this as a good first issue because we have a patch for it. Will still need to write a test, a simplified version of the initial bug report would probably work (we do not need convolve in the tests / real signals etc).


also good to see fellow NYers around!

@janosh
Copy link

janosh commented Aug 25, 2021

Having the same problem with perhaps a somewhat simpler example. If the registered callbacks were triggered by changes in axes limits from plots with shared x/y-axes, the gray dashed line in the left plot would extend across the whole canvas:

tmp

from typing import Any

import matplotlib.pyplot as plt
from matplotlib.axes import Axes


def add_identity(ax: Axes = None, **line_kwargs: Any) -> None:
    """Add a parity line (y = x) to the provided axis."""
    if ax is None:
        ax = plt.gca()

    # zorder=0 ensures other plotted data displays on top of line
    default_kwargs = dict(alpha=0.5, zorder=0, linestyle="dashed", color="black")
    (identity,) = ax.plot([], [], **default_kwargs, **line_kwargs)

    def callback(axes: Axes) -> None:
        x_min, x_max = axes.get_xlim()
        y_min, y_max = axes.get_ylim()
        low = max(x_min, y_min)
        high = min(x_max, y_max)
        identity.set_data([low, high], [low, high])

    callback(ax)
    # Register callbacks to update identity line when moving plots in interactive
    # mode to ensure line always extend to plot edges.
    ax.callbacks.connect("xlim_changed", callback)
    ax.callbacks.connect("ylim_changed", callback)


fig, (ax1, ax2) = plt.subplots(1, 2, sharex=True, sharey=True)

ax1.plot([0, 1], [1, 0])
add_identity(ax1)

ax2.plot([0, 2], [2, 0])
add_identity(ax2)

plt.savefig('tmp.png')

@QuLogic
Copy link
Member

QuLogic commented Aug 25, 2021

While not the point of this issue, that identity line can be achieved with axline.

@janosh
Copy link

janosh commented Aug 25, 2021

@QuLogic Damn, that's what I get for not reading the docs closely enough: unnecessary work reinventing a (worse) wheel. Thanks for the pointer!

@QuLogic
Copy link
Member

QuLogic commented Aug 25, 2021

No worries, it's new-ish.

@QuLogic QuLogic modified the milestones: v3.5.0, v3.6.0 Sep 25, 2021
@QuLogic QuLogic modified the milestones: v3.6.0, v3.7.0 Aug 24, 2022
@ksunden ksunden modified the milestones: v3.7.0, v3.7.1 Feb 14, 2023
@QuLogic QuLogic modified the milestones: v3.7.1, v3.7.2 Mar 4, 2023
@QuLogic QuLogic removed this from the v3.7.2 milestone Jul 5, 2023
@QuLogic QuLogic added this to the v3.7.3 milestone Jul 5, 2023
@QuLogic QuLogic modified the milestones: v3.7.3, v3.8.0 Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue Open a pull request against these issues if there are no active ones! status: has patch patch suggested, PR still needed topic: axes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants