Skip to content

Regression: Path.contains_points now returns uint instead of bool #6566

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

Jump to bottom
Closed
astrofrog opened this issue Jun 10, 2016 · 6 comments
Closed

Regression: Path.contains_points now returns uint instead of bool #6566

astrofrog opened this issue Jun 10, 2016 · 6 comments
Assignees
@mdboom
Milestone
v1.5.2

Comments

@astrofrog
Copy link
Contributor

In Matplotlib 1.5.1, contains_points return a boolean, consistent with the docstring:

In [1]: from matplotlib.path import Path

In [2]: Path([[1,2],[3,4],[5,6]]).contains_points([[1,2]])
Out[2]: array([False], dtype=bool)

However, in the latest developer version, an uint8 is returned:

In [15]: from matplotlib.path import Path

In [16]: Path([[1,2],[3,4],[5,6]]).contains_points([[1,2]])
Out[16]: array([0], dtype=uint8)

This causes issues with packages that are assuming bool, since masking arrays using an array of ints has a different results (it will either select the first or second element or both from an array), so I believe this is a regression.
@astrofrog astrofrog mentioned this issue Jun 10, 2016
Merged
@tacaswell tacaswell added this to the 1.5.2 (Critical bug fix release) milestone Jun 10, 2016
@tacaswell
Copy link
Member

In < 1.4 it returned uints, 1.5 return bool (as part of removing CXX usage), and we reverted to uint in #6450 as part of fixing a major path related speed regression.

We should probably cast to bool on the way out?

attn @mdboom

@WeatherGod
Copy link
Member

I thought we were casting to bool on the way out? Is that only on master
then?

On Fri, Jun 10, 2016 at 8:58 AM, Thomas A Caswell notifications@github.com
wrote:

In < 1.4 it returned uints, 1.5 return bool (as part of removing CXX
usage), and we reverted to uint in #6450
#6450 as part of fixing a
major path related speed regression.

We should probably cast to bool on the way out?

attn @mdboom https://github.com/mdboom

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#6566 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AARy-EwVVseop5OZ9Gy7cphgrW2v67Ldks5qKV9kgaJpZM4Iy2YH
.

@astrofrog
Copy link
Contributor Author

astrofrog commented Jun 10, 2016 •
edited
Loading

@WeatherGod - the failure is on master, I haven't tested elsewhere

@tacaswell
Copy link
Member

attn @mdboom

@tacaswell
Copy link
Member

I think we should revert the change to return uint -> bool. We broke that api 1.4->1.5 without realizing it, but it is better to stick with it (rather than swing back and forth).

Also as @astrofrog the bool is more natural to use for masking.

tacaswell added a commit to tacaswell/matplotlib that referenced this issue Jun 28, 2016
@tacaswell
FIX: cast return of contains_points to bool
4819a2c 
Closes matplotlib#6566
@tacaswell tacaswell mentioned this issue Jun 28, 2016
Merged
@jenshnielsen
Copy link
Member

Closed by #6654

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

@mdboom mdboom

Labels
None yet
Projects
None yet
Milestone
v1.5.2
Development

No branches or pull requests
5 participants
@mdboom @tacaswell @WeatherGod @astrofrog @jenshnielsen