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

Include outward ticks in bounding box #5683

Merged
merged 5 commits into from Dec 19, 2015
Merged

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Dec 15, 2015

Replaces #5502.

@mdboom
Copy link
Member Author

mdboom commented Dec 15, 2015

This needs a test specifically of tight layout on outward ticks, but I thought I'd put this up for comment early anyway...

@mdboom mdboom added this to the next major release (2.0) milestone Dec 15, 2015
@QuLogic
Copy link
Member

QuLogic commented Dec 15, 2015

I think you meant this replaces #5502?

@mdboom
Copy link
Member Author

mdboom commented Dec 15, 2015

I think you meant this replaces #5502?

Oops. You're right.

@QuLogic
Copy link
Member

QuLogic commented Dec 15, 2015

Well, I don't see anything particularly wrong with it, but are there no test images that use outward ticks? Or just none with a tight bounding box?

@mdboom
Copy link
Member Author

mdboom commented Dec 16, 2015

A test has been added

x_pad = self.xaxis.get_tick_padding()
y_pad = self.yaxis.get_tick_padding()
return mtransforms.Bbox([[bbox.x0 - x_pad, bbox.y0 - y_pad],
[bbox.x1 + x_pad, bbox.y1 + y_pad]])
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to work for polar axes? I would also double-check that this doesn't utterly bork for axes3d (not that I'd expect it to work great there, just to not crash or something).

Copy link
Member Author

Choose a reason for hiding this comment

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

polar axes don't have ticks -- but it does appear that this needs to be updated so it's aware of when the ticks are present or not.

It doesn't seem to crash for axes3d, though as you say, it doesn't really do the right thin other than add a bit more space.

@WeatherGod
Copy link
Member

image tests that have just been added are failing

@WeatherGod
Copy link
Member

I don't think the test is really all that useful. tight_layout is for ensuring that subplots don't run into each other. So, a good test would be to have a grid of subplots, perhaps each with different tick directions?

@mdboom mdboom force-pushed the tight-ticks branch 2 times, most recently from 83e8a00 to 14190e4 Compare December 16, 2015 15:35
@mdboom
Copy link
Member Author

mdboom commented Dec 17, 2015

@WeatherGod: I've updated the test as you helpfully suggested.

Get the length of the tick outside of the axes.
"""
tickdir = self._tickdir
if tickdir == 'in':
Copy link
Member

Choose a reason for hiding this comment

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

bit of a bike shed, better to do this with a dictionary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

@mdboom
Copy link
Member Author

mdboom commented Dec 17, 2015

I've addressed @tacaswell's comments, and added minor ticks to the test to catch the pathological case he mentions.

@mdboom
Copy link
Member Author

mdboom commented Dec 17, 2015

Also updated to handle the case where there are no ticks, as suggested by @WeatherGod

tacaswell added a commit that referenced this pull request Dec 19, 2015
Include outward ticks in bounding box
@tacaswell tacaswell merged commit 08fc864 into matplotlib:master Dec 19, 2015
@tacaswell
Copy link
Member

backported as 2a6bb26

tacaswell added a commit that referenced this pull request Dec 19, 2015
Include outward ticks in bounding box
@QuLogic
Copy link
Member

QuLogic commented Oct 16, 2016

Backport to v2.x was actually via 8ac5b4b.

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.

None yet

4 participants