Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

BUG: interpolate.interp2d would ignore three of its arguments #361

Closed
wants to merge 1 commit into from

2 participants

@larsmans
Collaborator

This should solve #1072 and #1444. I've yet to update the tests, but please tell me if this the right way to go, as I have a few questions remaining.

  • I changed the docstring's meaning of copy=False to not promise a reference to be stored. This matches the old behavior. The reference/copy is useless anyway, as it's never used. Maybe we should get rid of the x, y, z members and document copy as being a no-op, only accepted for compatibility with interp1d?
  • The proposed code is backwards-incompatible. Should we add an extrapolate argument (or accept bounds_error="extrapolate") to get the old behavior back?
  • Finally, should the bounds be public or private (leading _) attributes? Does SciPy have any conventions that I should follow here?
@pv pv commented on the diff
scipy/interpolate/interpolate.py
((12 lines not shown))
z = fitpack.bisplev(x, y, self.tck, dx, dy)
z = atleast_2d(z)
z = transpose(z)
+ z[out_of_bounds] = self.fill_value
@pv Owner
pv added a note

I don't think this works as intended? The fill value is assigned only according to the first axis?

@larsmans Collaborator

You're right. It works in the 1-d case, but I didn't test the 2-d case. Will fix.

@pv Owner
pv added a note

This assignment to z[out_of_bounds] does not work as intended --- z has shape (len(x), len(y)), whereas out_of_bounds has shape (nx,).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pv
Owner
pv commented

Yep, I think some new tests are needed here.

Having fill_value=None signal extrapolation (and having that as the default) could be another alternative --- I think I like it more than having string arguments in bounds_error. Backward compatibility probably should be preserved here.

For class attributes not part of API, I'd personally prefer having an underscore prefix, but I'm not sure existing code should be changed to follow this. Of course, if it's "if undocumented, then private" is another rule.

(For Scipy module and function/class names my preference for _ prefix is stronger, because this helps people to see more clearly what may change in the future, and what probaly not. Case in point: there's loads of code out there importing stuff from Scipy's sub-submodules...)

@larsmans
Collaborator

Personally, I'd find overloading bounds_error easier to read and more explicit ("if a bounds error occurs, then just extrapolate") than overloading fill value (which would signal "if a bounds error occurred, then fill with None"), also because None becomes np.nan when assigned to a Numpy array element.

What to do with the x, y, z attributes? They were entirely undocumented, so can they be removed?

@pv
Owner
pv commented

I suppose the x, y, z attributes are not widely used by existing code, so they probably could be removed. On the other hand, at least x and y may come useful in some cases, so there's also a rationale for preserving (and documenting) them.

Re: bounds_error / fill_value --- as long as backwards compatibility is preserved, any sensible approach works here.

@larsmans
Collaborator

Hm, I've been trying to write a test, but I can't import the Scipy I just built; scipy.special complains about a missing symbol zbesh_. Any idea what that could be? I'm running a pretty vanilla Ubuntu 12.04.

@pv
Owner
pv commented

rm -rf build, rebuild probably helps.

@pv
Owner
pv commented

gh-353 touched this part of the code, so you may want to rebase

@larsmans larsmans BUG: interp2d would ignore three of its arguments
Also changed the meaning of copy=False to not *promise* a reference
to be stored. The reference/copy is useless anyway.
70fa2e2
@pv
Owner
pv commented

Revised version merged in c3cc0ee

@pv pv closed this
@ClemensFMN ClemensFMN referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 27, 2012
  1. @larsmans

    BUG: interp2d would ignore three of its arguments

    larsmans authored
    Also changed the meaning of copy=False to not *promise* a reference
    to be stored. The reference/copy is useless anyway.
This page is out of date. Refresh to see the latest.
Showing with 39 additions and 24 deletions.
  1. +39 −24 scipy/interpolate/interpolate.py
View
63 scipy/interpolate/interpolate.py
@@ -1,4 +1,3 @@
-
""" Classes for interpolating values.
"""
@@ -93,10 +92,11 @@ class interp2d(object):
kind : {'linear', 'cubic', 'quintic'}, optional
The kind of spline interpolation to use. Default is 'linear'.
copy : bool, optional
- If True, then data is copied, otherwise only a reference is held.
+ If True, the class makes internal copies of x, y and z.
+ If False, references may be used. The default is to copy.
bounds_error : bool, optional
If True, when interpolated values are requested outside of the
- domain of the input data, an error is raised.
+ domain of the input data (x,y), an error is raised.
If False, then `fill_value` is used.
fill_value : number, optional
If provided, the value to use for points outside of the
@@ -142,27 +142,27 @@ class interp2d(object):
def __init__(self, x, y, z, kind='linear', copy=True, bounds_error=False,
fill_value=np.nan):
- self.x, self.y, self.z = map(asarray, [x, y, z])
- self.x, self.y = map(ravel, [self.x, self.y])
-
- if self.z.size == len(self.x) * len(self.y):
- rectangular_grid = True
- if not all(self.x[1:] > self.x[:-1]):
- j = np.argsort(self.x)
- self.x = self.x[j]
- self.z = self.z[:,j]
- if not all(self.y[1:] > self.y[:-1]):
- j = np.argsort(self.y)
- self.y = self.y[j]
- self.z = self.z[j,:]
- self.z = ravel(self.z.T)
+ x = ravel(x)
+ y = ravel(y)
+ z = asarray(z)
+
+ rectangular_grid = z.size == len(x) * len(y)
+ if rectangular_grid:
+ if not all(x[1:] > x[:-1]):
+ j = np.argsort(x)
+ x = x[j]
+ z = z[:,j]
+ if not all(y[1:] > y[:-1]):
+ j = np.argsort(y)
+ y = y[j]
+ z = z[j,:]
+ z = ravel(z.T)
else:
- rectangular_grid = False
- self.z = ravel(self.z)
- if len(self.x) != len(self.y):
+ z = ravel(z)
+ if len(x) != len(y):
raise ValueError(
"x and y must have equal lengths for non rectangular grid")
- if len(self.z) != len(self.x):
+ if len(z) != len(x):
raise ValueError(
"Invalid length for input z for non rectangular grid")
@@ -175,15 +175,21 @@ def __init__(self, x, y, z, kind='linear', copy=True, bounds_error=False,
if not rectangular_grid:
# TODO: surfit is really not meant for interpolation!
- self.tck = fitpack.bisplrep(self.x, self.y, self.z,
- kx=kx, ky=ky, s=0.0)
+ self.tck = fitpack.bisplrep(x, y, z, kx=kx, ky=ky, s=0.0)
else:
nx, tx, ny, ty, c, fp, ier = dfitpack.regrid_smth(
- self.x, self.y, self.z, None, None, None, None,
+ x, y, self.z, None, None, None, None,
kx=kx, ky=ky, s=0.0)
self.tck = (tx[:nx], ty[:ny], c[:(nx - kx - 1) * (ny - ky - 1)],
kx, ky)
+ self.bounds_error = bounds_error
+ self.fill_value = fill_value
+ self.x, self.y, self.z = [array(a, copy=copy) for a in x, y, z]
+
+ self.x_min, self.x_max = np.amin(x), np.amax(x)
+ self.y_min, self.y_max = np.amin(y), np.amax(y)
+
def __call__(self,x,y,dx=0,dy=0):
"""Interpolate the function.
@@ -207,9 +213,18 @@ def __call__(self,x,y,dx=0,dy=0):
x = atleast_1d(x)
y = atleast_1d(y)
+
+ out_of_bounds = np.logical_or.reduce([x < self.x_min, x > self.x_max,
+ y < self.y_min, y > self.y_max])
+ if self.bounds_error and np.any(out_of_bounds):
+ raise ValueError("Values out of range; x must be in %r, y in %r"
+ % ((self.x_min, self.x_max),
+ (self.y_min, self.y_max)))
+
z = fitpack.bisplev(x, y, self.tck, dx, dy)
z = atleast_2d(z)
z = transpose(z)
+ z[out_of_bounds] = self.fill_value
@pv Owner
pv added a note

I don't think this works as intended? The fill value is assigned only according to the first axis?

@larsmans Collaborator

You're right. It works in the 1-d case, but I didn't test the 2-d case. Will fix.

@pv Owner
pv added a note

This assignment to z[out_of_bounds] does not work as intended --- z has shape (len(x), len(y)), whereas out_of_bounds has shape (nx,).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
if len(z)==1:
z = z[0]
return array(z)
Something went wrong with that request. Please try again.