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

simple transforms #429

Merged
merged 6 commits into from
Nov 17, 2014
Merged

simple transforms #429

merged 6 commits into from
Nov 17, 2014

Conversation

llllllllll
Copy link
Contributor

Removes the old transform classes and machinery to make simple transforms use history under the hood.
Changes the add_transform method to accept a string naming the simple transform and a window length (unless you pass returns).

simple transforms are now actual methods on the SIDData that do internal calls to history and apply the proper transforms.

@@ -1,5 +1,5 @@
#
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: The diff here is crazy; however, this file was re-written entirely.

@twiecki
Copy link
Contributor

twiecki commented Nov 9, 2014

"454 additions and 942 deletions. " <- that's great!

self.registered_transforms[tag] = {'class': transform_class,
'args': args,
'kwargs': kwargs}
self.add_history(2, '1d', 'price')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So returns are always daily?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because a history of '1d' in daily will have index -1 be the current minute's price, this gives us the price and yesterday's eod price.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I just can't get minute returns. I suppose that wasn't possible before either?


def __init__(self, initial_values=None):
if initial_values:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always better to check explicitly initial_values is not None

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that leaving the implicit truth value here is okay because if initial_values={} then this should still be avoided.

@twiecki
Copy link
Contributor

twiecki commented Nov 11, 2014

I like this a lot. My main concern is the complexity inside of https://github.com/quantopian/zipline/pull/429/files#diff-318b64beea94178a9d6b7fa10381f433R280. I suppose it's necessary for all that smart caching. Could the complexity be reduced by caching less?

@llllllllll llllllllll force-pushed the actually-simple-transforms branch 2 times, most recently from 5048d3d to 669c450 Compare November 12, 2014 22:35
@llllllllll llllllllll merged commit 39eb80e into master Nov 17, 2014
@llllllllll llllllllll deleted the actually-simple-transforms branch November 17, 2014 20:09
@twiecki
Copy link
Contributor

twiecki commented Nov 17, 2014

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants