Skip to content

Use data limits plus a little padding by default #5583

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Dec 8, 2015
Merged

Use data limits plus a little padding by default #5583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
41d7e12
Scale margins to data, plus padding, by default
mdboom Nov 8, 2015
1bad2eb
Add test
mdboom Nov 30, 2015
7fd7b7b
Update test data
mdboom Nov 30, 2015
0e0c005
PEP8
mdboom Nov 30, 2015
b68950c
Add properties in addition to get/set
mdboom Dec 2, 2015
73bac38
Update baseline image
mdboom Dec 6, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 95 additions & 0 deletions lib/matplotlib/artist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ def __init__(self):
self._sketch = rcParams['path.sketch']
self._path_effects = rcParams['path.effects']

self._margins = {}

def __getstate__(self):
d = self.__dict__.copy()
# remove the unpicklable remove method, this will get re-added on load
Expand Down Expand Up @@ -898,6 +900,99 @@ 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)

"""
Get whether a margin should be applied to the top of the Artist.
"""
return self._margins.get('top', True)

def set_top_margin(self, margin):
"""
Set whether a margin should be applied to the top of the Artist.
"""
if margin != self._margins.get('top', True):
self.stale = True
self._margins['top'] = margin

top_margin = property(get_top_margin, set_top_margin)

def get_bottom_margin(self):
"""
Get whether a margin should be applied to the bottom of the Artist.
"""
return self._margins.get('bottom', True)

def set_bottom_margin(self, margin):
"""
Set whether a margin should be applied to the bottom of the Artist.
"""
if margin != self._margins.get('bottom', True):
self.stale = True
self._margins['bottom'] = margin

bottom_margin = property(get_bottom_margin, set_bottom_margin)

def get_left_margin(self):
"""
Get whether a margin should be applied to the left of the Artist.
"""
return self._margins.get('left', True)

def set_left_margin(self, margin):
"""
Set whether a margin should be applied to the left of the Artist.
"""
if margin != self._margins.get('left', True):
self.stale = True
self._margins['left'] = margin

left_margin = property(get_left_margin, set_left_margin)

def get_right_margin(self):
"""
Get whether a margin should be applied to the right of the Artist.
"""
return self._margins.get('right', True)

def set_right_margin(self, margin):
"""
Set whether a margin should be applied to the right of the Artist.
"""
if margin != self._margins.get('right', True):
self.stale = True
self._margins['right'] = margin

right_margin = property(get_right_margin, set_right_margin)

def get_margins(self):
"""
Returns a dictionary describing whether a margin should be applied on
each of the sides (top, bottom, left and right).
"""
return self._margins

def set_margins(self, margins):
"""
Set the dictionary describing whether a margin should be applied on
each of the sides (top, bottom, left and right). Missing keys are
assumed to be `True`. If `True` or `False` are passed in, all
sides are set to that value.
"""
if margins in (True, False):
margins = {
'top': margins,
'bottom': margins,
'left': margins,
'right': margins
}

if margins != self._margins:
self.stale = True

self._margins = margins

margins = property(get_margins, set_margins)

def update_from(self, other):
'Copy properties from *other* to *self*.'
self._transform = other._transform
Expand Down
47 changes: 33 additions & 14 deletions lib/matplotlib/axes/_axes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2104,16 +2104,21 @@ def make_iterable(x):
if yerr is not None:
yerr = self.convert_yunits(yerr)

if align == 'edge':
pass
elif align == 'center':
margins = {}

if orientation == 'vertical':
margins = {'bottom': False}
elif orientation == 'horizontal':
margins = {'left': False}

if align == 'center':
if orientation == 'vertical':
left = [left[i] - width[i] / 2. for i in xrange(len(left))]
elif orientation == 'horizontal':
bottom = [bottom[i] - height[i] / 2.
for i in xrange(len(bottom))]

else:
elif align != 'edge':
raise ValueError('invalid alignment: %s' % align)

args = zip(left, bottom, width, height, color, edgecolor, linewidth)
Expand All @@ -2129,7 +2134,8 @@ def make_iterable(x):
facecolor=c,
edgecolor=e,
linewidth=lw,
label='_nolegend_'
label='_nolegend_',
margins=margins
)
r.update(kwargs)
r.get_path()._interpolation_steps = 100
Expand Down Expand Up @@ -5267,7 +5273,7 @@ def pcolor(self, *args, **kwargs):

kwargs.setdefault('snap', False)

collection = mcoll.PolyCollection(verts, **kwargs)
collection = mcoll.PolyCollection(verts, margins=False, **kwargs)

collection.set_alpha(alpha)
collection.set_array(C)
Expand Down Expand Up @@ -5302,9 +5308,9 @@ def pcolor(self, *args, **kwargs):
maxy = np.amax(y)

corners = (minx, miny), (maxx, maxy)
self.add_collection(collection, autolim=False)
self.update_datalim(corners)
self.autoscale_view()
self.add_collection(collection, autolim=False)
return collection

@unpack_labeled_data(label_namer=None)
Expand Down Expand Up @@ -5419,7 +5425,8 @@ def pcolormesh(self, *args, **kwargs):

collection = mcoll.QuadMesh(
Nx - 1, Ny - 1, coords,
antialiased=antialiased, shading=shading, **kwargs)
antialiased=antialiased, shading=shading, margins=False,
**kwargs)
collection.set_alpha(alpha)
collection.set_array(C)
if norm is not None and not isinstance(norm, mcolors.Normalize):
Expand Down Expand Up @@ -5451,9 +5458,9 @@ def pcolormesh(self, *args, **kwargs):
maxy = np.amax(Y)

corners = (minx, miny), (maxx, maxy)
self.add_collection(collection, autolim=False)
self.update_datalim(corners)
self.autoscale_view()
self.add_collection(collection, autolim=False)
return collection

@unpack_labeled_data(label_namer=None)
Expand Down Expand Up @@ -5603,7 +5610,8 @@ def pcolorfast(self, *args, **kwargs):
# The QuadMesh class can also be changed to
# handle relevant superclass kwargs; the initializer
# should do much more than it does now.
collection = mcoll.QuadMesh(nc, nr, coords, 0, edgecolors="None")
collection = mcoll.QuadMesh(nc, nr, coords, 0, edgecolors="None",
margins=False)
collection.set_alpha(alpha)
collection.set_array(C)
collection.set_cmap(cmap)
Expand Down Expand Up @@ -5649,15 +5657,19 @@ def contour(self, *args, **kwargs):
if not self._hold:
self.cla()
kwargs['filled'] = False
return mcontour.QuadContourSet(self, *args, **kwargs)
contours = mcontour.QuadContourSet(self, *args, **kwargs)
self.autoscale_view()
return contours
contour.__doc__ = mcontour.QuadContourSet.contour_doc

@unpack_labeled_data()
def contourf(self, *args, **kwargs):
if not self._hold:
self.cla()
kwargs['filled'] = True
return mcontour.QuadContourSet(self, *args, **kwargs)
contours = mcontour.QuadContourSet(self, *args, **kwargs)
self.autoscale_view()
return contours
contourf.__doc__ = mcontour.QuadContourSet.contour_doc

def clabel(self, CS, *args, **kwargs):
Expand Down Expand Up @@ -6037,6 +6049,11 @@ def hist(self, x, bins=None, range=None, normed=False, weights=None,
else:
n = [m[slc].cumsum()[slc] for m in n]

if orientation == 'horizontal':
margins = {'left': False}
else:
margins = {'bottom': False}

patches = []

if histtype.startswith('bar'):
Expand Down Expand Up @@ -6177,14 +6194,16 @@ def hist(self, x, bins=None, range=None, normed=False, weights=None,
patches.append(self.fill(
x, y,
closed=True,
facecolor=c))
facecolor=c,
margins=margins))
else:
for x, y, c in reversed(list(zip(xvals, yvals, color))):
split = 2 * len(bins)
patches.append(self.fill(
x[:split], y[:split],
closed=False, edgecolor=c,
fill=False))
fill=False,
margins=margins))

# we return patches, so put it back in the expected order
patches.reverse()
Expand Down
Loading