Skip to content
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

make_interp_spline bc_type incorrect input interpretation #8911

Closed
wualbert opened this issue Jun 7, 2018 · 3 comments · Fixed by #8919
Closed

make_interp_spline bc_type incorrect input interpretation #8911

wualbert opened this issue Jun 7, 2018 · 3 comments · Fixed by #8919

Comments

@wualbert
Copy link

wualbert commented Jun 7, 2018

<<Please describe the issue in detail here, and for bug reports fill in the fields below.>>

bc_type crashes when it takes two 2-tuples for constraining the ends of the trajectory. For example, for zero 1st-derivatives (clamped), the input should be bc_type=((1, 0.0), (1, 0.0)).
One possible fix is to change

 deriv_l_ords, deriv_l_vals = zip(*deriv_l)

in line 682 of _bsplines.py to

deriv_l_ords, deriv_l_vals = deriv_l

and

deriv_r_ords, deriv_r_vals = zip(*deriv_r)

in line 690 of _bsplines.py to

deriv_r_ords, deriv_r_vals = deriv_r

This way the input is interpreted without error.

Reproducing code example:

from scipy.interpolate import make_interp_spline
make_interp_spline([1,2,3,4,5],[6,7,8,9,10],bc_type = ((1,0.0),(1,0.0)))

Error message:

Traceback (most recent call last):
  File "/anaconda/lib/python3.6/site-packages/IPython/core/interactiveshell.py", line 2881, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-3-f57c7724483d>", line 1, in <module>
    make_interp_spline([1,2,3,4,5],[6,7,8,9,10],bc_type = ((1,0.0),(1,0.0)))
  File "/anaconda/lib/python3.6/site-packages/scipy/interpolate/_bsplines.py", line 682, in make_interp_spline
    deriv_l_ords, deriv_l_vals = zip(*deriv_l)
TypeError: zip argument #1 must support iteration

Scipy/Numpy/Python version information:

0.19.1 1.13.1 sys.version_info(major=3, minor=6, micro=0, releaselevel='final', serial=0)
@ev-br
Copy link
Member

ev-br commented Jun 7, 2018

Not quite.

bc_type parameter indeed needs to be a two-element. Each element of this tuple is an iterable of pairs, where each pair is ``(order, value)`. However, in your example, each element is a pair itself, hence the error.

This way, to get the clamped cubic spline, you do

In [8]: x = y = [1.0, 2, 3, 4, 5, 6]

In [9]: l, r = [(1, 0.0)], [(1, 0.0)]      # Note here: `l` is a one-element list of pairs.

In [10]: make_interp_spline(x, y, bc_type=(l, r))
Out[10]: <scipy.interpolate._bsplines.BSpline at 0x7fb448ba2198>

Somewhat embarassingly, the explicit equivalences in the docs were wrong, thus adding to the confusion. gh-8914 fixes it.


On a side note, one may argue that the whole thing is confusing. Which it is, because of this tuple of iterables of pairs thingy. The complexity is needed to support a general case (e.g., a 5th order spine needs four boundary conditions). However for a common case of a cubic spline with exactly two boundary conditions at each side of the interval, this can be viewed as extraneous, and the OP example looks more natural (and is consistent with CubicSpline).

In principle, we may special case a common need for a cubic with bc_type=(first pair, second pair), but I'm two minds about adding a workaround on top of already arcane construction. So I'd ask for a second opinion. Thoughts?

@ev-br ev-br added scipy.interpolate needs-decision Items that need further discussion before they are merged or closed labels Jun 7, 2018
@rgommers
Copy link
Member

I'd say a special case doesn't make things better here, it'll be even more messy to explain things well. I'd be -0.5.

The exception is TypeError: zip argument #1 must support iteration - that could be more informative. Maybe add a check that each element of bc_type is iterable and contains the expected number of size-2 elements?

@ev-br
Copy link
Member

ev-br commented Jun 10, 2018

Thanks Ralf. gh-8919 attempts to improve error messages.

@rgommers rgommers removed the needs-decision Items that need further discussion before they are merged or closed label Jun 10, 2018
@rgommers rgommers added this to the 1.2.0 milestone Jun 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants