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

BUG: Fix factor.top(1) with no groupby. #2218

Merged
merged 4 commits into from Jun 22, 2018

Conversation

Projects
None yet
4 participants
@ssanderson
Member

ssanderson commented Jun 21, 2018

No description provided.

@ssanderson ssanderson force-pushed the fix-factor-top-1 branch from 9798372 to b54501e Jun 21, 2018

@ssanderson ssanderson requested a review from dmichalowicz Jun 21, 2018

@@ -593,6 +595,10 @@ class MaximumFilter(Filter, StandardOutputs):
window_length = 0
def __new__(cls, factor, groupby, mask):
from zipline.pipeline.classifiers import Everything
if groupby is NotSpecified:
groupby = Everything()

This comment has been minimized.

@yankees714

yankees714 Jun 21, 2018

Contributor

If this import is going to be lazy anyway, is there a reason not to put it inside the if block?

This comment has been minimized.

@ssanderson

ssanderson Jun 21, 2018

Member

Probably not. I can move this down.

@dmichalowicz

Left 2 small questions, otherwise looks good.

@@ -593,6 +595,10 @@ class MaximumFilter(Filter, StandardOutputs):
window_length = 0
def __new__(cls, factor, groupby, mask):
from zipline.pipeline.classifiers import Everything

This comment has been minimized.

@dmichalowicz

dmichalowicz Jun 21, 2018

Contributor

Why does this import happen here?

This comment has been minimized.

@ssanderson

ssanderson Jun 21, 2018

Member

We can't import classifiers at module scope here because the classifiers module imports filters, because a bunch of the Classifier methods return Filter instances. I don't think there's a straightforward way to break that circular dependency more cleanly than this.

factor = TestingDataSet.float_col.latest
maximum = factor.top(1)
pipe = Pipeline({'factor': factor, 'maximum': maximum})

This comment has been minimized.

@dmichalowicz

dmichalowicz Jun 21, 2018

Contributor

Any reason for including the actual factor in the pipeline?

This comment has been minimized.

@ssanderson

ssanderson Jun 21, 2018

Member

The line below that does result.groupby(level=0).max() is using the factor to re-compute the max to verify the result that came out of pipeline.

@coveralls

This comment has been minimized.

coveralls commented Jun 21, 2018

Coverage Status

Coverage decreased (-0.07%) to 86.306% when pulling c417675 on fix-factor-top-1 into 40ba0ea on master.

ssanderson added some commits Jun 21, 2018

TEST: Clean up test.
Don't compute max() of a boolean column; just compare the floats.

@ssanderson ssanderson merged commit 7056a90 into master Jun 22, 2018

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 fix-factor-top-1 branch Jun 22, 2018

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