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: made filters window safe #1338

Merged
merged 11 commits into from Jul 25, 2016

Conversation

Projects
None yet
4 participants
@thewassermann
Contributor

thewassermann commented Jul 20, 2016

filters are now window safe

@ssanderson

This comment has been minimized.

Member

ssanderson commented Jul 20, 2016

@thewassermann the change looks correct. Can we add a test for this? A good place to look would be tests/pipeline/test_filter.py.

A reasonable test would be to do something like define a CustomFactor that takes another filter as an input with a window_length > 1 and does some computation on it (e.g., summing the number of Trues for each asset in the input window).

@thewassermann

This comment has been minimized.

Contributor

thewassermann commented Jul 20, 2016

Will do. Thanks again

On Wed, Jul 20, 2016 at 5:20 PM, Scott Sanderson notifications@github.com
wrote:

@thewassermann https://github.com/thewassermann the change looks
correct. Can we add a test for this? A good place to look would be
tests/pipeline/test_filter.py.

A reasonable test would be to do something like define a CustomFactor
that takes another filter as an input with a window_length > 1 and does
some computation on it (e.g., summing the number of Trues for each asset in
the input window).


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1338 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ALaqDYT2IxwHy9bDoJahSfNPMOnO7oA7ks5qXpEegaJpZM4JRNve
.

Gilbert Wassermann
Harvard College '18
A.B. Candidate in Statistics
617-308-6070

@thewassermann

This comment has been minimized.

Contributor

thewassermann commented Jul 21, 2016

@ssanderson just added a test that would not work if filters was not window safe. I am having some issues setting up the nose tests on my machine, but this test does not raise an error

@ssanderson

This comment has been minimized.

Member

ssanderson commented Jul 21, 2016

but this test does not raise an error

I'm gonna go ahead and disagree with that:

======================================================================
ERROR: test_window_safe (tests.pipeline.test_filter.FilterTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/quantopian/zipline/tests/pipeline/test_filter.py", line 396, in test_window_safe
    TermGraph({'filter': nansum(CustomFilterBool, axis=0)}),
  File "/home/travis/build/quantopian/zipline/zipline/pipeline/graph.py", line 51, in __init__
    self._add_to_graph(term, parents, extra_rows=0)
  File "/home/travis/build/quantopian/zipline/zipline/pipeline/graph.py", line 215, in _add_to_graph
    for dependency, additional_extra_rows in term.dependencies.items():
AttributeError: 'lazyval' object has no attribute 'items'

You're trying to call a numpy function on your CustomFilter type.

@ssanderson

This comment has been minimized.

Member

ssanderson commented Jul 21, 2016

As far as setting up a dev environment goes, the most reliable way to set it up in one shot for just zipline should be to run this from the git root in a fresh virtualenv:

pip install -e .[all] -c etc/requirements.txt --no-binary :all: && pip install -r etc/requirements_blaze.txt -c etc/requirements.txt

when that completes, you should be able to run nosetests tests to run all the tests, or nosetests tests/pipeline to run just the pipeline tests.

@thewassermann

This comment has been minimized.

Contributor

thewassermann commented Jul 21, 2016

so that is definitely incorrect but it did not come up on my machine, probably due to the fact that it is not importing the right modules. will run your one-liner now. Thanks for all the help, I really appreciate it

@thewassermann

This comment has been minimized.

Contributor

thewassermann commented Jul 21, 2016

@ssanderson have modified the test and now it passes

message:

test_window_safe (tests.pipeline.test_filter.FilterTestCase) ... ok (0.0019s)

@thewassermann

This comment has been minimized.

Contributor

thewassermann commented Jul 22, 2016

confirmed windowsafe

class TestFactor(CustomFactor):
dtype = float64_dtype
inputs = (InputFilter(), )

This comment has been minimized.

@ssanderson

ssanderson Jul 22, 2016

Member

extra whitespace here

def compute(self, today, assets, out, filter_):
# sum for each column
out[:] = nansum(filter_, axis=0)

This comment has been minimized.

@ssanderson

ssanderson Jul 22, 2016

Member

NaN is only a thing for floating-point values, so using nansum doesn't really make sense here since your inputs are bools. (It's harmless, since it should just fall back to sum, but it's a little misleading to the reader.). I would just use filter_.sum.

@thewassermann

This comment has been minimized.

Contributor

thewassermann commented Jul 22, 2016

sorted out the discrepancy. sum now used

@ssanderson

This comment has been minimized.

Member

ssanderson commented Jul 22, 2016

Your build is failing because flake8 is marking style failures:

$ flake8 zipline tests
tests/pipeline/test_filter.py:385:1: W293 blank line contains whitespace
tests/pipeline/test_filter.py:418:80: E501 line too long (96 > 79 characters)

You can run the same thing locally from your zipline root with flake8 zipilne tests.

@thewassermann

This comment has been minimized.

Contributor

thewassermann commented Jul 22, 2016

I realized that I didnt do that just now. doing it now

@ssanderson

This comment has been minimized.

Member

ssanderson commented Jul 22, 2016

@thewassermann can you also add a whatsnew entry in the Enhancements section of docs/source/whatsnew.txt? It should just be a one/two-sentence description of the change as it affects a user. In this case it would be something to the effect of `"Filters are now window-safe by default (:issue: issuenum in backticks)". @nathanwolfe can help you out with the format, since he wrote one for the AverageDollarVolume bug.

@coveralls

This comment has been minimized.

coveralls commented Jul 22, 2016

Coverage Status

Coverage increased (+0.003%) to 83.552% when pulling 9c6aa0c on window-safe-filters into 71b2363 on master.

@coveralls

This comment has been minimized.

coveralls commented Jul 22, 2016

Coverage Status

Coverage increased (+0.003%) to 83.552% when pulling 9c6aa0c on window-safe-filters into 71b2363 on master.

def compute(self, today, assets, out, filter_):
# sum for each column
out[:] = sum(filter_, axis=0)

This comment has been minimized.

@ssanderson

ssanderson Jul 22, 2016

Member

We should use filter_.sum rather than importing sum from numpy. sum is a built-in, and it's generally a bad idea to alias builtins. Another thing you could do would be to change the import to rebind sum to something else (e.g. from numpy import sum as np_sum) and then refer to it as np_sum.

This comment has been minimized.

@llllllllll

llllllllll Jul 22, 2016

Member

most people would just import numpy as np and use np.sum

@coveralls

This comment has been minimized.

coveralls commented Jul 22, 2016

Coverage Status

Coverage increased (+0.003%) to 83.552% when pulling ea01fb0 on window-safe-filters into 71b2363 on master.

@coveralls

This comment has been minimized.

coveralls commented Jul 25, 2016

Coverage Status

Coverage increased (+0.09%) to 83.642% when pulling 3cc1cf0 on window-safe-filters into 71b2363 on master.

@coveralls

This comment has been minimized.

coveralls commented Jul 25, 2016

Coverage Status

Coverage increased (+0.004%) to 83.553% when pulling 3cc1cf0 on window-safe-filters into 71b2363 on master.

@ssanderson ssanderson merged commit 75b3dc6 into master Jul 25, 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 window-safe-filters branch Jul 25, 2016

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