New Sell order followed by Stop Loss - double `_close_position` bug

[Voyz]

It's a rare situation - you need both stop loss and new order to happen in the same time step - but when happens it is such a pain to nail down what's really wrong, since the framework won't break. Instead, all the data following will be slightly wrong. Started debugging this when trying to understand why ohlc_trades plot would draw two trades going on at the same time, if clearly existing positions need to be closed before opening new ones. That "double trade" plotting is result of that misaligned data due to the erroneous open position and instant stop loss.

Expected Behavior

Close the original position, ignore the stop loss and create a new order

Actual Behavior

Close the original position, create a new order, and instantly close it due to stop loss

Steps to Reproduce

1. Create a strategy that will produce a new sell order the same time step in which a stop loss threshold is reached.

Additional info

A couple of fix suggestions:

1. Update open, high, low once self._open_position(entry, is_long) is called (backtesting.py line 539)

backtesting.py line 532

if entry or orders._close:
    self._close_position()
    orders._close = False

################ THIS
# First make the entry order, if hit
if entry:
    if entry is _MARKET_PRICE or high > orders._entry > low:
        self._open_position(entry, is_long)
################ THIS

# Check if stop-loss threshold was hit
if sl and self._position:
    price = (sl if low <= sl <= high else              # hit
             open if (is_long and open < sl or         # gapped hit
                      not is_long and open > sl) else
             None)                                     # not hit
    if price is not None:
        self._close_position(price)
        self.orders.cancel()

# Check if take-profit threshold was hit
if tp and self._position:
    price = (tp if low < tp < high else
             open if (is_long and open > tp or
                      not is_long and open > sl) else
             None)
    if price is not None:
        self._close_position(price)
        self.orders.cancel()
   
###########################################
#### SHOULD PROBABLY BE MOVED HERE #######
###########################################

[kernc]

Apologies for the inconvenience. 😅 You are welcome to propose a more ready fix as part of a pull request.

I don't think moving the if entry: block down to the bottom is a valid solution. Buy and SL prices can be hit within the same candle, and if position is not already established, if sl and self._position: block won't do anything.

[kernc]

Can you provide a MWE strategy that exhibits this?

[Voyz]

Thanks for a fast reply. Could you expand on "Buy and SL prices can be hit within the same candle"? If a Buy happens on a candle, the SL on that same candle shouldn't matter, given that the Buy would already have closed the previous position, making SL redundant.

Or do you mean the SL to apply on that new Buy order? If that's the case then I'm not following - could you explain how could this ever happen and/or be what we'd want? Buy and Stop Loss on the same close price?

[kernc]

Or do you mean the SL to apply on that new Buy order?

Exactly. Case: At time t decide to enter on market price, set SL some pips below. The position is opened at next (t+1) candle's Open price. Since hypothetical t+1 Low is below the set SL threshold, we assume SL was hit some time after t+1 (but well before t+2) Open, and close the position.

[kernc]

The position is opened at next (t+1) candle's Open price.

The positions are always opened on following candles (market order opens at Open, other orders at specified price). Unless Backtest(..., trade_on_close=True) in which case the order is executed on Close. Is this where the bug is?

[Voyz]

A situation when we open a position at price X at t+1 (which must be Open > X > Close and High > X > Low, line538) and at the same time the t+1 Low happens to be below SL threshold - isn't this quite a rare case?

I personally can't imagine a trade where the SL is so tightly close to the entry price that it would close on the same timestep. By doing so, you'd essentially prohibit open positions with any movement against your prediction, a strategy which would dry out equity pretty fast due to commissions. If I'm missing something I'd gladly catch up.

Though even if it is the case, I can understand you'd like to cover for it. In which case, some bug probably exist in the _close_position or _open_position, since like I say - performing such close, open, sl close on same time step t+1 seems to cause that datapoint missing somewhere, which I believe is reflected later in that double trades plot. After debugging these two functions for a while I still can't figure out where the bug is I'm afraid. I hope you can spot it.

[Voyz]

Just another observation - if one really did want to have SL so tightly coupled with the entry price, wouldn't it be cheaper to do the math before opening the position, and preventing the trade in the first place? In other words - why open and close at the same timestep, when you could just look up the low price, and if it's below your SL prevent performing the trade.

In such scenario, it's the strategy's task to cover that case you're describing, not broker's. From my point of view, a broker which doesn't accept open and SL close on the same time step feels more robust. Although I have to admit, I don't think I fully understand the viability of that tightly coupled SL, so I may be missing some key point here - sorry if that's the case and I'm just mumbling.

[Voyz]

Okay, having reflected on that last observation - you wouldn't know low price when opening a trade, so doing the figuring out on strategy wouldn't work. Sorry about that

[kernc]

Yes, a candle can be of any timeframe, and since we can't possibly know the exact intracandle price movement, we assume pessimistically, in user's favor. 👍

bug in that double trades plot.

Share a short example code that reproduces this?

[kernc]

Example code to reproduce the issue:

from backtesting import Strategy, Backtest
from backtesting.lib import crossover
from backtesting.test import GOOG, SMA


class S(Strategy):
    def init(self):
        self.sma1 = self.I(SMA, self.data.Close, 10)
        self.sma2 = self.I(SMA, self.data.Close, 20)
        
    def next(self):
        if crossover(self.sma1, self.sma2):
            self.buy(sl=self.data.Low)
        if crossover(self.sma2, self.sma1):
            self.sell(sl=self.data.High)


bt = Backtest(GOOG.iloc[220:340], S)
bt.run()
bt.plot()

[Voyz]

As I can see this is too late as you refactored the library significantly in that new PR, though I did figure out what was wrong in the end.

When self.log.exit_price[i] = price executes in a situation that an existing position is closed, new position is opened and then stop-loss kicks in, the original position will have written to the exit_price[i] upon being closed, and then overwritten by the stop-loss position closing at the same i. This meant that exit_price was missing some elements and as such producing the double lines. I've temporarily fixed it with

        if not np.isnan(self.log.exit_price[i]):
            self.log.exit_price[i-1] = self.log.exit_price[i]

Though it seems that replacing it with storing orders' history as objects rather than list dependant on indices is a better idea - so thumbs up for that recent update.

[kernc]

Yeah, diving into this issue certainly convinced me the former architecture was ill posed.

The "2.0" PR is not merged yet, so if you feel there's something that can be done for the current "1.0" branch to mitigate such issues, I'd have a look at it.

Thanks for glancing over #47. I was waiting for someone to do a more exhaustive review, but maybe your 2¢ will have to do. 😄

[Voyz]

This is more of a patch than an actual solution, but arguably covers a good amount of cases:

        if not np.isnan(self.log.exit_price[i]):
            self.log.pl[i-1] = self.log.pl[i]
            self.log.pl_raw[i-1] = self.log.pl_raw[i]
            self.log.exit_entry[i-1] = self.log.exit_entry[i]
            self.log.exit_price[i-1] = self.log.exit_price[i]
            self.log.exit_position[i-1] = self.log.exit_position[i]
            self.log.trigger_exit[i-1] = self.log.trigger_exit[i]

Yeah, it will overwrite data in i-1 if it exists. But then probability of the following situation is relatively low:

• open trade on i
• close trade on i+1, open next trade on i+1
• stop_loss on i+1

Two trades one interval after another and then a stop loss on the same time? This is still an issue but this code adds an extra screwup requirement when compared to the situation that it fixes.

Btw. I've been re-writing your library on my own too, largely over the plotting, but also simplifying things. I felt like it was a great basis, but indeed needed some improvements here and there. This is how it looks like for me atm:

If you'd be willing to break the unmanageably large backtesting.py into submodules then maybe we can talk about collaboration?