Resolves #22 , Fixed line 914 from 'plimit' to 'plow'

[neilsmurphy]

Issue #22

Existing code was allowing for slippage to execute trade outside of price range. Code should read 'plow'. All other examples of self.slipdown() in the module use 'plow'.

p = self._slip_down(plow, popen, doslip=self.p.slip_open, lim=True)

Using the sample script as input by @ab-trader

# Bug in bbroker._try_exec_limit()
# https://community.backtrader.com/topic/2542/bug-in-bbroker-_try_exec_limit

from datetime import datetime
import backtrader as bt
import backtrader_addons as bta

class LimitOrderTest(bt.SignalStrategy):

    def log(self, txt, dt=None):
        dt = dt or self.datas[0].datetime.date(0)
        print('%s, %s' % (dt.isoformat(), txt))

    def notify_order(self, order):
        if order.status in [order.Submitted, order.Accepted]:
            return

        if order.status in [order.Completed]:
            if order.isbuy():
                self.log(
                    'BUY EXECUTED, Price: %.2f, Cost: %.2f, Ref %d' %
                    (order.executed.price,
                     order.executed.value,
                     order.executed.comm))
            else:
                self.log('SELL EXECUTED, Price: %.2f, Cost: %.2f, Ref %d' %
                         (order.executed.price,
                          order.executed.value,
                          order.executed.comm))

        elif order.status in [order.Canceled, order.Margin, order.Rejected]:
            self.log('Order Canceled/Margin/Rejected')

        self.order = None

    def __init__(self):
        self.prices = [1285.0, 1299.5, 1305.0, 1306.0]
        self.counter = 0

    def next(self):
        self.log('close %0.2f' % self.data.close[0])
        if self.counter == 0:
           self.order = self.sell(exectype=bt.Order.Limit,
                                  price=self.prices[self.counter])
           self.log('SELL ISSUED @ %0.2f' % self.prices[self.counter])
        self.counter += 1

cerebro = bt.Cerebro()
cerebro.addstrategy(LimitOrderTest)
cerebro.broker.set_slippage_fixed(4.0)
cerebro.broker.setcash(10000.0)

data0 = bta.datafeeds.PremiumDataFutCSV(dataname='test_data_1.csv', plot=True,
                                        name='Test Data')
cerebro.adddata(data0)

cerebro.run()
cerebro.plot(style='candle')

Data date, open, high, low, close, volume, openinterest

20190101,1295.5,1305.5,1290.1,1298.6,235334,319231
20190102,1297.5,1303.5,1293.1,1296.6,235334,319231
20190103,1301.0,1309.4,1298.9,1307.3,244542,311254
20190104,1309.0,1312.9,1285.0,1298.3,316063,302111

Previous output was:

2019-01-02, close 1296.60
2019-01-02, SELL ISSUED @ 1285.00
2019-01-03, SELL EXECUTED, Price: 1297.00, Cost: -1297.00, Ref 0
2019-01-03, close 1307.30

Code correction results of testing in correct outputs.

/home/runout/projects/scratch/venv/bin/python /home/runout/projects/scratch/slippage_issue.py
2019-01-02, close 1296.60
2019-01-02, SELL ISSUED @ 1285.00
2019-01-03, SELL EXECUTED, Price: 1298.90, Cost: -1298.90, Ref 0
2019-01-03, close 1307.30
2019-01-04, close 1298.30

Process finished with exit code 0

[neilsmurphy]

Could you please add an appropriate test for that. This is a core simulating broker of Backtrader - it is very sensitive IMHO and need to be properly tested.

Agreed.

[neilsmurphy]

File submitted using pmin instead of plimit.

Tests created and added to backtrader test directory.

[vladisld]

Although Daniel already merged this fix to his repo - it would be gr8 to publish this PR to his repo nevertheless - just for the sake of adding your test ( which is very good btw :-) ).

[neilsmurphy]

Although Daniel already merged this fix to his repo - it would be gr8 to publish this PR to his repo nevertheless - just for the sake of adding your test ( which is very good btw :-) ).

Thanks! Is there an easy way to publish from here? Or should I make a new pull request using a fresh clone of mementum/backtrader?

[neilsmurphy]

BTW this fix is not in Daniel's repo. Current code still using plimit instead of plow.

if plimit <= popen:
        # open greater/equal than requested - sell more expensive
        pmin = max(plow, plimit)
        p = self._slip_down(plimit, popen, doslip=self.p.slip_open,
                            lim=True)
        self._execute(order, ago=0, price=p)

[vladisld]

BTW this fix is not in Daniel's repo. Current code still using plimit instead of plow.

my mistake - sorry - I was under impresstion the similar fix was submitted.

[vladisld]

Is there an easy way to publish from here?

Unfortunately no - publishing the same branch to the memento/backtrader will bring all bunch of unrelated commits to the PR (all the diffs between backtrader2 and the original repo ) :-(

Or should I make a new pull request using a fresh clone of mementum/backtrader?

What I usually do is to create an additional branch at the latest merged revision in the backtrader2 repository and cherry-pick the changes from the PR branch. Then the PR could be created in Daniel repo from this additional branch. ( I know it is awkward - but this is what I figured out - would appreciate if you have a better idea

[neilsmurphy]

cherry-pick the changes from the PR branch I couldn't find a good solution for this and I don't know how you are cherry picking the pr branch.

I made a pr but it had all of the exisitng commits in it.

Would it make sense to rebase and then try to put in a pr that would have left the net outstanding items?

[vladisld]

I made a pr but it had all of the existing commits in it.

That what I was talking about and was afraid it will happen.

Would it make sense to rebase and then try to put in a pr that would have left the net outstanding items?

this will not solve the problem since there are still differences between the two repositories ( README files, CI config, probably others). So the new PR should be built ( I'm doing it using 'git cherry-pick' command, while creating a new branch in the backtrader2 repository and then publishing the PR from this new branch).

If you would like I can do this - np.

As for now I'm suggesting to close the current PR you've opened in Daniel's repository - since it can't be submitted the way it is - it contains too much unrelated changes.

[neilsmurphy]

If you would like I can do this - np.

That would be awesome, thanks. I'm not great with GIT.

[neilsmurphy]

I closed the pr but then tried to delete the branch but I think that was a mistake? I was trying to delete the pr. I'm going to stop and let you run with this. I'm new to git and I don't want to upset the applecart too much.

[vladisld]

I've opened the PR to our rep with the merge changes from Daniel's repository. Could you please approve - can't approve myself

[neilsmurphy]

I've opened the PR to our rep with the merge changes from Daniel's repository. Could you please approve - can't approve myself

Done. I'm going to try to do the cherry pick if you haven''t done it.

[vladisld]

I'm going to try to do the cherry pick if you haven''t done it

I was going to do this right now - but if you like to try - no problem - go on. Update me if you need any help if you like.

[neilsmurphy]

I'm going to try to do the cherry pick if you haven''t done it

I was going to do this right now - but if you like to try - no problem - go on. Update me if you need any help if you like.

Hey Vlad, I'm not sure how to do this and don't want to mess things up. Would you mind doing this one and just jotting down the steps for me?

[neilsmurphy]

@vladisld Can you go ahead with this? I don't want to practice learning with the main repo. Could you also jot down steps to complete if it's not too inconvenient?

ps: thanks for the referral!

[vladisld]

Sorry - there were a couple of very busy days at work - wasn't able to take care of this commit. Will try to complete it today-tomorrow.

[vladisld]

Done: https://github.com/mementum/backtrader/pull/415

Sorry for being so late - as I said it was a very busy week for me at work - some crisis, you know.

Steps I took to upload to memento/backtrader repo:

On local dev machine in backtrader2 repository:

1. Create the local branch 'slip_down_fix_upload' at the point of original divergence from the memento\backtrader repository (Release 1.9.75.123) - this is the point where both our repositories were equal so the diff from the current change and the memento/backtrader repository will only contain your particular change.

git branch slip_down_fix_upload git checkout slip_down_fix_upload

2. Cherry-pick (without actual local commit) all the changes from your fix one after another in history order from the earliest to the latest - in order to get a single commit after all. Possibly fixing all the conflicts if any.

git cherry-pick --no-commit commit-id-earliest git cherry-pick ... git cherry-pick --no-commit commit-id-latest

3. Locally commit all the changes ( to the local 'slip_down_fix_upload'), updating the commit message on the way

git commit -a

4. Push the commit to the backtrader2/backtrader repository

git push origin slip_down_fix_upload

5. Create the pull-request in memento/backtrader repository from backtrader2/backtrader repo and slip_down_fix_upload branch

Done.