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

Add security class #520

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@jfkirk
Contributor

jfkirk commented Mar 3, 2015

Adds a cython Security class. This is the first step to adding more types of assets to Zipline for simulation.

@ehebert

This comment has been minimized.

Member

ehebert commented Mar 3, 2015

ACK'd
On Mar 3, 2015 5:22 PM, "James Kirk" notifications@github.com wrote:

Assigned #520 #520 to @ehebert
https://github.com/ehebert.


Reply to this email directly or view it on GitHub
#520 (comment).

# IMPORTANT NOTE: You must change this template if you change
# Security.__reduce__, or else we'll attempt to unpickle an old version of this
# class
CACHE_FILE_TEMPLATE = '/tmp/.%s-%s.v3.cache'

This comment has been minimized.

@ehebert

ehebert Mar 4, 2015

Member

This might be able to be removed?

It's an artifact of how we do caching, but might want to remove it until/unless there is an eventual solution in Zipline.

This comment has been minimized.

@jfkirk

jfkirk Mar 4, 2015

Contributor

Remove the numpy imports, specifically?

This comment has been minimized.

@ehebert

ehebert Mar 4, 2015

Member

Oh, sorry I meant the CACHE_FILE_TEMPLATE.

def __hash__(self):
return self.sid_hash
property security_start_date:

This comment has been minimized.

@ehebert

ehebert Mar 4, 2015

Member

This could be removed, I don't think it is used anywhere internally.

This comment has been minimized.

@jfkirk

jfkirk Mar 4, 2015

Contributor

Remove security_start_date?

This comment has been minimized.

@ehebert

ehebert Mar 4, 2015

Member

Yes, as well as security_end_date.

@ehebert

This comment has been minimized.

Member

ehebert commented Mar 4, 2015

I think this is all that is needed to get the TravisCI build unstuck, which can be done either as part of this PR, or right after.

diff --git a/.travis.yml b/.travis.yml
index bcec803..3910918 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -41,6 +41,7 @@ install:
   - grep nose-ignore-docstring== etc/requirements_dev.txt | xargs pip install
   - pip install coveralls
   - pip install nose-timer
+  - python setup.py build_ext --inplace
 before_script:
   - "flake8 zipline tests"
 script:
return Security(**dict_)
def make_security_array(int size, Security security):

This comment has been minimized.

@ehebert

ehebert Mar 4, 2015

Member

This might be able to be trimmed as well.

This comment has been minimized.

@ssanderson

ssanderson Mar 4, 2015

Member

This is used internally in Quantopian. We could move it out of zipline though.

def __get__(self):
return self.start_date
property security_end_date:

This comment has been minimized.

@ehebert

ehebert Mar 4, 2015

Member

Ibid.

self.sid = sid
self.sid_hash = hash(sid)
self.symbol = symbol

This comment has been minimized.

@ehebert

ehebert Mar 4, 2015

Member

Something that I had not considered until reading through is that some of these extra fields are extraneous in a pure Zipline context. We could remove the 'extra' fields and have an internal Security class that inherits from this one, with the internal class adding the display related fields.

(That internal class could also handle the custom caching etc.)

However, that is more of an open question, and wouldn't say we would need to block merge on it.

This comment has been minimized.

@jfkirk

jfkirk Mar 4, 2015

Contributor

As we move forward on implementing Futures, these fields may come in to use. Would you be alright with leaving them in with a mark to remove if not needed later?

This comment has been minimized.

@ehebert

ehebert Mar 4, 2015

Member

Yes, I think it's ok to leave them.

@ehebert

This comment has been minimized.

Member

ehebert commented Mar 4, 2015

Looks good.

My recommendations:

  • Squash all the commits into one.
  • Consider removing more of the internal fields/functions, i.e. anything that isn't directly related to Zipline functionality. (i.e. could the Security class just have a sid and hash_sid member without any of the other members like symbol, first_traded etc.) But I could be sold on either way.
@ssanderson

This comment has been minimized.

Member

ssanderson commented Mar 4, 2015

(i.e. could the Security class just have a sid and hash_sid member without any of the other members like symbol, first_traded etc.)

What would the point be of having the class at all at this point? If you have a struct that just has an int, and a copy of that int, shouldn't you just use an int?

@jfkirk

This comment has been minimized.

Contributor

jfkirk commented Mar 4, 2015

Merged via 0375b35

@jfkirk jfkirk closed this Mar 4, 2015

@jfkirk jfkirk deleted the add-security-class branch Mar 4, 2015

@twiecki

This comment has been minimized.

Contributor

twiecki commented Mar 5, 2015

Are we forcing SID to be int with this?

@jfkirk

This comment has been minimized.

Contributor

jfkirk commented Mar 5, 2015

Not yet. This addition makes no functional change outside making this class
available. We want to preserve the current functionality which allows data
to be loaded with a symbol/string so, when that data is loaded, an int
would be assigned for the string.

On Thu, Mar 5, 2015 at 4:54 AM, Thomas Wiecki notifications@github.com
wrote:

Are we forcing SID to be int with this?


Reply to this email directly or view it on GitHub
#520 (comment).

@twiecki

This comment has been minimized.

Contributor

twiecki commented Mar 6, 2015

I don't think this should have gotten merged. It broke travis, makes installation harder for users, and does not add any functionality.

@ehebert

This comment has been minimized.

Member

ehebert commented Mar 6, 2015

@twiecki I see your point, and post-merge did second guess some of my recommendations on this merge, but I think it was worth it.

Merging it exposed some of the wrinkles that we would have to deal with sooner or later, and this is a key piece of incorporating other data sources.

Though we should probably chat about Cython-izing in general, it is in the plans for improving some bottlenecks, but I've neglected to check in with you about that.

Also, I admit may have been lax with considering all different combinations of Travis. I may need to transition my mental model away from it being a nice to have.

@dalejung

This comment has been minimized.

Contributor

dalejung commented Mar 7, 2015

I'm ambivalent to the Security stuff, but how was this merged with travis failures?

@ehebert

This comment has been minimized.

Member

ehebert commented Mar 8, 2015

I signed off on the merge because I didn't notice that the Python 3 one was
gray. Mea culpa.

On Sat, Mar 7, 2015 at 6:36 PM, Dale Jung notifications@github.com wrote:

I'm ambivalent to the Security stuff, but how was this merged with travis
failures?


Reply to this email directly or view it on GitHub
#520 (comment).

@jikamens

This comment has been minimized.

Contributor

jikamens commented Mar 8, 2015

The python 3 break is fixed on my PyPI update branch? Should we merge it?

Sent from my Android device

-----Original Message-----
From: Eddie Hebert notifications@github.com
To: quantopian/zipline zipline@noreply.github.com
Sent: Sun, 08 Mar 2015 8:53 AM
Subject: Re: [zipline] Add security class (#520)

I signed off on the merge because I didn't notice that the Python 3 one was
gray. Mea culpa.

On Sat, Mar 7, 2015 at 6:36 PM, Dale Jung notifications@github.com wrote:

I'm ambivalent to the Security stuff, but how was this merged with travis
failures?


Reply to this email directly or view it on GitHub
#520 (comment).


Reply to this email directly or view it on GitHub:
#520 (comment)

@twiecki

This comment has been minimized.

Contributor

twiecki commented Mar 8, 2015

I think we should definitely merge the python 3 fix but the pandas upgrade will make zipline non-shippable.

@jikamens

This comment has been minimized.

Contributor

jikamens commented Mar 8, 2015

OK, I cherry-picked the Python3 fixes for the Security class. While I was at it I cherry-picked the fix for not repeatedly downloading the benchmark and treasury curves unnecessarily.

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