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

Conversation

Projects
None yet
2 participants
@llllllllll
Member

llllllllll commented Nov 7, 2014

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.

@llllllllll llllllllll force-pushed the actually-simple-transforms branch from 2b55323 to fc90d7d Nov 7, 2014

@@ -1,5 +1,5 @@
#

This comment has been minimized.

@llllllllll

llllllllll Nov 8, 2014

Member

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

@twiecki

This comment has been minimized.

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')

This comment has been minimized.

@twiecki

twiecki Nov 11, 2014

Contributor

So returns are always daily?

This comment has been minimized.

@llllllllll

llllllllll Nov 11, 2014

Member

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.

This comment has been minimized.

@twiecki

twiecki Nov 11, 2014

Contributor

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

def handle_data(algo, data):
algo.days += 1
if algo.days < algo.window_length:
return

This comment has been minimized.

@twiecki

twiecki Nov 11, 2014

Contributor

So this only works because of the add_transform call above, right?

This comment has been minimized.

@llllllllll

llllllllll Nov 11, 2014

Member

If you don't use add_transform, the warmup period will begin on the first call to the transform.

range(abs(n)),
date,
)
if n == 1:

This comment has been minimized.

@twiecki

twiecki Nov 11, 2014

Contributor

While you're at it, can you add a doc string here? Not sure what it does / when it's used jsut from looking at it.

# if we are producing daily timestamps, we
# use midnight
cur = cur.replace(hour=0, minute=0, second=0,
microsecond=0)

check = env.is_trading_day if daily_delta else env.is_market_hours

This comment has been minimized.

@twiecki

twiecki Nov 11, 2014

Contributor

not liking the variable name here. Maybe market_open?

cur_midnight = cur.replace(hour=0, minute=0, second=0,
microsecond=0)
cur = cur.replace(day=cur_midnight.day)
if not check(cur):

This comment has been minimized.

@twiecki

twiecki Nov 11, 2014

Contributor

Maybe this logic can be combined with the logic in check into a small local function with a descriptive name that you call here instead.

This comment has been minimized.

@llllllllll

llllllllll Nov 11, 2014

Member

I have a small function that does this now.



class TestRegisterTransformAlgorithm(TradingAlgorithm):
def initialize(self, *args, **kwargs):
self.add_transform(MovingAverage, 'mavg', ['price'],

This comment has been minimized.

@twiecki

twiecki Nov 11, 2014

Contributor

Ugh, just got reminded how ugly this was before.

@llllllllll llllllllll force-pushed the actually-simple-transforms branch from 5bcee87 to ade8d56 Nov 11, 2014


def __init__(self, initial_values=None):
if initial_values:

This comment has been minimized.

@twiecki

twiecki Nov 11, 2014

Contributor

Always better to check explicitly initial_values is not None

This comment has been minimized.

@llllllllll

llllllllll Nov 12, 2014

Member

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


now = algo.datetime
if now != cls._history_cache_dt:
cls._history_cache_dt = now

This comment has been minimized.

@twiecki

twiecki Nov 11, 2014

Contributor

So this essentially a global variable that lives in the class?

self._get_bars = minute_get_bars

# Not actually recursive.
return self._get_bars(days)

This comment has been minimized.

@twiecki

twiecki Nov 11, 2014

Contributor

OK, I guess you have to do that here because it relies on information we don't have available during __init__? It's very clever, just wondering if not a bit too clever.

@twiecki

This comment has been minimized.

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 Nov 12, 2014

llllllllll added some commits Nov 11, 2014

ENH: Replaces the simple transforms with history calls. Switches
transforms to quantopian syntax.

Adds the sid attribute to the siddata so it is aware of which security
it represents.
MAINT: Updates the add_trading_days to use the index of the date for a
more efficient means of jumping larger gaps of dates.

Adds a docstring to explain the usage of the function.

@llllllllll llllllllll force-pushed the actually-simple-transforms branch from 27f8378 to 39eb80e Nov 17, 2014

@llllllllll llllllllll merged commit 39eb80e into master Nov 17, 2014

1 check passed

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

@llllllllll llllllllll deleted the actually-simple-transforms branch Nov 17, 2014

@twiecki

This comment has been minimized.

Contributor

twiecki commented Nov 17, 2014

👍

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