-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add Order.tag for tracking orders and trades #200
Conversation
@kernc this PR is finally passing all checks now that I configured my PEP8 linter as you have it. I have made all changes that you referenced in #199 except the sample usage and unit tests. Sorry, I've not had time to do this yet. The question of what to do with orders that close trades - this refers to the FIFO rule in non-hedging mode, yes? The changes I've made do seem to work with my own trade simulation code, but I'm still working out a few bugs and would want to work those out before recommending any merge of this to master. |
Looks good to me! The |
Indeed, it's a way to carry over arbitrary info from orders to trades. Thanks for the reminder about tests. Testing at least that backtesting.py/backtesting/test/_test.py Lines 444 to 476 in 73e1534
yield self.buy(tag='foo')
assert self.trades[-1].tag == 'foo' or anything better you can think of. @qacollective Have you managed to test it all? Does it work? 😃 |
Yes, I have indeed tested this change and it works for what I intended it to - you can now track particular trades through whatever tag you assign to it - totally solved the problem I was trying to fix! |
@qacollective Care to add a simple unit test that confirms the working of the implementation? 😃 |
Wish I read the PR change before I implemented something similar. Nice 👍 |
Why is this still open? Will it be merged? I will now have to manually search my trades and the only distinguishing feature they have is a unique stop loss. Trades are all executed at same time and price (but even using that would be a cludge). Please merge or inform me of how one might reference trades from orders. |
7fd493d
to
e7981c7
Compare
@kernc |
The PR is still missing:
I guess I should get round to it. 😅 |
60eff81
to
109c352
Compare
It took a while, but now it's in. Thanks @qacollective for doing most of the legwork! |
Hello, def __repr__(self):
return '<Order {}>'.format(', '.join(f'{param}={round(value, 5)}'
for param, value in (
('size', self.__size),
('limit', self.__limit_price),
('stop', self.__stop_price),
('sl', self.__sl_price),
('tp', self.__tp_price),
('contingent', self.is_contingent),
('tag', self.__tag),
) if value is not None)) since the tag is processed through the round function it will throw errors when providing a string tag. |
Fixes #197
Fixes #53
Hello again. Many apologies for my butchering of the PR process. Git is certainly something I am no expert at. 😢
This is another PR which implements the tagging feature, makes tags an object type and fixes previous pep8 issues.
I am currently testing this for the first time on my own trading platform. The code seems to work fine to persist tags at the moment, but there are far too many trades being taken out, so unless that's a likely outcome of these changes, that problem is likely with my code outside of backtesting.py.