Skip to content
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: added smoothing to zipline #1358

Merged
merged 8 commits into from Aug 2, 2016
Merged

ENH: added smoothing to zipline #1358

merged 8 commits into from Aug 2, 2016

Conversation

@thewassermann
Copy link
Contributor

@thewassermann thewassermann commented Aug 1, 2016

smoothing filter added to zipline

@thewassermann
Copy link
Contributor Author

@thewassermann thewassermann commented Aug 1, 2016

making docs now. needed to submit pr first to get issue number

@coveralls
Copy link

@coveralls coveralls commented Aug 1, 2016

Coverage Status

Coverage increased (+0.003%) to 85.534% when pulling 2d5a205 on smoothing into 9103516 on master.

@coveralls
Copy link

@coveralls coveralls commented Aug 1, 2016

Coverage Status

Coverage increased (+0.003%) to 85.534% when pulling 2d5a205 on smoothing into 9103516 on master.

"""
A Filter that requires its inputs to have
been True for the last `window_length` days.
An integral part of the Q500US methodology

This comment has been minimized.

@ssanderson

ssanderson Aug 1, 2016
Contributor

This note doesn't belong here. A reader of Zipline won't necessarily know what Q500US is, and nothing about the behavior here is specific to that project.

"""

def compute(self, today, assets, out, arg):
out[:] = (sum_(arg, axis=0) == self.window_length)

This comment has been minimized.

@ssanderson

ssanderson Aug 1, 2016
Contributor

I would probably just use arg.sum here instead of the top-level np.sum.

@@ -488,3 +489,18 @@ def _compute(self, arrays, dates, assets, mask):
asset=self._asset, start_date=dates[0], end_date=dates[-1],
)
return out


class SmoothingFilter(CustomFilter):

This comment has been minimized.

@ssanderson

ssanderson Aug 1, 2016
Contributor

I think we need a more specific name for this. There are lots of ways we can implement smoothing. This is one specific way of doing it.

This comment has been minimized.

@thewassermann

thewassermann Aug 1, 2016
Author Contributor

Here are some ideas:
AllTrueInWindowFilter
AllTrueSmoothingFilter
AllTrueFilter
StrictSmoothingFilter
StrictlyTrueFilter

Any of these work/inspire workable ideas?

from zipline.pipeline.filters import SmoothingFilter

data = full(self.default_shape, True, dtype=bool)
# one column all false

This comment has been minimized.

@ssanderson

ssanderson Aug 1, 2016
Contributor

This comment isn't true, you have an array with the first two diagonal values False.

This comment has been minimized.

@ssanderson

ssanderson Aug 1, 2016
Contributor

A more robust test case for this is ~np.eye(self.default_shape, dtype=bool), which gives you True values everywhere except the diagonal. Though honestly, the easiest might just be to write out the array as a literal and then write out the expected output as a literal as well.

This comment has been minimized.

@thewassermann

thewassermann Aug 1, 2016
Author Contributor

i updated the code but not the comment apparently, ill use np.eye

expected_result[0] = False
expected_result[1] = False
check_arrays(
results['smoothing'].flatten(),

This comment has been minimized.

@ssanderson

ssanderson Aug 1, 2016
Contributor

This is losing information. If expected_result was 4 x 4, and the actual output came back as 2 x 8, this wouldn't catch the error. Instead of flattening here, you should reshape expected_result to the shape that's actually expected.

@coveralls
Copy link

@coveralls coveralls commented Aug 1, 2016

Coverage Status

Coverage increased (+0.003%) to 85.534% when pulling 73de8e6 on smoothing into 9103516 on master.

@coveralls
Copy link

@coveralls coveralls commented Aug 1, 2016

Coverage Status

Coverage increased (+0.003%) to 85.534% when pulling c10af2a on smoothing into 9103516 on master.

"""

def compute(self, today, assets, out, arg):
out[:] = (sum(arg) == self.window_length)

This comment has been minimized.

@ssanderson

ssanderson Aug 1, 2016
Contributor

This is badly incorrect:

In [1]: data = np.arange(25).reshape(5, 5)

In [2]: data
Out[2]:
array([[ 0,  1,  2,  3,  4],
       [ 5,  6,  7,  8,  9],
       [10, 11, 12, 13, 14],
       [15, 16, 17, 18, 19],
       [20, 21, 22, 23, 24]])

In [3]: sum(data)
Out[3]: 300

If tests are passing here, then we're missing coverage in a serious way.

@coveralls
Copy link

@coveralls coveralls commented Aug 1, 2016

Coverage Status

Coverage increased (+0.4%) to 85.915% when pulling 7623c0f on smoothing into 9103516 on master.

@thewassermann
Copy link
Contributor Author

@thewassermann thewassermann commented Aug 1, 2016

unsure why this is failing the build. have checked all tests and flake8...

@coveralls
Copy link

@coveralls coveralls commented Aug 1, 2016

Coverage Status

Coverage increased (+0.01%) to 85.54% when pulling 574d7b1 on smoothing into 9103516 on master.

Scott Sanderson and others added 2 commits Aug 2, 2016
Also, moved All() and Any() to `zipline.pipeline.filters.smoothing`.
ENH: Rename StrictlyTrue to All and add Any().
@thewassermann
Copy link
Contributor Author

@thewassermann thewassermann commented Aug 2, 2016

have had a look and run the tests on my machine. looks amazing. ready to merge when the ci tests finish/you give the word

@ssanderson
Copy link
Contributor

@ssanderson ssanderson commented Aug 2, 2016

LGTM

@ssanderson ssanderson merged commit 129d16f into master Aug 2, 2016
2 checks passed
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 smoothing branch Aug 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.