MAINT/BUG: Remove special-casing for 0d arrays, now that indexing with a single boolean is ok #9900
Conversation
…h a single boolean is ok Also fix the test added in numpygh-4792, which didn't make sense, but passed anyway
| if (isscalar(condlist) or not (isinstance(condlist[0], list) or | ||
| isinstance(condlist[0], ndarray))): | ||
| if not isscalar(condlist) and x.size == 1 and x.ndim == 0: | ||
| condlist = [[c] for c in condlist] |
There was a problem hiding this comment.
This line inserts an extra dim into cond to match what we to did to 0d x below - so is no longer needed.
Note that this would misfire on non-0d x sometimes - which is why a test below needed changing
Essentially, this line is replaced with pass, and then the nested ifs are collapsed
| @@ -2538,7 +2538,7 @@ def test_0d(self): | |||
| assert_(y == 0) | |||
|
|
|||
| x = 5 | |||
| y = piecewise(x, [[True], [False]], [1, 0]) | |||
There was a problem hiding this comment.
This didn't make any sense, and should have failed. The conditions should be the same shape as the x. We got away with it because the 0d special-casing made the shapes match.
| zerod = False | ||
| if x.ndim == 0: | ||
| x = x[None] | ||
| zerod = True |
There was a problem hiding this comment.
I think this worked around arr_0d[bool_0d] not being supported in the past
numpy/lib/function_base.py
Outdated
| condlist = [condlist] | ||
|
|
||
| # undocumented: single condition is promoted to a list of one condition | ||
| if np.ndim(condlist) == 0 or ( |
There was a problem hiding this comment.
This took me very long to parse; could you add a comment for what the second part of the if statement is for? It might have helped me a little to have seen a reversed order x.ndim >0 and not isinstance(...)
So, part of the problem is that this statement is to hit an arbitrary selection of cases where the user maybe forgot to pass a list. It performs the following mappings
piecewise(x_1d, cond_1d, ...)->piecewise(x_1d, [cond_1d])piecewise(x_sc, cond_0d, ...)->piecewise(x_0d, [cond_0d])piecewise(x_0d, cond_0d, ...)->piecewise(x_0d, [cond_0d])(after the second commit)
But doesn't do anything to the following, which are read as:
piecewise(x_2d, cond_2d, ...)->piecewise(x_2d, list(cond_2d), ...)piecewise(x_3d, cond_3d, ...)->piecewise(x_3d, list(cond_3d), ...)
Which it interprets as broadcasting an n-1d condition against an nd array.
This is super inconsistent - perhaps I should leave the isscalar, and just deprecate the entire code path? Especially since it was never documented.
a51c5e2 to
303941c
Compare
|
I've removed the |
| if isscalar(condlist) or ( | ||
| not isinstance(condlist[0], (list, ndarray)) and x.ndim != 0): | ||
| condlist = [condlist] | ||
|
|
There was a problem hiding this comment.
So, was the idea to deprecate this? An alternative might be to just add ndmin=1 in the conversion to boolean array below.
There was a problem hiding this comment.
I'd like to deprecate this in future, but this patch is orthogonal to that
There was a problem hiding this comment.
ndim=1 won't help - this is intended to also promote 1d arrays to a list of length 1 containing a single 1d array.
|
OK, makes sense to postpone |
| assert_raises_regex(ValueError, '2 or 3 functions are expected', | ||
| piecewise, x, x, [x <= 3, x > 3], [1]) | ||
| assert_raises_regex(ValueError, '2 or 3 functions are expected', | ||
| piecewise, x, x, [x <= 3, x > 3], [1, 1, 1, 1]) |
There was a problem hiding this comment.
Typos here - should be x just once. Will fix this evening
Especially necessary given the strange heuristics that decay the number of conditions to 1
9f54488 to
c875b13
Compare
|
Thanks Eric. |
Also fix the test added in gh-4792, which didn't make sense, but passed anyway