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

Fixed xlim issue when clearing shared axes #28101

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JamesCamilleri13
Copy link

PR summary

Making sure that when two subplots are sharing an axes, clearing the leader one doesn't also reset the xlim and ylim to [0,1]

import matplotlib.pyplot as plt
import numpy as np

ax0 = plt.subplot(211)
ax1 = plt.subplot(212, sharex=ax0)
ax0.plot(range(100))
ax1.plot(range(100))
ax0.cla()
plt.show()

Befor PR:

Example

After PR:
Example2

PR checklist

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@tacaswell tacaswell added this to the v3.10.0 milestone Apr 19, 2024
@tacaswell
Copy link
Member

To make sure I understand what is going on here

fig = plt.figure()
subfigs = fig.subfigures(1, 2)
(ax1, ax2) = subfigs[0].subplots(2, sharex=True)
ax2.plot(range(500))
ax1.cla()
(ax1, ax2) = subfigs[1].subplots(2, sharex=True)
ax1.plot(range(500))
ax2.cla()

so

and by setting the bi-directional sharing makes them behave the same (presumably there is a "do I have a share friend? if so don't reset my limits" check someplace else?)

This definitely needs a test. I think something like my example in a compare_images test (do it once to the test image with the cla() and once out the cla()) for both sharex and sharey.

@timhoffm
Copy link
Member

timhoffm commented Apr 19, 2024

TL;DR: I believe we need to define the intended behavior of clear() much better to ensure a fix to the admitted undesired behavior of #27825 doesn't lead to other undesired behavior.

On a general note: Do we have a precise idea what clear() means? xref #23839. This and the associated PR codify the current behavior, but I'm not sure whether it's the desired behavior. In particular, what do we expect for shared Axes? Is sharing a symmetrical relation or one way?

Note for example that with the new version

fig, (ax1, ax2) = plt.subplots(2, sharex=True)
ax1.plot(range(500))
ax2.plot(range(500))
ax1.cla()
ax2.cla()

maintains the 0, 500 xlim on both axes due to the cross-wise sharing blocking a reset of the limits. Whereas the same code without sharex=True will reset both Axes limits. I'm unclear whether that's ok or at least better than a one-way sharing, which breaks the symmetry of the calls, but let's you reset the limits if you know what you're doing.

Logically, the situation is ill-defined: We can't have both: clear (reset) limits of a single Axes AND still share with another Axes that should keep its limits. One could as well argue that clear() should break the sharing. But whether that'd be appropriate depends on the intended use case. It may also be a good solution, to reset the internal state such that

fig, (ax1, ax2) = plt.subplots(2, sharex=True)
ax1.plot(range(500))
[do anything on ax2]
ax1.cla()

would result in the same internal state as

fig, (ax1, ax2) = plt.subplots(2, sharex=True)
[do anything on ax2]

but we currently don't do this, and it might be hard to implement.

@anntzer
Copy link
Contributor

anntzer commented Apr 20, 2024

Regarding the semantics of clear(): I think the semantics (in the case here) should be as follows:

  • Autoscaling is not performed by updating dataLim immediately when adding an artist to the axes, but rather, autoscale() collects the bounds of all relevant artists and merges them (*, **).
  • Clearing an axes does not reset the axes limits if they have been set by autoscaling, but simply removes the artists and requests a new autoscale() call (that will just take into account the remaining artists). (Whether clear() should also undo a set_xlim() call is a separate question.) In particular, in the specific case here, the limits should not be reset, because they are imposed by the not-cleared-out artists on the other axes.

(*) This "lazy dataLim" computation is also needed to make #15595 (add an in_autoscale property to artists) work in the case of the property being updated. (See also #7413 (comment).)
(**) To be more accurate, when an artist is added to an axes, the artist's current datalim gets cached (likely on the axes), and these cached per-artist datalim get merged during the autoscale() call. This is necessary so that later set_data() calls don't update the datalims (which is the current semantics and definitely something we cannot break); calling relim() would reset these caches.

@timhoffm
Copy link
Member

@anntzer The described behavior sounds reasonable. Just to be sure I understand correctly: Changing to "lazy dataLim" is necessary and may be a substantial effort.

This is necessary so that later set_data() calls don't update the datalims (which is the current semantics and definitely something we cannot break);

I'm not so sure. While current behavior, this is not really desireable/intuitive. Not continuing to adapt the limits to show all data unless explicit limits are set is even dangerous: One may accitentally overlook new data. - I fixed limits are desired, users should explicitly opt-in. I would be willing to consider a breaking change here after a deprecation period (whether that deprecation can be supported by a runtime warning with reasonable effort would need to be inverstigated but is not necessarily a blocker). - Long-term, not having to cache inital limits would also make the code simpler.

@anntzer
Copy link
Contributor

anntzer commented Apr 20, 2024

  • Yes, changing to lazy-datalims would be a significant effort (but I think it would be useful).
  • FWIW I regularly rely on the ability to change artist data without changing the autoscale bounds they imply. For example you can imagine an oscilloscope-like application where data is regularly updated but you don't want the ylims to be jumping around all the time, as making the plot jump around all the time can be quite jarring. Obviously, you do want to provide the user with a way to reset the ylims (effectively calling relim()); you just don't want that to happen at every set_data call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

[Bug]: Axes.cla() with sharex may wrongly reset the axes limit.
4 participants