Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

patches.polygon: fix bug in handling of path closing, #1018. #1071

Merged
merged 3 commits into from

3 participants

@efiring
Owner

This is a modification of that pull request, with a test added, both based on the comments on #1018

@efiring efiring patches.polygon: fix bug in handling of path closing, #1018.
This is a modification of that pull request, with a test added.
c6cc861
lib/matplotlib/patches.py
@@ -783,20 +779,22 @@ def get_closed(self):
def set_closed(self, closed):
if self._closed == bool(closed):
return
- self._closed = closed
- xy = self._get_xy()
- if closed:
+ self._closed = bool(closed)
+ self.set_xy(self.get_xy())
+
+ def get_xy(self):
+ return self._path.vertices
+
+ def set_xy(self, xy):
+ xy = np.asarray(xy, np.float_)
@WeatherGod Collaborator

Is it necessary to explicitly cast this dtype to be floats? I'm not against it, I just don't see it done elsewhere.

@efiring Owner
efiring added a note

The cast was already in the code, but in fact it is not necessary because xy is passed to the Path initializer, which performs the cast. I can take it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson
Collaborator

One thing I would like to see more of is to have a docstring in the top of a test module explaining its purpose so that future developers know where to put their tests. In this case, what other tests do you think might go in the test_patches module in the future?

Additionally, I believe the test_patches module name needs to be put in matplotlib/__init__.py (see http://matplotlib.sourceforge.net/devel/coding_guide.html#creating-a-new-module-in-matplotlib-tests).

@efiring
Owner

@pelson, I think that there are two primary categories of test modules: those that hold miscellaneous tests from a corresponding mpl module (e.g. test_axes), and those that hold tests of tricky behavior (e.g., test_simplification), sophisticated enough to warrant a separate module. So the purpose of test_patches is to hold the present test, and to provide a home for any additional such tests that are added as bugs are identified and fixed. This is all just my impression of how the use of nosetests has evolved; I don't think the strategy was ever fully planned and documented in detail.

@efiring efiring merged commit 8a5afe3 into matplotlib:master
@efiring efiring deleted the efiring:close_polygon branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 12, 2012
  1. @efiring

    patches.polygon: fix bug in handling of path closing, #1018.

    efiring authored
    This is a modification of that pull request, with a test added.
  2. @efiring
  3. @efiring
This page is out of date. Refresh to see the latest.
View
5 CHANGELOG
@@ -1,3 +1,8 @@
+2012-08-11 Fix path-closing bug in patches.Polygon, so that regardless
+ of whether the path is the initial one or was subsequently
+ set by set_xy(), get_xy() will return a closed path if and
+ only if get_closed() is True. Thanks to Jacob Vanderplas. - EF
+
2012-08-05 When a norm is passed to contourf, either or both of the
vmin, vmax attributes of that norm are now respected.
Formerly they were respected only if both were
View
1  lib/matplotlib/__init__.py
@@ -1020,6 +1020,7 @@ def tk_window_focus():
'matplotlib.tests.test_delaunay',
'matplotlib.tests.test_legend',
'matplotlib.tests.test_colorbar',
+ 'matplotlib.tests.test_patches',
]
def test(verbosity=1):
View
26 lib/matplotlib/patches.py
@@ -767,12 +767,8 @@ def __init__(self, xy, closed=True, **kwargs):
"""
Patch.__init__(self, **kwargs)
- xy = np.asarray(xy, np.float_)
- self._path = Path(xy)
self._closed = closed
- if closed and len(xy):
- xy = np.concatenate([xy, [xy[0]]])
- self._set_xy(xy)
+ self.set_xy(xy)
def get_path(self):
return self._path
@@ -783,20 +779,22 @@ def get_closed(self):
def set_closed(self, closed):
if self._closed == bool(closed):
return
- self._closed = closed
- xy = self._get_xy()
- if closed:
+ self._closed = bool(closed)
+ self.set_xy(self.get_xy())
+
+ def get_xy(self):
+ return self._path.vertices
+
+ def set_xy(self, xy):
+ xy = np.asarray(xy)
+ if self._closed:
if len(xy) and (xy[0] != xy[-1]).any():
xy = np.concatenate([xy, [xy[0]]])
else:
if len(xy)>2 and (xy[0]==xy[-1]).all():
- xy = xy[0:-1]
- self._set_xy(xy)
+ xy = xy[:-1]
+ self._path = Path(xy, closed=self._closed)
- def get_xy(self):
- return self._path.vertices
- def set_xy(self, vertices):
- self._path = Path(vertices, closed=self._closed)
_get_xy = get_xy
_set_xy = set_xy
xy = property(
View
42 lib/matplotlib/tests/test_patches.py
@@ -0,0 +1,42 @@
+"""
+Tests specific to the patches module.
+"""
+
+from numpy.testing import assert_array_equal
+from matplotlib.patches import Polygon
+
+def test_Polygon_close():
+ """
+ Github issue #1018 identified a bug in the Polygon handling
+ of the closed attribute; the path was not getting closed
+ when set_xy was used to set the vertices.
+ """
+ # open set of vertices:
+ xy = [[0,0], [0,1], [1,1]]
+ # closed set:
+ xyclosed = xy + [[0,0]]
+
+ # start with open path and close it:
+ p = Polygon(xy, closed=True)
+ assert_array_equal(p.get_xy(), xyclosed)
+ p.set_xy(xy)
+ assert_array_equal(p.get_xy(), xyclosed)
+
+ # start with closed path and open it:
+ p = Polygon(xyclosed, closed=False)
+ assert_array_equal(p.get_xy(), xy)
+ p.set_xy(xyclosed)
+ assert_array_equal(p.get_xy(), xy)
+
+ # start with open path and leave it open:
+ p = Polygon(xy, closed=False)
+ assert_array_equal(p.get_xy(), xy)
+ p.set_xy(xy)
+ assert_array_equal(p.get_xy(), xy)
+
+ # start with closed path and leave it closed:
+ p = Polygon(xyclosed, closed=True)
+ assert_array_equal(p.get_xy(), xyclosed)
+ p.set_xy(xyclosed)
+ assert_array_equal(p.get_xy(), xyclosed)
+
Something went wrong with that request. Please try again.