BUG: check for closed path in Polygon.set_xy() #1018

wants to merge 2 commits into


None yet

4 participants

jakevdp commented Jul 17, 2012

This was something unexpected I came across: I had to dig through the code to see why it didn't behave as I thought it should. I'm not sure whether it should be changed (I can see arguments both ways) but I thought I'd bring it up just in case.

Former behavior:

>>> xy = [[0,0], [0,1], [1,1]]
>>> p = Polygon(xy, closed=True)
>>> p.get_xy()
array([[ 0.,  0.],
       [ 0.,  1.],
       [ 1.,  1.],
       [ 0.,  0.]])
>>> p.set_xy(xy)
>>> p.get_xy()
array([[ 0.,  0.],
       [ 0.,  1.],
       [ 1.,  1.]])

This PR changes things so the second output is the same as the first, for both closed = True and closed = False.

pelson commented Jul 17, 2012

I'm happy that this is like-for-like behaviour with the added set_xy fix. Because there is a behavioural change (or bug fix), probably needs an entry in the doc/api/api_changes.rst doc.

Other than that, gets my +1.

efiring commented Jul 17, 2012

I think the idea is valid, but additional refactoring is needed. The logic in set_closed starting with "if closed:" should be moved into set_xy(), so that the closing point is not duplicated unnecessarily. set_closed would then end with "self.set_xy(self.get_xy)"

mdboom commented Aug 3, 2012

A agree with @efiring, but other than that, looks ok. Let's add a unit test, too.

@efiring efiring added a commit to efiring/matplotlib that referenced this pull request Aug 12, 2012
@efiring efiring patches.polygon: fix bug in handling of path closing, #1018.
This is a modification of that pull request, with a test added.
efiring commented Aug 12, 2012

This is superceded by #1071, so I am closing it.

@efiring efiring closed this Aug 12, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment