BUG: Interpolate limit=n GH16282 #16429

WBare · 2017-05-22T19:32:28Z

closes df.interpolate limit_direction= does not work without limit=x setting #16282
tests added / passed
passes git diff upstream/master --name-only -- '*.py' | flake8 --diff
whatsnew entry

codecov · 2017-05-22T21:21:23Z

Codecov Report

Merging #16429 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16429      +/-   ##
==========================================
- Coverage   90.42%   90.42%   -0.01%     
==========================================
  Files         161      161              
  Lines       51023    51023              
==========================================
- Hits        46138    46137       -1     
- Misses       4885     4886       +1

Flag	Coverage Δ
#multiple	`88.26% <100%> (-0.01%)`	⬇️
#single	`40.17% <0%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/missing.py	`84.27% <100%> (ø)`	⬆️
pandas/core/common.py	`91.05% <0%> (-0.34%)`	⬇️

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 49ec31b...843d427. Read the comment docs.

codecov · 2017-05-22T21:21:23Z

Codecov Report

Merging #16429 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #16429   +/-   ##
=======================================
  Coverage   90.42%   90.42%           
=======================================
  Files         161      161           
  Lines       51023    51023           
=======================================
  Hits        46138    46138           
  Misses       4885     4885

Flag	Coverage Δ
#multiple	`88.26% <100%> (ø)`	⬆️
#single	`40.17% <0%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/missing.py	`84.27% <100%> (ø)`	⬆️
pandas/core/reshape/merge.py	`94.18% <0%> (-0.01%)`	⬇️
pandas/errors/__init__.py	`100% <0%> (ø)`	⬆️

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 49ec31b...d3d9cb3. Read the comment docs.

WBare · 2017-05-22T21:46:30Z

Sorry to have to ask a general questions, but can someone explain how my code coverage can go down in a file that I have not changed?

Codecov correctly identified an uncovered return parameter from a function in common.py, but I have not modified that file.

I went ahead and added the appropriate test (which does not relate to this issue), because it was very simple to cover the case.

I just don't know why codecov is saying my branch reduces coverage in that file if I have not changed it.

jreback · 2017-05-23T10:53:01Z

doc/source/whatsnew/v0.21.0.txt

@@ -106,6 +106,8 @@ Reshaping

 Numeric
 ^^^^^^^
+- DataFrame.interpolate was not respecting limit_direction when using the default limit=None (unlimited). Specifically, it would always use limit_direction='forward' even when limit_direction was set otherwise. Now default limit=None will work with other directions. :issue:`16282`


you can move this to 0.20.0. Make much shorter.

Bug in .interpolate(), where limit_direction was not respected when limit=None (default) was passed (:issue:16282)

Hi Jeff @jreback , 2 questions:

Can you give me a basic pointer on getting this into 0.20.0? i.e. is that a new pull request? Is there a way I can apply my branch to 0.20.0 (I'm just learning GitHub).

What is the proper way to ping when a pull request is ready for review (all green). Just a comment?

Thanks!

I think 0.20 was a typo. Probably mean 0.20.2. There's a whatsnew/v0.20.2.txt. You can move this section there (and make it a bit shorter. Typically just a sentence, and people can click on the link to the issue if they're interested in more).

And yeah, just a comment when you're ready for review, or when you notice that all the checkmarks at the bottom are green and it's ready for review.

Thanks @TomAugspurger, I thought Jeff wanted me to somehow move the change to the 0.20 branch. That change is in. I will ping when green.

jreback

minor comment. ping when pushed and green.

WBare · 2017-05-23T15:25:48Z

All green. Ready for review. Thanks.

TomAugspurger · 2017-05-23T15:35:44Z

pandas/tests/test_common.py

@@ -16,6 +16,7 @@ def test_mut_exclusive():
        com._mut_exclusive(a=1, b=2)
    assert com._mut_exclusive(a=1, b=None) == 1
    assert com._mut_exclusive(major=None, major_axis=None) is None
+    assert com._mut_exclusive(a=None, b=2) == 2


Was this change intentional? If not I can remove it just before merging.

WBare · 2017-05-23T15:53:22Z

Basically, codecov was failing because a particular line of common.py was not covered. I did not touch that file, but I did see that the line needed a test. I added it, and coverage was fixed. You can remove it, but it is actually the correct test for increased coverage, and you can see that it is very simple. (unless someone else has already added that test since yesterday). I still don't know how my coverage can go down in a file I have not touched, but I noticed it was happening to others as well. Thanks

…

On Tue, May 23, 2017 at 11:35 AM, Tom Augspurger ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pandas/tests/test_common.py <#16429 (comment)>: > @@ -16,6 +16,7 @@ def test_mut_exclusive(): com._mut_exclusive(a=1, b=2) assert com._mut_exclusive(a=1, b=None) == 1 assert com._mut_exclusive(major=None, major_axis=None) is None + assert com._mut_exclusive(a=None, b=2) == 2 Was this change intentional? If not I can remove it just before merging. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#16429 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APZUQe8I1TEwxwQLAWgSxkqM8eFr8OOFks5r8vzcgaJpZM4NiySk> .

TomAugspurger · 2017-05-23T15:56:18Z

@WBare thanks, more tests are good :) Just making sure it was intentional. Codecov can be a bit weird about how it counts changes.

Removed extraneous newline

TomAugspurger · 2017-05-23T15:59:01Z

Just pushed a small change to remove the newline in the 0.21 file. Merging.

* BUG: Interpolate limit=n GH16282 * Fix: comment line over the 80 char limit * Test: Added small test for code coverage * DOC: Moved whats new comment from 0.21.0 to 0.20.2 * Update v0.21.0.txt Removed extraneous newline (cherry picked from commit a8a497f)

jreback · 2017-06-02T22:38:05Z

pandas/core/missing.py

+
+    # each possible limit_direction
+    if limit_direction == 'forward':
+        violate_limit = sorted(start_nans |


@TomAugspurger not sure if this is where the perf issue is (maybe because not this IS hit in all code).

these can be done via Index set ops

IOW

start_nans = Index(range(.....)) violate_limit = start_nans.union(Index(_interp_limit(invalid, limit, 0))).sort_values()

for example

TomAugspurger · 2017-06-03T00:14:50Z

Most of the time is in the _interp_limit function actually. I have a vectorized solution I'll push tonight.

…

On Jun 2, 2017, at 17:38, Jeff Reback ***@***.***> wrote: @jreback commented on this pull request. In pandas/core/missing.py: > + # are more than 'limit' away from any non-NaN. + # + # If limit=None, then use default behavior of filling an unlimited number + # of NaNs in the direction specified by limit_direction + + # default limit is unlimited GH #16282 + if limit is None: + limit = len(xvalues) + elif not is_integer(limit): + raise ValueError('Limit must be an integer') + elif limit < 1: + raise ValueError('Limit must be greater than 0') + + # each possible limit_direction + if limit_direction == 'forward': + violate_limit = sorted(start_nans | @TomAugspurger not sure if this is where the perf issue is (maybe because not this IS hit in all code). these can be done via Index set ops IOW start_nans = Index(range(.....)) violate_limit = start_nans.union(Index(_interp_limit(invalid, limit, 0))).sort_values() for example — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

* BUG: Interpolate limit=n GH16282 * Fix: comment line over the 80 char limit * Test: Added small test for code coverage * DOC: Moved whats new comment from 0.21.0 to 0.20.2 * Update v0.21.0.txt Removed extraneous newline

WBare added 2 commits May 22, 2017 15:22

BUG: Interpolate limit=n GH16282

b207ada

Fix: comment line over the 80 char limit

843d427

Test: Added small test for code coverage

e465311

jreback added Bug Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate labels May 23, 2017

jreback reviewed May 23, 2017

View reviewed changes

jreback approved these changes May 23, 2017

View reviewed changes

jreback added this to the 0.20.2 milestone May 23, 2017

DOC: Moved whats new comment from 0.21.0 to 0.20.2

2f7c715

TomAugspurger reviewed May 23, 2017

View reviewed changes

Update v0.21.0.txt

d3d9cb3

Removed extraneous newline

TomAugspurger merged commit a8a497f into pandas-dev:master May 23, 2017

WBare deleted the Fix-Interpolate-Limit-16282 branch May 23, 2017 16:02

TomAugspurger added Backported and removed Needs Backport labels May 30, 2017

TomAugspurger mentioned this pull request Jun 2, 2017

PERF: Regressions between 0.20.1 and 0.20.x #16584

Closed

jreback reviewed Jun 2, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Interpolate limit=n GH16282 #16429

BUG: Interpolate limit=n GH16282 #16429

WBare commented May 22, 2017

codecov bot commented May 22, 2017

codecov bot commented May 22, 2017 •

edited

Loading

WBare commented May 22, 2017

jreback May 23, 2017

WBare May 23, 2017

TomAugspurger May 23, 2017

WBare May 23, 2017

jreback left a comment

WBare commented May 23, 2017

TomAugspurger May 23, 2017

WBare commented May 23, 2017 via email

TomAugspurger commented May 23, 2017

TomAugspurger commented May 23, 2017

jreback Jun 2, 2017

TomAugspurger commented Jun 3, 2017 via email

BUG: Interpolate limit=n GH16282 #16429

BUG: Interpolate limit=n GH16282 #16429

Conversation

WBare commented May 22, 2017

codecov bot commented May 22, 2017

Codecov Report

codecov bot commented May 22, 2017 • edited Loading

Codecov Report

WBare commented May 22, 2017

jreback May 23, 2017

Choose a reason for hiding this comment

WBare May 23, 2017

Choose a reason for hiding this comment

TomAugspurger May 23, 2017

Choose a reason for hiding this comment

WBare May 23, 2017

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

WBare commented May 23, 2017

TomAugspurger May 23, 2017

Choose a reason for hiding this comment

WBare commented May 23, 2017 via email

TomAugspurger commented May 23, 2017

TomAugspurger commented May 23, 2017

jreback Jun 2, 2017

Choose a reason for hiding this comment

TomAugspurger commented Jun 3, 2017 via email

codecov bot commented May 22, 2017 •

edited

Loading