Skip to content

BUG: various fixes to np.gradient #9408

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.

Sign up for GitHub

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
merged 4 commits into from
Sep 21, 2017
Merged

BUG: various fixes to np.gradient #9408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d86e3fe
BUG: Allow 0d arrays instead of scalars in gradient
eric-wieser Jul 13, 2017
2bfec2c
BUG: Only allow 1d distance arrays
eric-wieser Jul 13, 2017
24508c1
BUG: Use np.ndim not asarray, to allow duck-types
eric-wieser Jul 13, 2017
1f4ba5b
MAINT: Use clearer variable
eric-wieser Jul 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions numpy/lib/function_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1685,23 +1685,28 @@ def gradient(f, *varargs, **kwargs):
len_axes = len(axes)
n = len(varargs)
if n == 0:
# no spacing argument - use 1 in all axes
dx = [1.0] * len_axes
elif n == len_axes or (n == 1 and np.isscalar(varargs[0])):
elif n == 1 and np.ndim(varargs[0]) == 0:
# single scalar for all axes
dx = varargs * len_axes
elif n == len_axes:
# scalar or 1d array for each axis
dx = list(varargs)
for i, distances in enumerate(dx):
if np.isscalar(distances):
if np.ndim(distances) == 0:
continue
elif np.ndim(distances) != 1:
raise ValueError("distances must be either scalars or 1d")
Copy link
Contributor

Choose a reason for hiding this comment

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

right, didn't thought about explicit error message for not 1D arrays

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@apbard apbard Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

@apbard apbard Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@eric-wieser eric-wieser Jul 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Let's discuss that type of extension elsewhere: #9409

if len(distances) != f.shape[axes[i]]:
raise ValueError("distances must be either scalars or match "
raise ValueError("when 1d, distances must match "
"the length of the corresponding dimension")
diffx = np.diff(dx[i])
diffx = np.diff(distances)
# if distances are constant reduce to the scalar case
# since it brings a consistent speedup
if (diffx == diffx[0]).all():
diffx = diffx[0]
dx[i] = diffx
if len(dx) == 1:
dx *= len_axes
else:
raise TypeError("invalid number of arguments")

Expand Down Expand Up @@ -1751,7 +1756,7 @@ def gradient(f, *varargs, **kwargs):
# result allocation
out = np.empty_like(y, dtype=otype)

uniform_spacing = np.isscalar(dx[i])
uniform_spacing = np.ndim(dx[i]) == 0

# Numerical differentiation: 2nd order interior
slice1[axis] = slice(1, -1)
Expand Down
9 changes: 8 additions & 1 deletion numpy/lib/tests/test_function_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,8 +742,11 @@ def test_args(self):

# distances must be scalars or have size equal to gradient[axis]
gradient(np.arange(5), 3.)
gradient(np.arange(5), np.array(3.))
gradient(np.arange(5), dx)
gradient(f_2d, 1.5) # dy is set equal to dx because scalar
# dy is set equal to dx because scalar
gradient(f_2d, 1.5)
gradient(f_2d, np.array(1.5))

gradient(f_2d, dx_uneven, dx_uneven)
# mix between even and uneven spaces and
Expand All @@ -753,6 +756,10 @@ def test_args(self):
# 2D but axis specified
gradient(f_2d, dx, axis=1)

# 2d coordinate arguments are not yet allowed
assert_raises_regex(ValueError, '.*scalars or 1d',
gradient, f_2d, np.stack([dx]*2, axis=-1), 1)

def test_badargs(self):
f_2d = np.arange(25).reshape(5, 5)
x = np.cumsum(np.ones(5))
Expand Down