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

Questionable margin-cancellation logic #6565

Closed
anntzer opened this issue Jun 10, 2016 · 7 comments
Closed

Questionable margin-cancellation logic #6565

anntzer opened this issue Jun 10, 2016 · 7 comments
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Milestone

Comments

@anntzer
Copy link
Contributor

anntzer commented Jun 10, 2016

#5583 lets artists "cancel" the use of margins on an Axes, with the typical use case being images. However, blindly cancelling all margins seem wrong: as of 2.0b1,

plt.imshow(np.random.rand(5, 5))
plt.plot([0, 6], [0, 6])

results in no margins at the (6, 6) end of the line plot, even though it is well beyond the limits of the image.
Intuitively, it seems instead that the margins setting of an artist should simply determine whether its default view limits use margins.

@tacaswell tacaswell added this to the 2.0 (style change major release) milestone Jun 10, 2016
@anntzer
Copy link
Contributor Author

anntzer commented Jun 12, 2016

After some more thought, I realized that what the proper behavior should be is not obvious. For example, if I display an image and add some annotations on top of it, then it makes sense to not add margins (by default) even if the annotations are very close to the edges. But if the image is just a small part of a larger plot then as explained above it is a bit weird that they have a long "at-distance" effect.

Ignoring for now implementation issues, my current best guess at what the proper behavior of margin cancellation should be would be "find the view limits as if margins was zero; then, add margins to each edge that is not in contact with an axes-cancelling artist".

Practically speaking, I'm a bit worried that the abstraction and API of #5583 may not be the best one for this (for example it may make sense to also provide the possibility for an artist to indicate that it does not want to be taken into account for autoscaling (#5538)), so perhaps it may make sense, for now, to just directly and manually set the margins to zero when adding an image (or pcolormesh, etc.) to an axes, to avoid later back-compatibility issues?

@anntzer
Copy link
Contributor Author

anntzer commented Aug 10, 2016

Thinking about it again, I now believe that there should just not be any margins cancellation logic in the artist themselves. Basically this is a similar issue as #3946 (should imshow cancel grids?), where it was suggested that if the rcParams have axes.grid set to True then it should not be cancelled upon a call to imshow.
Here it's the same idea; note that since #5583, imshow always cancels margins, even if one explicitly calls margins(.2) after!
Now I wouldn't mind (let's say -0) a "do the right thing by default" approach that's robust against the fact that we now have nonzero default margins, but I think that should just mean that imshow() should call margins(0), instead of going through these convolutions.

ping @mdboom as he authored #5583.

@tacaswell
Copy link
Member

The top/bottom/left/right thing is to enable bar to do the right thing (margins on top, but not on the bottom).

I would suggest that when the user explicitly sets magins that it flips some state on the axis to short-circuit the disabling logic (or just disable it on all of the artists currently in the axes).

@efiring
Copy link
Member

efiring commented Sep 12, 2016

I think user-set margins should disable the disabling logic for all artists, not just the ones added to that point. Otherwise we have another order-of-operations problem.

@anntzer
Copy link
Contributor Author

anntzer commented Nov 5, 2016

Well, the top/bottom/left/right thing actually doesn't work that well for bars either (see #7175).

I still think that rather than rolling out a problematic API in 2.0, we should just call margins(0) in imshow and extend margins to be able to call bottom=0/top=0/etc. for handling bars (#7175). Note that this is effectively what the new margins system does -- the only difference, from the end user point of view, is that in order to restore the default margins, he has to call (approximately)

ax.margins(0.2) # or some value read out from the rcparams

with the old approach, but

for artist in ax.lines + ax.images + ...: artist.set_margins(False)

with the new one... which clearly seems more complex for little gain.

I'm going to mark this as release critical so that this API is not released with at least one core dev saying "nope, I think this is fine".

@anntzer anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Nov 5, 2016
@anntzer
Copy link
Contributor Author

anntzer commented Nov 17, 2016

xref'ing that separate left/right margins was proposed also in #1912.

@efiring
Copy link
Member

efiring commented Dec 4, 2016

closed by #7476

@efiring efiring closed this as completed Dec 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

No branches or pull requests

3 participants