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

Closed positions #111

Merged
merged 6 commits into from
Jul 28, 2016
Merged

Conversation

nwillemse
Copy link

Ryan, shouldn't self.close_positions be a list of all the close Positions classes instead of a dict with tickers?

When the position is closed, it should get added to the list. If at the end the backtest the position is still open, it also needs to be appended at the end with status 'open'.

@ryankennedyio
Copy link
Contributor

ryankennedyio commented Jul 27, 2016

Yes sorry, still work-in-progress, I don't think it'll be ready to merge to merge for a few days. Dict are slower than list, and for the closed positions we never need to look up by index, so I think dict doesn't provide anything useful there. My branch is storing closed positions, indexed by the ticker. Has the side-effect of overwriting any closed position if we try to add the same closed position twice, so it really does need to be a list.

Good pickup RE moving open positions over at end of backtest. Would that be so Statistics can access all positions, including current ones?

@nwillemse
Copy link
Author

Yes I have used list to manage the position list as well. I also added a PositionId to the position class that starts at 1 and increments with each new position creates and also acts as a key.

Yes Statistics will need the list of positions at the end of the Backtest to create the trade statistics.

@ryankennedyio
Copy link
Contributor

ryankennedyio commented Jul 28, 2016

Thanks @nwillemse

I saw you wrote something in the Wiki, I've been meaning to write a CONTRIBUTING.md or something.

Basically as a rule of thumb, features should be branched from master on your own fork, set your fork up to test with Travis.ci and ensure test suite passes before doing a PR. Once we have actual Releases the complexity will step up a notch to something like git-flow, but at this stage master & forks should be all that's needed.

Do you have a branch set up on your fork? Feel free to @ me over there if you need any help with anything.

@coveralls
Copy link

coveralls commented Jul 28, 2016

Coverage Status

Coverage increased (+0.6%) to 89.523% when pulling 63d71c0 on ryankennedyio:closed-positions into 379a385 on mhallsmoore:master.

@coveralls
Copy link

coveralls commented Jul 28, 2016

Coverage Status

Coverage increased (+0.6%) to 89.523% when pulling 63d71c0 on ryankennedyio:closed-positions into 379a385 on mhallsmoore:master.

@ryankennedyio
Copy link
Contributor

ryankennedyio commented Jul 28, 2016

@mhallsmoore ready to go. Turned out to be a fairly simple solution. Tested some 10 year backtests on 2,000 odd daily bars and performance was rock solid.

Closes #91.

Allows #90 to move forward.

Primary difference is that we archive closed positions in portfolio.closed_positions list, and we also keep a member variable of portfolio to tally realised_pnl. We also modify portfolio.cur_cash whenever transact_position occurs. Cash never changes for live positions, so I've done it outside of update_portfolio().

"Live" value; that is value of currently open positions, is still calculated in update_portfolio

@nwillemse -- I felt that merging live & closed positions at the end of a backtest from within portfolio was going to add too much state to this part of the system (portfolio would need to know when backtest is finished). Instead of doing this, I strongly suggest your Statistics class will do this merging internally whenever it's own get_results() method is called. This ensures the logic is encapsulated in only the function that needs it.

@mhallsmoore
Copy link
Owner

Great work, everyone. Are we all happy that this is ready to go in?

@nwillemse
Copy link
Author

@ryankennedyio looks very good. I would like to have a position_id added to the Position class like the following: (Only the 3 lines with + is needed)

What do you think?


+import itertools

��class Position(object):
+    newid = itertools.count(start=1).next
    def __init__(
        self, action, ticker, init_quantity,
        init_price, init_commission,
        bid, ask
    ):
        """
        Set up the initial "account" of the Position to be
        zero for most items, with the exception of the initial
        purchase/sale.
        Then calculate the initial values and finally update the
        market value of the transaction.
        """
+        self.position_id = Position.newid()
        self.action = action
        self.ticker = ticker
...

@ryankennedyio
Copy link
Contributor

Hmmm. What's the benefit there @nwillemse ?

If closed positions are stored in list, it shouldn't matter if two positions shared the same ticker? We could just use their index within closed_positions as a kind of identifier.

Maybe in future if more than one strategy has an open position on the same ticker...

Either way, maybe make the change in your branch if it's necessary for your code, then we can review it with respect to your changes :). Just a bit easier to manage, as well as see how/why/alternatives if viewing in context to the rest of the changes an author has made.

Once this has been merged into master, it should be pretty easy for you to pull master, then merge master back into your branch.

@nwillemse
Copy link
Author

@ryankennedyio the benefit will be you can sort the list various ways, by exit_date, entry_date, symbol, and always go back to sort by position_id to get back to the original sequence of the positions taken. Also whenever you store these objects in a database, you always need id's.

@mhallsmoore lets merge this and I can test and update my branch.

@mhallsmoore
Copy link
Owner

I think it is quite unlikely that we'll ever need two simultaneously open positions on the same ticker. I don't see how it would work if we simultaneously longed and shorted an identical quantity of the same ticker in any system. We would be both "in" and "out" of the market (and down on transaction costs!).

In Interactive Brokers, for instance, I don't think it is possible to have two separate open positions on the same ticker in the same account. Correct me if I'm wrong though!

In fact, this is why I designed the Position class like this in the first place, so that it always tracked the net longed/shorted. I kept thinking about this sort of situation:

Time 1: LONG 100 -> NET 100 LONG
Time 2: LONG 50 -> NET 150 LONG
Time 3: SHORT 200 -> NET 50 SHORT
Time 4: LONG 50 -> NET 0 [This is "closed"]
Time 5: LONG 50 -> NET 50 [New trade or the same position "re-opened"?]

It gets a bit tricky...

@ryankennedyio
Copy link
Contributor

@mhallsmoore True, was assuming there were some good reasons RE Position design to begin with!

I'd be comfortable with the distinction that a position with exactly 0 quantity has been "fully closed". Anything on the same ticker in future would be "New".

Anyway, too tired to partake in much more thinking right now! Can get very tricky indeed. Glad we have a decent test suite here to fall back on as we change things, was very helpful in doing this feature.

@hpsilva
Copy link

hpsilva commented Jul 28, 2016

@mhallsmoore @ryankennedyio Trading simultaneously the same instrument in opposite directions particularly happen i.e. when mixing different strategies (trend following vs. mean reversion) and timeframes (i.e. daily vs. intraday).

A way i previously used (oanda) to avoid the mess of tracking long/short positions within the same account, was to include a strategy id/tag at the entries in the position class, and long/short positions are traded in different accounts. Might this be of use here?

I am curious if there is a wiser way of doing the same operation within a single account.

@mhallsmoore
Copy link
Owner

@ryankennedyio, @nwillemse, @hpsilva:

I'm happy to partition them across several accounts or via a strategy ID, as @hpsilva pointed out that it does occur. This makes a lot of sense.

However, I don't think it is ever going to be wise to allow two open positions with the same strategy/account and same ticker! :-)

We will need to handle this at the Portfolio/PositionSizer level as well, because if two strategies are giving opposing signals we should either a) let them occur because of @hpsilva's scenario or b) just net them out (i.e. a simultaneous long of 100 and short of 50 equals an order of long 50), if only for reducing transaction costs.

@mhallsmoore mhallsmoore merged commit 91a56b9 into mhallsmoore:master Jul 28, 2016
@mhallsmoore
Copy link
Owner

Merged for now, though!

@femtotrader
Copy link
Contributor

We might see hedging system (vs netting system) in MT5
https://www.mql5.com/en/articles/2299

Quantopian also have a concept named "round trips"
https://github.com/quantopian/pyfolio/blob/master/pyfolio/round_trips.py#L144

I prefer how MT5 is working

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.

None yet

6 participants