Skip to content

BUG: raise ValueError if sharex, sharey point to a different figure #6463

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

Closed
wants to merge 2 commits into from

Conversation

efiring
Copy link
Member

@efiring efiring commented May 22, 2016

Sharex and sharey are only supported for sharing within a single
figure.
Closes #2790.

@efiring
Copy link
Member Author

efiring commented May 22, 2016

This is by no means critical, but it addresses an issue that was still tagged with the v1.5.0 milestone.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone May 22, 2016
@tacaswell
Copy link
Member

Apparently we have an example showing how to do this!

/home/travis/build/matplotlib/matplotlib/doc/examples/pylab_examples/shared_axis_across_figures.rst:8: WARNING: Exception occurred in plotting shared_axis_across_figures
 from /home/travis/build/matplotlib/matplotlib/doc/mpl_examples/pylab_examples/shared_axis_across_figures.py:
Traceback (most recent call last):
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/sphinxext/plot_directive.py", line 520, in run_code
    six.exec_(code, ns)
  File "<string>", line 13, in <module>
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/figure.py", line 1006, in add_subplot
    a = subplot_class_factory(projection_class)(self, *args, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_subplots.py", line 73, in __init__
    self._axes_class.__init__(self, fig, self.figbox, **kwargs)
  File "/home/travis/build/matplotlib/matplotlib/lib/matplotlib/axes/_base.py", line 510, in __init__
    raise ValueError('Shared Axes must be in the same figure')
ValueError: Shared Axes must be in the same figure

@efiring
Copy link
Member Author

efiring commented May 22, 2016

@tacaswell, allowing shared axes between figures can work only with aspect set to 'auto'. It is fundamentally incompatible with aspect-ratio handling. This is not a matter of implementation, it is a matter of an inconsistent over-specification: like 3 equations in 2 unknowns. I don't think there is any easy way to get around it. One option might be to lock the figure aspect ratio--but I doubt people would accept that, and I think it would be messy to implement. Another would be to rework the whole position specification and coordinate machinery so as to get around the squishiness of our 0-1 figure and axes coordinate systems, which are great for scaling and resizing and reshaping, but lousy for aspect-preserving layout. Not likely to happen any time soon, right?

Maybe the workaround here is to add more logic to check at draw time for consistency of settings. In the case of shared axes it would have to cycle through the list each time--when each Axes is drawn.

@tacaswell
Copy link
Member

There was discussion of this being a useful feature recently (either on the mailing list or another issue).

@efiring
Copy link
Member Author

efiring commented May 23, 2016

Sorry, what is the "useful feature" you are referring to? Being able to share axes across figures?

@tacaswell
Copy link
Member

Yes, keeping two axis synched across figures can be useful. A not too
contrived example in a custom embedding you might need to break the canvas
into several parts to fit in the layout.

I think that there is a simpler way to keep the limits in synch that does
not involve sharing objects between them though.

On Sun, May 22, 2016, 23:52 Eric Firing notifications@github.com wrote:

Sorry, what is the "useful feature" you are referring to? Being able to
share axes across figures?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6463 (comment)

Sharex and sharey are only supported for sharing within a single
figure.
Closes matplotlib#2790.
@efiring
Copy link
Member Author

efiring commented Jun 4, 2017

@tacaswell, can we proceed with this? Or something like it? There is no longer an example of sharing across figures, and my basic point is that the sharex system was never intended to support that, and is fundamentally unsuited to it.

@anntzer
Copy link
Contributor

anntzer commented Jun 5, 2017

Could the sharex system be rewritten by simply connecting to the xlim_changed (and a new xscale_changed) event? Something like ax1.callbacks.connect("xlim_changed", lambda: ax2.set_xlim(ax1.get_xlim()))
.
Unless I am missing something, this would probably allow getting rid of a lot of code (... basically most of the shared axes code?), and also easily allow sharing and unsharing axes after creation time or across axes.

In order to avoid infinite loops, one could

  1. use emit=False (but other callbacks could legitimately be listening to ax2's xlim_changed)
  2. only have the callback apply the new xlims if the current xlims are different, but we can probably still get an infinite loop in the case of zooming problem on figures with shared axes #2790.
  3. set a local variable in to mark the axes as "already having listened to this cascade of xlim_changed events" so that further xlim_changed in the same "cascade" of events do not affect it (my preferred solution having thought 2 sec. about it).

@efiring
Copy link
Member Author

efiring commented Jun 5, 2017

That doesn't help with the most fundamental problem, which is conflict between aspect-ratio specification and axis-sharing.

Apart from that, there is more to the present axis-sharing system than synchronization of data limits, so what you are talking about is a different kind and degree of sharing--really, it is not sharing, it is just synchronization of view limits. And I think that doing it as you describe would lead to problems, and end up getting more complicated than you think, if you take into account the ability to synchronize multiple axes and being able to add and delete axes from the synchronized set.

It might be good to have an API for such synchronization, but I think it will have to exist alongside the present axes sharing.

@anntzer
Copy link
Contributor

anntzer commented Jun 5, 2017

That doesn't help with the most fundamental problem, which is conflict between aspect-ratio specification and axis-sharing.

As @tacaswell mentioned, there are legitimate uses of sharing across figures; and it is not clear why having multiple figures is a problem. For example, one can easily set up overconstrained axes within a single figure (say, share their x and y, and give them different aspect ratios) and matplotlib just gives up on one of the constraints (experimentally, it seems to drop the sharing on x); so I'd expect the same to happen if the axes are on two figures.

Apart from that, there is more to the present axis-sharing system than synchronization of data limits, so what you are talking about is a different kind and degree of sharing--really, it is not sharing, it is just synchronization of view limits.

That is my main question: what else is there, other than synchronization of axes limits and scales?

And I think that doing it as you describe would lead to problems, and end up getting more complicated than you think, if you take into account the ability to synchronize multiple axes and being able to add and delete axes from the synchronized set.

Not really? We can just define sharing two axes as adding a vertex between them, and any limits/scale changes just walks through whatever the connected component is (by progressively triggering the xlim_changed callbacks). Unsharing removes a vertex, and the connected components are just what they are (in particular, the two axes may still be shared if both are shared with a third axes). As unsharing was not possible before, we get to define it however we want :-)

@efiring
Copy link
Member Author

efiring commented Jun 5, 2017

Sharing shares formatters and locators. In addition, in subplots(), it suppresses tick labels on shared axes between subplots.

Yes, I know people may want to synchronize view limits between figures, but doing that via sharex/sharey is not consistent with the way our default locator works now. It chooses ticks based on physical dimensions so as not to make the tick labels too crowded. This only makes sense with sharing if the Axes are in the same figure.

The following sounds like the setup you describe, and it does indeed have problems:

fig = plt.figure()
ax1 = fig.add_subplot(2,1,1)
ax2 = fig.add_subplot(2, 1, 2, sharex=ax1, sharey=ax1)
ax1.set_aspect(2)
ax2.set_aspect(0.5)
ax1.plot([0, 1])
ax2.plot([0, 1])

Sharing more than one axis is incompatible with specifying the aspect ratio on more than one axes, so probably this should also be blocked--or enabled only with a "forced" kwarg somewhere, like "box-forced", which I have always disliked. Or, perhaps the whole system can be re-engineered.

But this is a separate issue from the present PR, which is simply trying to block the behavior identified in #2790. That behavior is more serious than the over-specification described above, because it causes 100% CPU usage, not just a potentially surprising plot.

@anntzer
Copy link
Contributor

anntzer commented Jun 5, 2017

It looks like 100% CPU usage does not occur when using the tkagg backend (it does occur when using the qt5agg or gtk3agg backend), which once again suggests to me that this is the wrong way to solve the issue (though I don't have any better proposal right now).

Indeed I had missed the sharing of locators and formatters, so it's not going to be that simple; thanks for the explanation.

@tacaswell
Copy link
Member

What happened to the example that uses this?

I am 👍 on merging this with the addition of an API change note (it may not have been intentionally API, but it is API that users have found) with a suggestion of how to get safe view limit sycing.

Related to #8700 and @astrofrog 's request for a way to add sharing after creation, but I suspect that what he really wants here is just view limit syncing

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would strongly suggest that we need to figure out why 100% CPU usage does not appear either

  1. using the tkagg backend, or
  2. when incompatible specs are used within the same figure

and see whether the same fix can be applied for the multiple figure case, non-tkagg case, before we throw away the baby with the bathwater.

@efiring
Copy link
Member Author

efiring commented Jun 6, 2017

To repeat: the fundamental major problems come when there is over-specification of parameters because of the combination of axis sharing with aspect-ratio locking. Tracking down the difference in backends that leads to different manifestations of the problem is fine, but it does nothing to solve the problem, because it is fundamental--it is an inherent conflict.

I agree that the present PR is not ideal; it is simply blocking one route to the conflict, and a side-effect is that it also blocks some non-conflicting usage.

A better PR would be one that clearly identifies all possible conflicts, and handles them via some combination of blocking and/or resolution via relaxing constraints in a clear way, with warnings when constraints are relaxed. And of course it would have to do this without imposing any excessive performance penalty.

I would be happy to withdraw this PR in favor of one that blocks the bad behavior in #2790 via the approach above, even if it does not handle all cases. If it handles additional cases, all the better. And lots of bonus points if it allows us to deprecate the 'box_forced' option.

@efiring
Copy link
Member Author

efiring commented Jun 6, 2017

I think I see a bug in code that is already supposed to block the problem cases, so let's hold off on this while I investigate.

@efiring
Copy link
Member Author

efiring commented Jun 6, 2017

Found the bug; I will make a PR to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants