Skip to content

Conversation

@priyankjain
Copy link
Contributor

@priyankjain priyankjain commented Jun 14, 2016

Added functionality in validate function of Rolling to ensure that window size is non-negative.

@codecov-io
Copy link

codecov-io commented Jun 14, 2016

Current coverage is 84.32%

Merging #13441 into master will increase coverage by <.01%

@@             master     #13441   diff @@
==========================================
  Files           138        138          
  Lines         51068      51072     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43065      43069     +4   
  Misses         8003       8003          
  Partials          0          0          

Powered by Codecov. Last updated by b06bc7a...26c9b2d

@jreback jreback added Bug Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Jun 14, 2016
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a another place where this is checked. in _Window (so need a test for that as well)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do this inside the elif

elif com.is_integer(window):
    if window < 0:
          raise ......
   ...

Copy link
Contributor Author

@priyankjain priyankjain Jun 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, the test case
series.rolling(0, win_type='boxcar') fails,
should we change travis configuration file (requirements-3.5_OSX.build) to have scipy installation as part of the build or change the test case to handle the import error? @jreback

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change the test case to skip if scipy is not installed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this is left off on purpose

@priyankjain priyankjain force-pushed the novice-bug-fixes branch 3 times, most recently from 9da3800 to 967ffb8 Compare June 16, 2016 15:56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make this a separate test (as skipping will cause the entire rest of the test to be skipped)

@jreback jreback added this to the 0.18.2 milestone Jun 21, 2016
@jreback jreback closed this in 0f351dc Jun 21, 2016
@jreback
Copy link
Contributor

jreback commented Jun 21, 2016

thanks!

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

Labels

Bug Reshaping Concat, Merge/Join, Stack/Unstack, Explode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Series.rolling/DataFrame.rolling don't fully check arguments, have odd behavior when used with invalid inputs

3 participants