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: Add account object to context #396

Merged
merged 0 commits into from Oct 10, 2014

Conversation

Projects
None yet
7 participants
@brianpfink
Contributor

brianpfink commented Sep 23, 2014

Add an account object to context. This account object keeps track of certain account metrics. A complete list is below. These metrics may be referenced and used in the same way one would use context.portfolio.

    account.settled_cash
    account.accrued_interest
    account.buying_power
    account.equity_with_loan
    account.total_positions_value
    account.regt_equity
    account.regt_margin
    account.initial_margin_requirement
    account.maintenance_margin_requirement
    account.available_funds
    account.excess_liquidity
    account.cushion
    account.day_trades_remaining
    account.leverage
    account.net_liquidation

@brianpfink brianpfink closed this Sep 23, 2014

@brianpfink brianpfink changed the title from add account object to context to ENH: Add account object to context Sep 23, 2014

@brianpfink brianpfink reopened this Sep 23, 2014

@brianpfink brianpfink force-pushed the context.account branch 2 times, most recently from 7cbb1cc to 8a79658 Oct 2, 2014

@brianpfink brianpfink assigned ehebert and unassigned jdricklefs Oct 2, 2014

@coveralls

This comment has been minimized.

coveralls commented Oct 2, 2014

Coverage Status

Coverage increased (+0.06%) when pulling 8a79658 on context.account into b3c7c26 on master.

@@ -233,6 +233,10 @@ def get_portfolio(self):
self.update_performance()
return self.cumulative_performance.as_portfolio()
def get_account(self):
self.update_performance()

This comment has been minimized.

@ehebert

ehebert Oct 2, 2014

Member

This will double the amount of times that update_performance is called, perhaps we need a method that is get_details (though not sure if that is the right name), which would be implemented as:

def get_details(self):
    self.update_performance()
    return (
        self.cumulative_performance.as_portfolio(),
        self.cumulative_performance.as_account()
    )
@ehebert

This comment has been minimized.

Member

ehebert commented Oct 2, 2014

I am a little worried about the overhead that this adds to algorithms that do not refer to account.
We should do a round of profiling on a noop algorithm to get a quantified number, but my gut says that it will be noticeable.

A way to mitigate it is maybe have a flag in PerformanceTracker of provides_account which is turned to True if an algorithm ever refers to context.account.
Then all the 'add to account object', 'serialize account object' type code would behind a if provides_account flag.

(We should probably do a similar flag for portfolio as well, but can do that later. The goal here would be to not have this be a degradation.)

@ehebert

This comment has been minimized.

Member

ehebert commented Oct 2, 2014

Also, should add some tests coverage.

An appropriate place (or at least the easiest) may be in test_algorithm.py in a similar style to test_empty_portfolio or test_events_through_risk.py where the portfolio.positions are asserted.

@brianpfink brianpfink force-pushed the context.account branch 3 times, most recently from c89a363 to fa1e082 Oct 6, 2014

@@ -251,6 +253,9 @@ def handle_commission(self, commission):
def adjust_cash(self, amount):
self.period_cash_flow += amount
def adjust_field(self, field, value):

This comment has been minimized.

@llllllllll

llllllllll Oct 6, 2014

Member

I don't think anything calls this

This comment has been minimized.

@brianpfink

brianpfink Oct 6, 2014

Contributor

qexec calls this to set the attribute and value with the information returned from the broker

This comment has been minimized.

@llllllllll

llllllllll Oct 6, 2014

Member

Okay; however, since this is just a wrapper around setattr, is there a reason to wrap setattr at all?

This comment has been minimized.

@brianpfink

brianpfink Oct 6, 2014

Contributor

nope, there is no reason for this. I will just use setattr

This comment has been minimized.

@brianpfink

brianpfink Oct 7, 2014

Contributor

Left as is after a discussion with Rich and Joe. Rich likes how this method abstracts away from live_algoproxy in the same way adjust_cash does.

@@ -229,10 +229,18 @@ def update_performance(self):
for perf_period in self.perf_periods:
perf_period.calculate_performance()
def get_portfolio(self):
self.update_performance()
self.performance_needs_update = False

This comment has been minimized.

@llllllllll

llllllllll Oct 6, 2014

Member

Is this used anywhere else?

This comment has been minimized.

@brianpfink

brianpfink Oct 6, 2014

Contributor

that was outdated code that I forgot to remove. Gone now

account = self._account_store
account.settled_cash = \
getattr(self, 'settled_cash', self.ending_cash)

This comment has been minimized.

@llllllllll

llllllllll Oct 6, 2014

Member

When are these on / not on self. could you put a comment at the top of the function explaining why these are all guarded with default values.

This comment has been minimized.

@brianpfink

brianpfink Oct 6, 2014

Contributor

done

@@ -336,6 +339,8 @@ def _create_generator(self, sim_params, source_filter=None):
self.perf_tracker = PerformanceTracker(sim_params)
self.portfolio_needs_update = True
self.account_needs_update = True

This comment has been minimized.

@llllllllll

llllllllll Oct 6, 2014

Member

Does the account change between this being set True here and it being set True in __init__. Same with the performance

@llllllllll

This comment has been minimized.

Member

llllllllll commented Oct 6, 2014

Looks like there are a few flake8 failures

@brianpfink brianpfink force-pushed the context.account branch from fa1e082 to 39df5f1 Oct 6, 2014

@llllllllll

This comment has been minimized.

Member

llllllllll commented Oct 6, 2014

/signoff

@@ -134,6 +134,32 @@ def __repr__(self):
return "Portfolio({0})".format(self.__dict__)
class Account(object):

This comment has been minimized.

@twiecki

twiecki Oct 7, 2014

Contributor

Maybe add a doc string about the purpose of this object.

return self._portfolio
@property
def account(self):
return self.updated_account()

This comment has been minimized.

@twiecki

twiecki Oct 7, 2014

Contributor

Why have the indirection here and not just move updated_account into the property?

This comment has been minimized.

@brianpfink

brianpfink Oct 7, 2014

Contributor

updated_account gets overwritten when connected to a broker. This allows the property to return the correct value in both cases without having to redefine the property.

This comment has been minimized.

@twiecki

twiecki Oct 7, 2014

Contributor

Got it.

@twiecki

This comment has been minimized.

Contributor

twiecki commented Oct 7, 2014

Only minor comments but looks good otherwise!

@twiecki

This comment has been minimized.

Contributor

twiecki commented Oct 7, 2014

Also, make sure to add this to the release announcements (separate DOC commit in this PR).

@brianpfink brianpfink force-pushed the context.account branch from 39df5f1 to 406dc11 Oct 7, 2014

@twiecki

This comment has been minimized.

Contributor

twiecki commented Oct 8, 2014

Looks good to merge! /sign-off

@brianpfink brianpfink merged commit 50c5b73 into master Oct 10, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

@brianpfink brianpfink force-pushed the context.account branch from 406dc11 to 50c5b73 Oct 10, 2014

@brianpfink brianpfink deleted the context.account branch Oct 10, 2014

@jbredeche

This comment has been minimized.

Member

jbredeche commented Oct 10, 2014

awesome
On Oct 10, 2014 5:16 PM, "brianpfink" notifications@github.com wrote:

Merged #396 #396.


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

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