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

ENH: Add previous/next to interp1d #8572

Merged
merged 1 commit into from Apr 6, 2018

Conversation

Projects
None yet
5 participants
@prisae
Copy link
Contributor

prisae commented Mar 17, 2018

Add next/previous to interp1d. Useful for instance when the x-axis is time, and your interpolation has to be causal, hence a change in signal only happens at t_x, and before the signal remains at the value of t_(x-1).

I realized a bit late that a very similar pull request was made at #6718. The approach here is slightly different, but it is beyond me to say which one is better or more adequate. As there are already two pull requests for the same feature I hope that you will include one or the other.

I included a few basic tests, copied from the nearest interpolation. Let me know if more are required.

Matlab has similar functions, as mentioned by @rdturnermtl in the above pull request, see https://mathworks.com/help/matlab/ref/interp1.html.

@prisae

This comment has been minimized.

Copy link
Contributor

prisae commented Mar 17, 2018

@Phillip-M-Feldman

This comment has been minimized.

Copy link

Phillip-M-Feldman commented Mar 18, 2018

The example in the code at https://github.com/prisae/tmp-share/blob/master/interp1d-left-right.ipynb seems very clean. I like this interface.

Interpolating to next can be thought of as a special case of a causal interpolating filter.

@prisae

This comment has been minimized.

Copy link
Contributor

prisae commented Mar 18, 2018

With the additional commit it works now fine for nd-arrays as well. I added previous/next to all tests which are run by nearest, so it is tested also for complex numbers and nd-arrays. I hope these tests are sufficient.

@@ -486,6 +488,19 @@ def __init__(self, x, y, kind='linear', axis=-1,
self.x_bds = self.x_bds[1:] + self.x_bds[:-1]

self._call = self.__class__._call_nearest
elif kind == 'previous':
# Side for np.searchsorted and index for clipping
self.side = 'left'

This comment has been minimized.

@ev-br

ev-br Mar 19, 2018

Member

Please add a leading underscore to private attributes, i.e., self._side and self._ind

self.side = 'left'
self.ind = 0
# Move x by one floating point value to the left
self.x_bds = np.nextafter(self.x, self.x-1)

This comment has been minimized.

@ev-br

ev-br Mar 19, 2018

Member

Not sure, does this work if diff(self.x) < 1, would it not skip a point?

This comment has been minimized.

@prisae

prisae Mar 19, 2018

Contributor

Not sure what you mean. np.nextafter(x, x-1) is equal to np.nextafter(x, x-1000) or np.nextafter(x, x-np.infty), the second argument is only to indicate the direction in which it will return the next floating-point value after x.

E.g., np.nextafter(1, 1-1) = np.nextafter(1, 1-1000) = 0.99999999999999989, on my machine.

@pv

This comment has been minimized.

Copy link
Member

pv commented Mar 19, 2018

@prisae

This comment has been minimized.

Copy link
Contributor

prisae commented Mar 19, 2018

@pv this behaviour is not obvious to me from the docs in np.nextafter. Do you suggest to replace the np.nextafter(x, x-1) by np.nextafter(x, x-np.inf)? In the docs it says that the second array is simply The direction where to look for the next representable value of x1, so I would assume that np.nextafter(x, x-1) and np.nextafter(x, x-np.inf) should yield the same result...

@prisae

This comment has been minimized.

Copy link
Contributor

prisae commented Mar 19, 2018

Another possibility would be to replace -1 by 0.9*x, and +1 by 1.1*x:

>>> x = 1e19
>>> np.nextafter(x, 0.9*x) == x
False
>>> np.nextafter(x, 0.9*x) == np.nextafter(x, x-np.inf)
True

What would you prefer?

@WarrenWeckesser

This comment has been minimized.

Copy link
Member

WarrenWeckesser commented Mar 19, 2018

@prisae The problem is not nextafter; it is that when x = 1e19, then x - 1 is equal to x, because 1 is smaller than the unit of least precision of such a large floating point number:

In [195]: x = 1e19

In [196]: x == x - 1
Out[196]: True

Instead of nextafter(x, x-1), why not use nextafter(x, -np.inf)?

@prisae

This comment has been minimized.

Copy link
Contributor

prisae commented Mar 20, 2018

Thanks @ev-br, @pv, and @WarrenWeckesser for your inputs regarding np.nextafter, I didn't realize the issue with very big numbers. I changed it now to np.nextafter(self.x, +/-np.inf). Let me know if there are any other changes required.

@prisae

This comment has been minimized.

Copy link
Contributor

prisae commented Mar 26, 2018

Following up on yesterdays email by @rgommers: Could this be a candidate for the 1.1.0 milestone (given that it is a relatively smallish commit)? Or is more work required?

@ev-br

This comment has been minimized.

Copy link
Member

ev-br commented Mar 31, 2018

The PR looks good. Will need to be squash-merged (or rebased and squashed manually)

@prisae

This comment has been minimized.

Copy link
Contributor

prisae commented Apr 1, 2018

Thanks for your answer @ev-br. Should I do the squashing or whoever will merge it will do?

@ev-br

This comment has been minimized.

Copy link
Member

ev-br commented Apr 1, 2018

If you can squash and rebase locally, great. Otherwise, no problem --- this PR seems small enough for squash-and-merge. I think I'll leave this open for a few more days in case someone wants to take a look. Otherwise I'm going to hit the green button soon (tm).

@ev-br ev-br added this to the 1.1.0 milestone Apr 1, 2018

@prisae prisae force-pushed the prisae:interp1d branch from c9956c4 to 01c88e9 Apr 2, 2018

@prisae

This comment has been minimized.

Copy link
Contributor

prisae commented Apr 2, 2018

I hope I did that correctly, was a first for me regarding rebasing/squashing.

@ev-br ev-br merged commit 98649cd into scipy:master Apr 6, 2018

4 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
codecov/project 76.25% (target 1%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ev-br

This comment has been minimized.

Copy link
Member

ev-br commented Apr 6, 2018

Thanks @prisae, it's in.

The notebook with an illustration is nice, might be worth including in either the cookbook, or the interpolate tutorial.

@prisae

This comment has been minimized.

Copy link
Contributor

prisae commented Apr 6, 2018

Great @ev-br . I will add the example to the tutorial and create a pull request soon.

@prisae prisae deleted the prisae:interp1d branch Apr 6, 2018

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