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: Dont allow length=1 regressions/correlations. #1466

Merged
merged 3 commits into from Sep 2, 2016

Conversation

Projects
None yet
5 participants
@ssanderson
Member

ssanderson commented Sep 2, 2016

They're not meaningful, and they cause warnings from numpy.

Implemented in terms of a new preprocessor, expect_bounded, which
takes a tuple of upper_bound and lower_bound.

ENH: Dont allow length=1 regressions/correlations.
They're not meaningful, and they cause warnings from numpy.

Implemented in terms of a new preprocessor, `expect_bounded`, which
takes a tuple of `upper_bound` and `lower_bound`.
def expect_bounded(**named):
"""
Preprocessing decorator that verifies inputs fall between upper and lower
bounds.

This comment has been minimized.

@richafrank

richafrank Sep 2, 2016

Member

Could you add a comment here about the bounds being inclusive or exclusive?

This comment has been minimized.

@ssanderson

ssanderson Sep 2, 2016

Member

Updated.

def _expect_bounded(bounds):
(lower, upper) = bounds
if lower is None:
should_fail = lambda value: value > upper

This comment has been minimized.

@richafrank

richafrank Sep 2, 2016

Member

👍 for the name should_fail - much clearer than pred.

@coveralls

This comment has been minimized.

coveralls commented Sep 2, 2016

Coverage Status

Coverage decreased (-0.4%) to 86.035% when pulling c84b5ad on disallow-length-1-regressions into 768cdb7 on master.

@jdricklefs

This comment has been minimized.

Member

jdricklefs commented Sep 2, 2016

Wow, this is a fancy fix. Thanks @ssanderson

@coveralls

This comment has been minimized.

coveralls commented Sep 2, 2016

Coverage Status

Coverage increased (+0.03%) to 86.478% when pulling c84b5ad on disallow-length-1-regressions into 768cdb7 on master.

@richafrank

This comment has been minimized.

Member

richafrank commented Sep 2, 2016

LGTM

@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented Sep 2, 2016

Thanks for this Scott! Good catch on the length-1 tests

@coveralls

This comment has been minimized.

coveralls commented Sep 2, 2016

Coverage Status

Coverage increased (+0.2%) to 86.608% when pulling 2e50a09 on disallow-length-1-regressions into 768cdb7 on master.

@ssanderson ssanderson merged commit 9a301dc into master Sep 2, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ssanderson ssanderson deleted the disallow-length-1-regressions branch Sep 2, 2016

bartosh pushed a commit to bartosh/zipline that referenced this pull request Sep 27, 2016

Merge pull request quantopian#1466 from quantopian/disallow-length-1-…
…regressions

ENH: Dont allow length=1 regressions/correlations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment