-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
np.diff
is broken for boolean ndarrays in 1.13.0
#9251
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
Comments
This seems like an easy fix. The https://github.com/numpy/numpy/blob/master/numpy/lib/function_base.py#L1835 You would probably want to branch on |
It seems to me that if |
Well - I can see the purity argument for disallowing, but it seems to me that it is reasonable to expect diff to work on booleans, and not surprising that scikit image was doing that. So I would have thought a branch on |
I'd vote for keeping the functionality. If we all agree, I'll submit a patch later today or tomorrow. P.S. Could you, please, point me at the discussion where the rationale for deprecating |
If |
Seems to me that we might want an |
My vote would be to avoid the user having to stop and debug an error on |
Both of these arguments apply to |
Yes, they do, and if you think of instinctively think of |
"is this value different from the previous one" sounds like an operation worthy of it's own name that generalizes to all dtypes, rather than spelling it Perhaps Although |
Sure, that might be a good idea as well - but I would still argue that it would be kinder (practicality beats purity) to forgive this way of thinking of bools and |
The subtraction operator for booleans has been deprecated for some time, since 1.9 I believe, scikit image hasn't been paying attention. Paying attention is also a good idea ;) I think a different name would be appropriate, xdiff or some such. |
That said, having |
It's a bit unfair to blame scikit image here - I can't see any deprecation warning for numpy 1.12.1 and:
|
That's because by default |
|
One other thing we should do is add a test for Since the solution seems to be controversial (I think I'm with Matthew on this one, although I don't care much either way), I have sent an e-mail to the list so that everyone gets a say. |
I'm also of the practical beats purity type here, especially since this used to work. Somehow, like @matthew-brett, there is a "diff" is "difference/differs" confusion in my mind, and I certainly have used p.s. @jaimefrio - I didn't see any message on the (numpy discussion) mailing list. |
+1 for practicality. And definitely -1 for a new function. If it's useful, then put it in |
Yes, I noticed I couldn't find my mail in the archives to link it here. But there's an email in my sent folder to numpy-discussion@scipy.org at 18:00 CET. Perhaps the server is acting up again? Or were those issues fixed for good already? |
Wrong address, it has moved ;) numpy-discussion@python.org . |
Ah, that would explain it... Here's a link to the mail, resent to the correct address, for future reference: https://mail.python.org/pipermail/numpy-discussion/2017-June/076877.html |
This doesn't look good at all - scipy/scipy#7493 . |
Someone want to make a PR adding support for boolean arrays to diff? |
@charris I do. Are we all agreed on this? |
@soupault I think that is the consensus. |
This also applies to |
Kind ping on @eric-wieser 's question. Should we fix |
Personally, I don't see much point in supporting gradients of bool arrays. |
Should modify the docstring accordingly, raise a meaningful error, or (preferably) make it work by hidding some logic under the hood.
Traceback:
xref scikit-image/scikit-image#2681
The text was updated successfully, but these errors were encountered: