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

Dnt list #478

Merged
merged 7 commits into from Feb 5, 2015

Conversation

Projects
None yet
5 participants
@fawce
Member

fawce commented Jan 29, 2015

adding support for passing a function to do not order, for dynamic restricted lists.

"""
super(RestrictedListOrder, self).__init__()
if hasattr(restricted_list, '__call__'):

This comment has been minimized.

@llllllllll

llllllllll Jan 29, 2015

Member
if callable(restricted_list):
    ...

it might be brought to 1 line with:

self.restricted_list = restricted_list if callable(restricted_list) else (lambda: restricted_list)

however that is more of a style thing.

super(RestrictedListOrder, self).__init__()
# if passed a callable, call it, otherwise
# wrap what we were passed in a lambda
self.restricted_list = \

This comment has been minimized.

@ssanderson

ssanderson Feb 3, 2015

Member

What benefit is gained by allowing the user to pass a callable here? Since the function can take no arguments, the only way this can ever return something different on different calls is if it's closing over global mutable state, which seems like an idiom we want to discourage (in particular, this is the doomsday scenario for serialization).

self._leveraged_etf = None
@property
def LEVERAGED_ETF_LIST(self):

This comment has been minimized.

@llllllllll

llllllllll Feb 4, 2015

Member

why is this all caps?

This comment has been minimized.

@fawce

fawce Feb 5, 2015

Member

BECAUSE I WAS THINKING OF IT LIKE AN ENUM. But, that's not right, so I can drop the caps :).

def __contains__(self, item):
rl = self.get_restricted_list()
return rl.__contains__(item)

This comment has been minimized.

@llllllllll

llllllllll Feb 4, 2015

Member

item in self.get_restricted_list()

rl = self.get_restricted_list()
return rl.__contains__(item)
def get_restricted_list(self):

This comment has been minimized.

@llllllllll

llllllllll Feb 4, 2015

Member

could this be a property instead of an getter?

self._knowledge_dates = self.make_knowledge_dates(self.data)
self.current_date = current_date_func
self.count = 0
self._list = set()

This comment has been minimized.

@llllllllll

llllllllll Feb 4, 2015

Member

I still think it is confusing that self._list is not a list, could we call this _security_list or something that is farther from the type name?

tzinfo=pytz.utc)
data[kd] = {}
kd_path = os.path.join(dir_path, kd_name)
for ld_name in listdir(dir_path + '/' + kd_name):

This comment has been minimized.

@llllllllll

llllllllll Feb 4, 2015

Member

os.path.join

from six import itervalues
import os
import os.path

This comment has been minimized.

@llllllllll

llllllllll Feb 4, 2015

Member

I believe that importing os is enough.

ld_path = os.path.join(kd_path, ld_name)
for fname in listdir(ld_path):
fpath = os.path.join(ld_path, fname)
with open(fpath) as f:

This comment has been minimized.

@llllllllll

llllllllll Feb 4, 2015

Member

At this point in the function, we are 3 loops in and adding a new indentation block, could we try to refactor out some of the subloops?

def __init__(self, restricted_list):
"""
restricted list can be an iterable, or
an object that implements __contains__ for dynamic

This comment has been minimized.

@llllllllll

llllllllll Feb 4, 2015

Member

restricted_list can be an iterable or container for dynamic restrictions.

I think the convention is to say that something that implements __contains__ is a container; however, this is not a big deal if you think this is more clear.

fawce added some commits Jan 24, 2015

modified do not order guard to take an iteratble or a container
container allows for dynamic restrictions, necessary for a
point in time implementation of the restricted list.
fixed catastrophic bug in load_from_directory
and added a new test case
was not iterating over lookup date directory names, and
therefore mising all by one list of stocks.
discovered because of differing sort orders between
my local machine, other devs, and travis ci.

@fawce fawce force-pushed the dnt_list branch from b0ff09d to 412baa3 Feb 5, 2015

@coveralls

This comment has been minimized.

coveralls commented Feb 5, 2015

Coverage Status

Coverage increased (+0.28%) to 87.14% when pulling 412baa3 on dnt_list into 4255016 on master.

fawce added a commit that referenced this pull request Feb 5, 2015

Merge pull request #478 from quantopian/dnt_list
ENH: no order guard, security lists

@fawce fawce merged commit ffe5a7a into master Feb 5, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@fawce fawce deleted the dnt_list branch Feb 5, 2015

@jikamens

This comment has been minimized.

Contributor

jikamens commented on zipline/utils/security_list.py in 536ace9 Apr 30, 2015

I'm prety sure this should use os.path.dirname(zipline.__file__) rather than os.path.join(*zipline.__path__). @fawce @llllllllll can you comment?

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