Skip to content
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.

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

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

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
Milestone

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.

@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

@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.

@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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants