FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Include outward ticks in bounding box by mdboom · Pull Request #5683 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Include outward ticks in bounding box - #5683

Merged
tacaswell merged 5 commits into
matplotlib:masterfrom
mdboom:tight-ticks
Dec 19, 2015
ConversationCommits (5)ChecksFiles changed
Merged

Include outward ticks in bounding box#5683
tacaswell merged 5 commits into
matplotlib:masterfrom
mdboom:tight-ticks

Conversation

mdboom commented Dec 15, 2015

Copy link
Copy Markdown
Member

Replaces #5502.

mdboom commented Dec 15, 2015

Copy link
Copy Markdown
Member Author

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

mdboom added this to the next major release (2.0) milestone Dec 15, 2015

QuLogic commented Dec 15, 2015

Copy link
Copy Markdown
Member

I think you meant this replaces #5502?

mdboom commented Dec 15, 2015

Copy link
Copy Markdown
Member Author

I think you meant this replaces #5502?

Oops. You're right.

QuLogic commented Dec 15, 2015

Copy link
Copy Markdown
Member

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 commented Dec 16, 2015

Copy link
Copy Markdown
Member Author

A test has been added

WeatherGod reviewed Dec 16, 2015
View reviewed changes
Comment thread lib/matplotlib/axes/_base.py

WeatherGod Dec 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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).

mdboom Dec 16, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Dec 16, 2015

Copy link
Copy Markdown
Member

image tests that have just been added are failing

WeatherGod commented Dec 16, 2015

Copy link
Copy Markdown
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 force-pushed the tight-ticks branch 2 times, most recently from 83e8a00 to 14190e4 Compare December 16, 2015 15:35

mdboom commented Dec 17, 2015

Copy link
Copy Markdown
Member Author

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

tacaswell reviewed Dec 17, 2015
View reviewed changes
Comment thread lib/matplotlib/axis.py Outdated

tacaswell Dec 17, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mdboom Dec 17, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sure.

mdboom commented Dec 17, 2015

Copy link
Copy Markdown
Member Author

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

mdboom commented Dec 17, 2015

Copy link
Copy Markdown
Member Author

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
Merge pull request #5683 from mdboom/tight-ticks
08fc864 
Include outward ticks in bounding box
tacaswell merged commit 08fc864 into matplotlib:master Dec 19, 2015

tacaswell commented Dec 19, 2015

Copy link
Copy Markdown
Member

backported as 2a6bb26

tacaswell added a commit that referenced this pull request Dec 19, 2015
Merge pull request #5683 from mdboom/tight-ticks
2a6bb26 
Include outward ticks in bounding box
tacaswell added a commit that referenced this pull request Dec 19, 2015
Merge pull request #5683 from mdboom/tight-ticks
8ac5b4b 
Include outward ticks in bounding box
mdboom mentioned this pull request Jan 18, 2016
Closed
mdboom mentioned this pull request Jan 25, 2016
Closed

QuLogic commented Oct 16, 2016

Copy link
Copy Markdown
Member

Backport to v2.x was actually via 8ac5b4b.

jklymak mentioned this pull request May 9, 2018
Merged
2 tasks done
anntzer mentioned this pull request Jan 29, 2019
Merged
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL