Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Path.contains_points() incorrect return #1787

Merged
merged 2 commits into from May 3, 2013

Conversation

Projects
None yet
4 participants
Owner

mdboom commented May 3, 2013

Path.contains_points() returns an array with all elements False if one element should be False.

Example:

from matplotlib.path import Path
import matplotlib.patches as patches

vertices = [(2., 2.), (2., 5.), (6., 5.), (6., 2.), (2., 2.)]
codes = [Path.MOVETO, Path.LINETO,
         Path.LINETO, Path.LINETO, Path.CLOSEPOLY]
path = Path(vertices, codes)

path.contains_points([[3, 3]])
>>> array([ True], dtype=bool)

path.contains_points([[3, 3], [0, 0]])
>>> array([False, False], dtype=bool)

The deprecated nxutils.points_inside_poly produces the correct result:

from matplotlib.nxutils import points_inside_poly
points_inside_poly([[3, 3], [0, 0]], vertices)
>>> array([ True, False], dtype=bool)

The contains_points docstring is also the same as contains_point

This is using the current master

Member

pelson commented Feb 26, 2013

Confirmed. From memory of looking at the code just a couple of days ago, I believe this was by design (as this case can terminate the inside checking early if a point is not found to be inside), but I agree, the functionality you describe is also desirable. Thanks for raising @rdgeorge.

Member

dmcdougall commented Mar 23, 2013

The question now is, how do we move forward? My feeling is that this shouldn't exit prematurely on a False. Having this check an array of points is useful.

Owner

mdboom commented Mar 25, 2013

Yeah -- I think we should try to fix as @dmcdougall suggests. There might be people relying on the "broken" behavior, but I can't see how it's terribly desired or useful the way it is.

hayne commented Apr 30, 2013

Moreover, since the nxutils function 'points_inside_poly' has been deprecated and the recommendation is to change code to use the Path.contains_points method, it is important that the contains_points method have the same semantics as the nxutils function 'points_inside_poly' (which returned an array with individual True/False results).
I.e. the current "broken" behaviour will break lots of old code that was using the nxutils function.

BTW, the matplotlib FAQ is still referring to the nxutils function. It should be updated to refer to the Path method.

Member

pelson commented May 1, 2013

@mdboom - I'm very tempted to put this under the v1.3 blockers. Thoughts?

hayne commented May 1, 2013

Looking at the nxutils source code, I see that (in matplotlib version version 1.2.1) the function 'points_inside_poly' is now implemented by creating a Path and calling the 'contains_points' method.

And (as pointed out by rdgeorge above), nxutils.points_inside_poly works correctly (to give an array with individual True/False results).

The reason why nxutils.points_inside_poly works while the direct call to Path.contains_points in rdgeorge's example above fails seems to be due to the different ways that the Path object was constructed. The wrapper code in nxutils constructs a Path by just passing the array of vertices, without specifying any 'codes'.
So it would seem that the problem is with Path's that were constructed with 'codes'.

Example code:

verts1 = [(0,0), (0,1), (1,1), (1,0)]
verts2 = [(0,0), (0,1), (1,1), (1,0), (0,0)]

path1 = Path(verts1)
path2 = Path(verts2, closed=True)

path1
Path([[ 0. 0.]
[ 0. 1.]
[ 1. 1.]
[ 1. 0.]], None)

path2
Path([[ 0. 0.]
[ 0. 1.]
[ 1. 1.]
[ 1. 0.]
[ 0. 0.]], [ 1 2 2 2 79])

points = [(0.5,0.5), (1.5,0.5)]

path1.contains_points(points)
array([ True, False], dtype=bool)

path2.contains_points(points)
array([False, False], dtype=bool)

The fact that two different ways of constructing Path objects (that as far as I understand should represent the same conceptual "path") leads to different behaviour may indicate a more fundamental problem with Path.

Owner

mdboom commented May 1, 2013

I've remilestoned this as a 1.3.x blocker. I haven't had a chance to look into this in any depth, but as the original author of Path.contains_points, I hope to find a fix.

Owner

mdboom commented May 3, 2013

The problem here is that in the latter case, you actually get a subpath followed by a zero-length subpath. There was a bug such that it only returns the inclusion in the last subpath. Since a point can't possibly be in a zero-length subpath, it was always returning zero. The solution is to calculate the result for each subpath separately and "or" them together for the final result.

@rdgeorge: Can you please confirm this resolves the issue for you?

hayne commented May 3, 2013

mdboom wrote:

The problem here is that in the latter case, you actually get a subpath followed by a zero-length subpath

I'm not sure if you are referring to 'path2' in my example code above, but if so, it would seem (to me) that there is a documentation problem. The doc on how to construct a Path object and the meaning and implications of the 'code' arguments and the 'close' flag need to be much better explained. Part of that explanation should be about when a Path is considered closed and what the implications are. I would have thought naively that it doesn't make any sense to call 'contains_points' on a non-closed Path (and hence such should cause an exception).

Member

pelson commented May 3, 2013

@mdboom - in principle this looks good. It could do with a test though.

Owner

mdboom commented May 3, 2013

@hayne: I didn't mean to imply there was anything wrong with the path. They both are correct. Only that when iterating over the latter, the points_in_path algorithm is fed first a subpath (equal to the whole path in this case) followed by a zero-length subpath -- which is completely valid, it's just that points_in_path was not handling it correctly.

mdboom added a commit that referenced this pull request May 3, 2013

Merge pull request #1787 from mdboom/points_in_path_bug
Path.contains_points() incorrect return

@mdboom mdboom merged commit 1cf4f5e into matplotlib:v1.2.x May 3, 2013

1 check passed

default The Travis build passed
Details

@mdboom mdboom deleted the mdboom:points_in_path_bug branch Aug 7, 2014

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