Fix interpolate -limit Add interpolate limit_direction='inside' #16307

WBare · 2017-05-09T18:10:18Z

[ x] closes df.interpolate limit_direction= does not work without limit=x setting #16282 and Enhancement Request: control extrapolation on .interpolate #16284
[x ] tests added / passed
[ x] passes git diff upstream/master --name-only -- '*.py' | flake8 --diff
[ x] whatsnew entry

crayxt · 2017-05-10T04:08:07Z

doc/source/whatsnew/v0.21.0.txt

@@ -20,6 +20,12 @@ Check the :ref:`API Changes <whatsnew_0210.api_breaking>` and :ref:`deprecations
 New features
 ~~~~~~~~~~~~

+- DataFrame.interpolate() has a new setting: limit_direction='inside'.
+    This will cause the interpolation to fill missing values only when
+    the missing value is surounded by valid values.  It is useful when 


jreback · 2017-05-10T11:15:03Z

pls add tests for any changes.

WBare · 2017-05-12T10:23:52Z

Sorry for the oversight. I will get the tests added. I also realized I probably should have done two separate pulls. On for the fix and one for the enhancement. Also, Baurzhan suggested a change from limit_direction='inside' to surrounded. Any thoughts on this? I'm thinking 'inside' sounds more like a direction, but I will make it whatever.

…

On May 10, 2017 7:15 AM, "Jeff Reback" ***@***.***> wrote: pls add tests for any changes. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#16307 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APZUQdS16yqKS4i86LHkyuQ9djmR5tR0ks5r4ZxDgaJpZM4NVrqz> .

WBare · 2017-05-17T17:56:50Z

I've added required tests for the limit direction feature. I see them in my fork and I see them referenced in the pull request.

It should (I think) be ready to go, but I'm not sure if I need to do anything to update the pull. If I've missed anything else, just let me know. This is my first contribution, so please bear with me.

Thanks

codecov · 2017-05-17T18:28:42Z

Codecov Report

Merging #16307 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16307      +/-   ##
==========================================
+ Coverage   90.38%   90.41%   +0.02%     
==========================================
  Files         161      161              
  Lines       50999    51001       +2     
==========================================
+ Hits        46096    46111      +15     
+ Misses       4903     4890      -13

Flag	Coverage Δ
#multiple	`88.24% <100%> (+0.02%)`	⬆️
#single	`40.19% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/missing.py	`84.37% <100%> (+0.09%)`	⬆️
pandas/core/indexes/datetimes.py	`95.33% <0%> (+0.09%)`	⬆️
pandas/plotting/_converter.py	`65.05% <0%> (+1.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6fcec6...aa92985. Read the comment docs.

WBare · 2017-05-22T13:08:23Z

Hi Guys,

Looks like I got all checks passed, coverage increased with new tests, I got my spelling error fixed, and I currently have no conflicts with the base branch.

May I get a review on this? It is actually a fairly simple change. I just had to learn the process for submitting.

Thanks!

TomAugspurger

The new limit_direction='inside' seems a bit confusing to me. We use that to refer to the number of consecutive NaNs to fill. With this change, it's either that, or a special case for "out of valid bounds".

How about we add a new keyword that mimics fill_value from https://docs.scipy.org/doc/scipy/reference/generated/scipy.interpolate.interp1d.html#scipy.interpolate.interp1d. We can call it either fill_value or extrapolate. I think that will require more discussion that the bugfix, so could you split it off of this change and make a new PR?

TomAugspurger · 2017-05-22T13:12:30Z

doc/source/whatsnew/v0.21.0.txt

+    This will cause the interpolation to fill missing values only when
+    the missing value is surrounded by valid values.  It is useful when 
+    a series needs to be interpolated, but must not expand into NaN
+    values that were outside the range of the original series. (GH16284)


The reference to the issue should be like

:issue:`16284`

TomAugspurger · 2017-05-22T13:14:45Z

doc/source/whatsnew/v0.21.0.txt

+- DataFrame.interpolate was not respecting limit_direction when
+    limit=0 (unlimited).  Specifically, it would always use
+    limit_direction='forward' even when specified otherwise. Now
+    default limit=0 will work with other directions. (GH16282)


Same comment about the reference

TomAugspurger · 2017-05-22T13:19:18Z

pandas/tests/series/test_missing.py

@@ -931,6 +931,40 @@ def test_interp_limit_forward(self):
                               limit_direction='FORWARD')
        assert_series_equal(result, expected)

+    def test_interp_limit_inside(self):
+        # these test are for issue #16307 'inside' direction


You can just change this comment to GH #16307 or the link to the issue.

WBare · 2017-05-22T14:16:36Z

Hi Tom,

Please help me understand your comment. There are currently two parameters in use:
limit=n This is the NUMBER of missing values to fill in a given direction.
limit_direction='param' is an already existing parameter indicating the direction in which values should be filled. Specifically, to the already existing values, I added a new direction of 'inside'.

forward - fill forward including values past the last non-NaN
backward - fill backward including values before the last non-NaN
both - fill both directions including values before and after the last non-NaN
ADD: inside - fill values that are surrounded by non-NaN (will only fill missing values that are surrounded by non-NaN and thus will not expand size of the series).

The new direction 'inside' fits with the other existing directions. Please help me understand the need to add a new parameter.

Thanks!

TomAugspurger · 2017-05-22T14:36:08Z

Your test kind of shows it (`s.interpolate(method='linear', limit=1, limit_direction='inside'):

s = Series([np.nan, 1, 3, np.nan, np.nan, np.nan, 11, np.nan])
expected = Series([np.nan, 1, 3, 5, np.nan, 9, 11, np.nan])

It looks like that's a limit_direction="both", but without extrapolate. You could imagine a case where the user wants to only fill forward (limit_direction="forward") but not extrapolate, so their expected is

expected = Series([np.nan, 1, 3, 5, np.nan, np.nan, 11, np.nan])
#                                           ^ this one changed to still be nan

Basically, extrapolation is different that interpolation ;)

WBare · 2017-05-22T14:59:07Z

Ah... right. Yes, I agree with you. My key issue was actually with the fact that we are not differentiating between extrapolating and interpolating. I was fixing one case, but a general solution would cover all the cases.

By default scypi interpolates, and by default pandas interpolates AND extrapolates.

Perhaps:
limit_type=None (default to interpolate and extrapolate as it is today)
limit_type=interpolate (only fills inside values)
limit_type=extrapolate (only fills outside values)

Or better yet... maybe:
interpolate=True (by default... False to turn it off)
extrapolate=True (by default... False to turn it off)

Since I'm in the middle of this, I'd be happy to do this if you guys want it done. Just let me know what the core contributors decide.

In the mean time, I will revert my code to master and just apply code required for the limit=n bug fix (I should not have combined these in the first place), then submit changes again.

Thanks!

TomAugspurger · 2017-05-22T15:22:40Z

@WBare let's move the extrapolate discussion back to #16284

Fix interpolate -limit Add interpolate limit_direction='inside'

e2d2c4b

crayxt reviewed May 10, 2017

View reviewed changes

WBare added 3 commits May 17, 2017 13:34

Prior change incorrectly eliminated -limit type check

8c08696

TST: Add test for -limit_direction and -limit=None default

50e3f71

Style: Remove repeated blank lines around new tests

47daeaf

WBare added 2 commits May 21, 2017 15:38

Merge branch 'master' into Fix-Interpolate-Limit

df32de2

Fix spelling error

aa92985

TomAugspurger reviewed May 22, 2017

View reviewed changes

TomAugspurger mentioned this pull request May 22, 2017

Enhancement Request: control extrapolation on .interpolate #16284

Closed

WBare closed this May 22, 2017

WBare deleted the Fix-Interpolate-Limit branch May 22, 2017 15:34

WBare restored the Fix-Interpolate-Limit branch May 22, 2017 15:34

WBare deleted the Fix-Interpolate-Limit branch May 22, 2017 15:41

TomAugspurger added this to the No action milestone May 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix interpolate -limit Add interpolate limit_direction='inside' #16307

Fix interpolate -limit Add interpolate limit_direction='inside' #16307

WBare commented May 9, 2017

crayxt May 10, 2017

jreback commented May 10, 2017

WBare commented May 12, 2017 via email

WBare commented May 17, 2017

codecov bot commented May 17, 2017 •

edited

Loading

WBare commented May 22, 2017

TomAugspurger left a comment

TomAugspurger May 22, 2017

TomAugspurger May 22, 2017

TomAugspurger May 22, 2017

WBare commented May 22, 2017

TomAugspurger commented May 22, 2017

WBare commented May 22, 2017 •

edited

Loading

TomAugspurger commented May 22, 2017

Fix interpolate -limit Add interpolate limit_direction='inside' #16307

Fix interpolate -limit Add interpolate limit_direction='inside' #16307

Conversation

WBare commented May 9, 2017

crayxt May 10, 2017

Choose a reason for hiding this comment

jreback commented May 10, 2017

WBare commented May 12, 2017 via email

WBare commented May 17, 2017

codecov bot commented May 17, 2017 • edited Loading

Codecov Report

WBare commented May 22, 2017

TomAugspurger left a comment

Choose a reason for hiding this comment

TomAugspurger May 22, 2017

Choose a reason for hiding this comment

TomAugspurger May 22, 2017

Choose a reason for hiding this comment

TomAugspurger May 22, 2017

Choose a reason for hiding this comment

WBare commented May 22, 2017

TomAugspurger commented May 22, 2017

WBare commented May 22, 2017 • edited Loading

TomAugspurger commented May 22, 2017

codecov bot commented May 17, 2017 •

edited

Loading

WBare commented May 22, 2017 •

edited

Loading