BUG: various fixes to np.gradient #9408
Conversation
numpy/lib/function_base.py
Outdated
| elif n == len_axes or (n == 1 and np.isscalar(varargs[0])): | ||
| dx = list(varargs) | ||
| dx = [asanyarray(d) for d in varargs] |
There was a problem hiding this comment.
This is fine WRT to array-likes, because np.diff calls asanyarray anyway
0fccd34 to
d1d4cc0
Compare
2d arrays would work, but in unpredictable and undocumented ways. This at least makes numpygh-9401 give a better error message.
d1d4cc0 to
2bfec2c
Compare
numpy/lib/function_base.py
Outdated
| @@ -1674,6 +1674,7 @@ def gradient(f, *varargs, **kwargs): | |||
| S0025-5718-1988-0935077-0/S0025-5718-1988-0935077-0.pdf>`_. | |||
| """ | |||
| f = np.asanyarray(f) | |||
| varargs = [asanyarray(d) for d in varargs] | |||
There was a problem hiding this comment.
This is fine WRT to array-likes, because np.diff calls asanyarray anyway
There was a problem hiding this comment.
Actually, this doesn't work for scalar duck types - now fixed
7176154 to
1f4ba5b
Compare
| continue | ||
| elif np.ndim(distances) != 1: | ||
| raise ValueError("distances must be either scalars or 1d") |
There was a problem hiding this comment.
right, didn't thought about explicit error message for not 1D arrays
There was a problem hiding this comment.
I suspect we can do something much more powerful for nd arrays if we think about it (perhaps the grad operator from vector calculus?), but short-term we just want to disable the currently-broken behaviour
There was a problem hiding this comment.
Well, that behaviour was already disabled and the docstring correctly says what is the expected input.
When I firstly implemented this I thought about allowing a list of coordinates instead of individual parameters, but I discarded that because I think it is quite error prone and error checking is more complex. Anyway, if I am not wrong, you cannot pass a "real"-2d matrix unless in the special case where your data have the same dimension along each axis.
So, IMHO, disabling this is not a short term patch but rather the most convenient thing to do.
There was a problem hiding this comment.
unless in the special case where your data have the same dimension along each axis.
Correct, but this is exactly the case that all our tests use!
There was a problem hiding this comment.
Allowing instead to compute the gradient with an arbitrary 2d list of coordinates would require a quite different approach and implementation. Anyway, even in this case you would require at maximum a 2d array with shape (num_points, num_dimensions)
There was a problem hiding this comment.
unless in the special case where your data have the same dimension along each axis.
Correct, but this is exactly the case that all our tests use!
I agree that we can add a more general test to check ND-"rectangular" data as well as "squared" ones, but this is quite unrelated to the dimension/type of input coordinates , is it?
What I meant that even in the case we decide to implement this it should not be a 2d-array but a list of 1d-arrays because each would require to have a potentially different size.
There was a problem hiding this comment.
Let's discuss that type of extension elsewhere: #9409
|
Added |
|
@charris: this is another one of those PRs that I rebased, for easy backport via the github ui. Since this fixes a new 1.13 feature, and a regression in 1.12, I think it is a candidate for 1.13.2 |
|
This was broken by the commit of #9411. |
|
Guess that #9411 should be backported if this one is. |
|
Ugh, this was not supposed to conflict with #9411 |
269b7ea to
1f4ba5b
Compare
|
Only the last (MAINT) commit conflicted, so I've just rolled that back - I can make a separate PR for that after this is backported. |
|
Thanks Eric. Want to do the backport? |
|
Guess I need to dig out that MAINT commit from my local history and submit that now too |
Follows on from @apbard's #8446.
This fixes #8292, and provides a clearer error for #9401, which looks invalid
This allowed some things to happen that shouldn't, and forbid some things from happening that should.