Skip to content

Revert "DEP: Raise TypeError for subtract(bool_, bool_)." #9255

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

Merged

Conversation

charris
Copy link
Member

@charris charris commented Jun 15, 2017

This reverts commit c9adc35.

Don't raise TypeError for boolean subtraction in Numpy 1.13, see discussion at #9251.

Closes #9251.

@charris charris added this to the 1.13.1 release milestone Jun 15, 2017
@juliantaylor
Copy link
Contributor

juliantaylor commented Jun 15, 2017

I would also revert the error for negative and positive for consistency.
I also disagree on the deprecation warning, subtract on booleans is pretty clearly defined. But those have probably been there for a while so not something to revert for 1.13.1

a = np.ones((), dtype=np.bool_)[()]
assert_raises(TypeError, operator.sub, a, a)

def test_result(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there's no reason to lose this second test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@charris charris force-pushed the revert-boolean-subtraction-error branch from 7fec80d to 45598d7 Compare June 15, 2017 15:07
@charris
Copy link
Member Author

charris commented Jun 15, 2017

I would also revert the error for negative and positive for consistency.

If someone reports a problem I will consider it. Note that a + (-b) != a - b.

@eric-wieser
Copy link
Member

Note that a + (-b) != a - b.

Can you elaborate on when this is true? Or are you just remarking that only one of these gives a TypeError

@charris
Copy link
Member Author

charris commented Jun 15, 2017

Also it conflicts with Python.

In [1]: False - True
Out[1]: -1

In [2]: -True
Out[2]: -1

@charris
Copy link
Member Author

charris commented Jun 15, 2017

@eric-wieser I was just remarking on the state before the deprecations where the unary - was the complement and the binary - was xor. Consistency in the ring/field Z/2 would be - does nothing, as every element is its own negative, and a - b == a+b == a^b In the case where binary + is "or" and * is "and", the customary meaning of a - b would be more like a*(~b). All of which is to argue that using ^ and ~ is more explicit than the various interpretations of -.

@eric-wieser
Copy link
Member

Also it conflicts with Python.

Perhaps then the deprecation should become a futurewarning, after which the loop becomes bool,bool->i8, which is less likely to be surprising

@seberg
Copy link
Member

seberg commented Jun 15, 2017

IIRC, we basically said that we should first make it an error a while, and then pick up that discussion @eric-wieser.

@njsmith
Copy link
Member

njsmith commented Jun 15, 2017

The python behavior is also pretty weird, and reflects the their history of using 0 and 1 as the bools for a long time, and then adding False and True as effectively aliases for those. (Notice issubclass(bool, int).) Numpy bools are different, and in particular are not a subtype of int in numpy's type hierarchy. One reason this is important is that bool arrays and int arrays have very different behavior in fancy indexing. (Compare python range(3)[True] vs numpy arange(3)[True].)

@charris
Copy link
Member Author

charris commented Jun 16, 2017

OK, putting this in. Might to this for 1.14 later as the deprecation cycle is just getting underway for real ;)

@charris charris merged commit 6e2c42d into numpy:maintenance/1.13.x Jun 16, 2017
@charris charris deleted the revert-boolean-subtraction-error branch June 16, 2017 13:00
@seberg
Copy link
Member

seberg commented Jun 16, 2017

We could even go for the VisibleDeprecationWarning for 1.14 if we expect more problems (especially, if we expect not too uncommon usage in small scripts, etc.)

@charris
Copy link
Member Author

charris commented Jun 16, 2017

@seberg Yeah, that might be a good idea, raising a exception sure helped visibility...

@jakirkham
Copy link
Contributor

Also it conflicts with Python.

Perhaps then the deprecation should become a futurewarning, after which the loop becomes bool,bool->i8, which is less likely to be surprising

This seems like a nice idea. In fact it is more informative that diff's current behavior as it indicates how something changed not just that it changed. Plus it is pretty easy to design things that will be compatible with the current and future behavior.

@charris
Copy link
Member Author

charris commented Jun 24, 2017

@jakirkham Might want to open issue, it would be more visible than this merged PR.

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

Successfully merging this pull request may close these issues.

6 participants