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

Adding a built in Returns factor to the pipeline API #884

Merged
merged 3 commits into from Dec 2, 2015

Conversation

Projects
None yet
2 participants
@TimShawver
Member

TimShawver commented Nov 30, 2015

I followed the RSI example in writing the test. @ssanderson this is ready for you to look at when you get a chance.

# seed(seed_value)
# data = abs(randn(window_length, 3))
# expected = (data[-1] - data[0]) / data[0]
(100, array([-0.99581969, 0.78866002, 0.12724000]), 15),

This comment has been minimized.

@ssanderson

ssanderson Nov 30, 2015

Member

I baked in the expected results for RSI because talib is an optional zipline dependency, so I couldn't just call it directly in the test. For this, we should just compute the expected value directly in the test.

**Default Inputs**: [USEquityPricing.close]
**Default Window Length**: 30

This comment has been minimized.

@ssanderson

ssanderson Nov 30, 2015

Member

I don't think this should have a default window_length.

class Returns(CustomFactor):
"""
Calculates the % change in close price over the given window_length
(i.e. the returns for a given asset).

This comment has been minimized.

@ssanderson

ssanderson Nov 30, 2015

Member

I'd remove the parenthetical note here.

@@ -25,6 +25,22 @@
from .factor import CustomFactor
class Returns(CustomFactor):
"""
Calculates the % change in close price over the given window_length

This comment has been minimized.

@ssanderson

ssanderson Nov 30, 2015

Member

% should probably be written out as percent.

@TimShawver

This comment has been minimized.

Member

TimShawver commented Dec 1, 2015

@ssanderson thanks for the feedback, I've made those change. Let me know if there's anything else.

@TimShawver TimShawver force-pushed the returns-factor branch from 1df8906 to 631a187 Dec 1, 2015

@ssanderson

This comment has been minimized.

Member

ssanderson commented Dec 1, 2015

LGTM

@ssanderson

This comment has been minimized.

Member

ssanderson commented Dec 1, 2015

@TimShawver can you add a whatsnew for this?

TimShawver added some commits Dec 1, 2015

Add the built-in factors to the API docs for zipline.io, so that I ca…
…n link to the Returns factor from the release notes.
@TimShawver

This comment has been minimized.

Member

TimShawver commented Dec 1, 2015

@ssanderson I added a whatsnew and also added our built-in factors to the API docs for zipline.io. I noticed they were missing when I tried to link to the new Returns factor from the release notes.

TimShawver added a commit that referenced this pull request Dec 2, 2015

Merge pull request #884 from quantopian/returns-factor
Adding a built in Returns factor to the pipeline API

@TimShawver TimShawver merged commit ce3727b into master Dec 2, 2015

2 checks passed

Scrutinizer 5 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@TimShawver TimShawver deleted the returns-factor branch Dec 2, 2015

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