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

Use data limits plus a little padding by default #5583

Merged
merged 6 commits into from Dec 8, 2015

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Nov 30, 2015

This is part of the big style change overhaul for 2.0

Rather than using limits that are "round numbers", this will use the data limits plus a little padding.

The complication here is that not all plot types should have the padding. To solve this, it adds left/right/top/bottom margins for each artist which is a boolean. When autoscaling limits, the axes looks at all of its artists and any artist that explicitly sets a margin to False on a given side means the whole axes will not have a margin on that side.

Fixes #4891.

@mdboom mdboom added this to the next major release (2.0) milestone Nov 30, 2015
@@ -898,6 +900,89 @@ def set_zorder(self, level):
self.pchanged()
self.stale = True

def get_top_margin(self):
Copy link
Member

Choose a reason for hiding this comment

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

can we do this via a property instead of getter/setters?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why be inconsistent with everything else? I know traits will eventually get us there, but we're not there yet...

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, does the auto kwarg handling handle properties?

Copy link
Member

Choose a reason for hiding this comment

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

One less thing that we have to support back compatibility with.

Copy link
Member

Choose a reason for hiding this comment

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

There is code in there to white-list properties to be updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

One less thing that we have to support back compatibility with.

I'm not really sold there -- I think it's more important to have everything working consistently than to have some things in the old way and some in the new better way... Do we already have other things that only exist as properties?

Copy link
Member

Choose a reason for hiding this comment

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

It is in Artist.update.

Copy link
Member Author

Choose a reason for hiding this comment

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

So there's only one property we currently whitelist: axes. I guess I'm willing to put this up to a "vote", but I'm strongly in favor of staying with setters/getters for now until we move over to traitlets en masse. There's just too much source for confusion otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

I have a local branch (that I was working on last night before I ended up down that formatter rabbit hole #5594) that uses a single code path for set, update, and setp.

The stale logic is implemented via a property and should (maybe) also be whitelisted so we are at two (both of which are my fault).

We should probably provide both, use the property version internally and advertise the getter/setter version until 2.1.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 on the single code path for set/update/setp. I like that idea a lot.

I have a lot less objection doing both property and getter/setter. In fact, we can probably implement it fairly easily using the "old" pre-decorator property style. Yes, it will look archaic, but it's the best way to reduce code duplication IMHO.

def get_FOO(self):
   ...

def set_FOO(self, val):
   ...

FOO = property(get_FOO, set_FOO)

@pelson
Copy link
Member

pelson commented Nov 30, 2015

Did you consider properties rather than getters/setters?

@mdboom
Copy link
Member Author

mdboom commented Dec 2, 2015

I've added properties in addition to getters/setters here.

@mdboom
Copy link
Member Author

mdboom commented Dec 8, 2015

I think this one is good to go.

tacaswell added a commit that referenced this pull request Dec 8, 2015
API: Use data limits plus a little padding by default
@tacaswell tacaswell merged commit 3ae7c2a into matplotlib:master Dec 8, 2015
@tacaswell
Copy link
Member

Lets see what the new examples look like

@mdboom
Copy link
Member Author

mdboom commented Dec 11, 2015

Backported to 2.0.x as 9617dfe

tacaswell added a commit that referenced this pull request Dec 11, 2015
API: Use data limits plus a little padding by default
@tacaswell
Copy link
Member

Sorry I failed to backport this!

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

3 participants