FIX: Fix interpolate #5624

larsoner · 2015-12-24T01:04:38Z

@ev-br this fixes a cryptic error I was getting:

  File "/home/larsoner/.local/lib/python2.7/site-packages/scipy/interpolate/interpolate.py", line 410, in __init__
    self._fill_value_below, self._fill_value_above = _duplicate(fill_value)
  File "/home/larsoner/.local/lib/python2.7/site-packages/scipy/interpolate/interpolate.py", line 311, in _duplicate
    a, b = ab
ValueError: too many values to unpack

Turns out that doing e.g. a, b = np.array([1.]) raises ValueError, not TypeError like a, b = 1. would.

larsoner · 2015-12-24T01:07:30Z

Although thinking about it a bit, this probably isn't going to be a safe check case for some fill values that start with dimension 2, right...? I guess it would be ambiguous at that point if the two elements were start and end fills, or if they were the 2 values that should be used for both the start and the end. So maybe an error should be raised in that specific case?

larsoner · 2015-12-24T01:17:43Z

scipy/interpolate/interpolate.py

@@ -309,7 +309,7 @@ def _duplicate(ab):
    # transform to a pair (a, a)
    try:
        a, b = ab
-    except TypeError:
+    except (TypeError, ValueError):  # ndarray can rais ValueError here


I can fix this typo and open the fix to master if people agree it's the right move

ev-br · 2015-12-24T09:00:14Z

Somehow I did not test array-valued fill_values, thanks for catching it!

Sadly, just catching ValueError does not do the right thing e.g., here:

>>> interp1d(x, y, fill_value=(1, 2, 3))
Traceback (most recent call last):
...
ValueError: too many values to unpack

Maybe the right thing is to do np.atleast_1d(fill_value) and explicitly check that the length of the result is 1 or 2. (or np.asarray, but then np.asarray(1).size == 1 while len(np.asarray(1)) raises).

One more issue with arrays as fill_values is if fill_value == "extrapolate" which raises a FutureWarning: interpolate.py:398: FutureWarning: elementwise comparison failed; returning scalar instead, but in the future will perform elementwise comparison.

You can fix this now or I'll do it separately a bit later.

larsoner · 2015-12-24T14:17:24Z

One more issue with arrays as fill_values is if fill_value ==
"extrapolate" which raises a FutureWarning: interpolate.py:398:
FutureWarning: elementwise comparison failed; returning scalar instead, but
in the future will perform elementwise comparison.

This could be fixed by checking isinstance(fill_value, string_types) and fill_value=='extrapolate'), which is easy enough.

larsoner · 2015-12-24T17:32:20Z

Hmmm... this is a bit more complex than I originally thought. I have some fixes in the works. But I realized that our use case actually made use of the fact that the fill_value could be broadcast. In other words, it didn't need to be a float at all, but anything that could be broadcast up to the proper shape. I realize from looking at the docstring that this wasn't actually a supported use case, but it seems like it should be. I guess that would have to be an enhancement, then?

larsoner · 2015-12-24T17:36:43Z

@ev-br pushed a commit that fixes the use cases you mentioned. It also makes the code more DRY by only doing fill_value setting in one place. Instead of having a bit of duplicated logic in the constructor and the @fill_value.setter, now the constructor calls the setter, so the logic can live in one place.

larsoner · 2015-12-24T17:38:06Z

Sadly this is going to actually break my use case, but I can fix it for my library. I hope it doesn't also silently break other people's code who had been using the broadcastability before, even if it wasn't documented. I could build the broadcastability check instead, if that's a worthwhile enhancement.

ev-br · 2015-12-24T22:10:40Z

Ugh. Yes, broadcasting fill_value is undocumented and untested. But. Assuming it's useful (and I trust you on that!), this is likely more important use case than having two fill_values: there's no need to break good code by small convenience enhancements.

Therefore, I guess it makes sense to i) write a bunch of tests ignoring the two fill_values first, ii) write the definitive statements in the docstring. Then, when it's tested and documented, the question is if we can add two fill_values on top of that in a way that is unambiguous and not too brittle :-). If we can, great. Otherwise, we take two fill_values off until it's too late and it's frozen by being released.

How does this sound? Since you have a use case, how much is it to ask you to work on it? I'm willing to help but I don't know your library and its uses.

larsoner · 2015-12-24T22:24:11Z

I can implement it hopefully in the next few days. The brittle cases will be where there is ambiguity between two values, or a first dimension of length two. Not sure what to do in that case, I'll think on it.

larsoner · 2015-12-28T18:43:40Z

@ev-br do you think it's sufficient to check the type as tuple, list for the case where the lower and upper bounds are set separately? That might take care of the iffy cases.

larsoner · 2015-12-28T19:56:41Z

Okay @ev-br I made it work with broadcasting, modified docstring (should explain the changes properly) and tests, got rid of the warnings during tests about fill_value, and this code works with the case I have been using in my other repo. Ready for review/merge from my end. When it's good to go, I can also squash to a single commit for easier inclusing in master.

larsoner · 2015-12-28T20:10:41Z

scipy/interpolate/interpolate.py

+    # at least one check failed
+    raise ValueError('%s argument must be able to broadcast up '
+                     'to shape %s but had shape %s'
+                     % (name, shape_to, shape_from))


It seems like something like this must already exist in numpy or scipy, but I couldn't find it with a little bit of googling so I implemented it myself

It could likely work with broacast_to (which is new in numpy 1.10 IIRC) in a try-except block?
Anyway, this version seems to work so it's fine IMO.

Okay I'll leave this version then

larsoner · 2015-12-30T16:42:12Z

@ev-br regarding backport -- the checks I added just make the failures come earlier, with a bit clearer error messages. Previously, broadcasting worked if the array was the right size. If the fill_value was the wrong size, it would bomb mentioning a size mismatch only once the fill_value was actually used. But the error message was reasonable, mentioning broadcasting problems. So the gain for backporting the size checks is fairly small. I probably wouldn't bother backporting unless others have reported failures previously, especially since the broadcasting was an implicit, unsupported / undocumented feature.

ev-br · 2015-12-30T22:43:36Z

@Eric89GXL I'm confused now. Is the external code of yours still broken with this PR?

larsoner · 2015-12-30T23:08:06Z

No, it works with this PR

ev-br · 2015-12-31T00:08:10Z

And it's broken with master/rc? Why then it should not be backported to 0.17.x?

larsoner · 2015-12-31T00:33:55Z

Ahh I see what you mean. This is a PR into 0.17 already so I assumed you meant 0.16 backport. Yeah I think it should be in both master and 0.17.

ev-br · 2015-12-31T08:56:12Z

Uh, I see now. We're on the same page, great! Thanks for clarifying it.

The fix looks pretty good. I think we need some more tests checking that an array-valued fill_value is actually used, similar to https://github.com/scipy/scipy/blob/master/scipy/interpolate/tests/test_interpolate.py#L305, for potentially confusing cases: a fill_value being a tuple of length 2, a list of length 2, a 1d array of two elements, a list of two lists (is this a single fill_value?), a tuple of two lists (this should be a pair of fill_values), a 2 by 2 array.

larsoner · 2015-12-31T18:58:16Z

@ev-br good call on the tests, I found a bug with the broadcasting. Ready to go from my end if you're happy with the latest commit.

ev-br · 2015-12-31T19:05:50Z

Will have a careful look a bit later, ran out of time this year :-). Happy New Year!

larsoner · 2015-12-31T19:06:09Z

Sounds good -- happy new year to you, too!

ev-br · 2016-01-04T13:35:12Z

scipy/interpolate/interpolate.py

-        y = self._reshape_yi(y)
+        self._y = self._reshape_yi(self.y)
+        self.x = x
+        del y, x


For my education: What does this do here?

Just makes it clear that the class attributes should be used instead. I'll add a comment. Or if you prefer it can be removed because functionally the code is the same without it.

Up to you, I'm fine either way.

ev-br · 2016-01-04T13:44:42Z

This had grown a bit hairier than anticipated, huh.
The patch looks good, the tests are extensive, but nonetheless this warrants an rc2, I'm afraid. Eric, you mentioned you could squash the commits --- now would be good time. Also this should be discussed in the release notes, would you be able to add a sentence or two please.

larsoner · 2016-01-04T16:32:39Z

This had grown a bit hairier than anticipated, huh.

Yeah, testing and covering all the possible cases took more lines than I thought. But I think the result should be pretty stable.

Eric, you mentioned you could squash the commits --- now would be good time. Also this should be discussed in the release notes, would you be able to add a sentence or two please.

Done and done. The only things changed since you last looked at the code is adding an inline comment to the del statement to explain it, plus changes to the release notes. Let me know if there's anything else.

ev-br · 2016-01-04T17:37:15Z

Great, thanks! I'll keep this open for a short while in case someone wants to have a look. Otherwise, I'll merge it and forward port to master.

FIX: Fix interpolate

ev-br · 2016-01-07T12:05:11Z

Merged, thank you Eric.

larsoner reviewed Dec 24, 2015
View reviewed changes

ev-br added defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.interpolate labels Dec 24, 2015

ev-br added this to the 0.17.0 milestone Dec 24, 2015

larsoner force-pushed the minor-fix branch from a8203f7 to d4bfc20 Compare December 28, 2015 19:59

larsoner reviewed Dec 28, 2015
View reviewed changes

ev-br added the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Dec 28, 2015

larsoner force-pushed the minor-fix branch 4 times, most recently from 00d0d9e to 163c638 Compare December 31, 2015 19:09

ev-br reviewed Jan 4, 2016
View reviewed changes

larsoner force-pushed the minor-fix branch from 163c638 to 32ede1d Compare January 4, 2016 16:32

ENH: Re-enable broadcasting of fill_value

0bdc6da

larsoner force-pushed the minor-fix branch from 32ede1d to 0bdc6da Compare January 4, 2016 16:33

ev-br mentioned this pull request Jan 6, 2016

forward port ENH: Re-enable broadcasting of fill_value #5681

Merged

ev-br added a commit that referenced this pull request Jan 7, 2016

Merge pull request #5624 from Eric89GXL/minor-fix

bf97bc7

FIX: Fix interpolate

ev-br merged commit bf97bc7 into scipy:maintenance/0.17.x Jan 7, 2016

ev-br removed the backport-candidate This fix should be ported by a maintainer to previous SciPy versions. label Jan 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Fix interpolate #5624

FIX: Fix interpolate #5624

larsoner commented Dec 24, 2015

larsoner commented Dec 24, 2015

larsoner Dec 24, 2015

ev-br commented Dec 24, 2015

larsoner commented Dec 24, 2015

larsoner commented Dec 24, 2015

larsoner commented Dec 24, 2015

larsoner commented Dec 24, 2015

ev-br commented Dec 24, 2015

larsoner commented Dec 24, 2015 via email

larsoner commented Dec 28, 2015

larsoner commented Dec 28, 2015

larsoner Dec 28, 2015

ev-br Dec 31, 2015

larsoner Dec 31, 2015 via email

larsoner commented Dec 30, 2015

ev-br commented Dec 30, 2015

larsoner commented Dec 30, 2015 via email

ev-br commented Dec 31, 2015

larsoner commented Dec 31, 2015 via email

ev-br commented Dec 31, 2015

larsoner commented Dec 31, 2015

ev-br commented Dec 31, 2015

larsoner commented Dec 31, 2015

ev-br Jan 4, 2016

larsoner Jan 4, 2016 via email

ev-br Jan 4, 2016

ev-br commented Jan 4, 2016

larsoner commented Jan 4, 2016

ev-br commented Jan 4, 2016

ev-br commented Jan 7, 2016

FIX: Fix interpolate #5624

FIX: Fix interpolate #5624

Conversation

larsoner commented Dec 24, 2015

larsoner commented Dec 24, 2015

larsoner Dec 24, 2015

Choose a reason for hiding this comment

ev-br commented Dec 24, 2015

larsoner commented Dec 24, 2015

larsoner commented Dec 24, 2015

larsoner commented Dec 24, 2015

larsoner commented Dec 24, 2015

ev-br commented Dec 24, 2015

larsoner commented Dec 24, 2015 via email

larsoner commented Dec 28, 2015

larsoner commented Dec 28, 2015

larsoner Dec 28, 2015

Choose a reason for hiding this comment

ev-br Dec 31, 2015

Choose a reason for hiding this comment

larsoner Dec 31, 2015 via email

Choose a reason for hiding this comment

larsoner commented Dec 30, 2015

ev-br commented Dec 30, 2015

larsoner commented Dec 30, 2015 via email

ev-br commented Dec 31, 2015

larsoner commented Dec 31, 2015 via email

ev-br commented Dec 31, 2015

larsoner commented Dec 31, 2015

ev-br commented Dec 31, 2015

larsoner commented Dec 31, 2015

ev-br Jan 4, 2016

Choose a reason for hiding this comment

larsoner Jan 4, 2016 via email

Choose a reason for hiding this comment

ev-br Jan 4, 2016

Choose a reason for hiding this comment

ev-br commented Jan 4, 2016

larsoner commented Jan 4, 2016

ev-br commented Jan 4, 2016

ev-br commented Jan 7, 2016