-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Include Commissions in _trades DataFrame. #1276
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
Conversation
backtesting/_stats.py
Outdated
@@ -67,6 +67,7 @@ def compute_stats( | |||
'SL': [t.sl for t in trades], | |||
'TP': [t.tp for t in trades], | |||
'PnL': [t.pl for t in trades], | |||
'Commissions': [t._commissions for t in trades], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasonable! Can we put this in conditionally, only if commission=
was passed non-0 which is the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this would make sense for 'SL' and 'TP' too ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, PnL
doesn't account for Commissions
. Rationale behind this decision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're implying PnL should be reduced for the respective value of Commissions, I guess it might be a bug!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, PnL
should ideally account for Commissions
and Spread
. I can make the fix if you're aligned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely! Ideally, also document it if you can, maybe here. Particularly if there was to become a discrepancy between what trade.pl
and this data frame reported.
Also think there might be a few open bugs related to commissions computation. Maybe see if you can get some of those by the way? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will push these fixes :)
Incrementally taking up the changes. Please review this CR which includes commissions as the first step. |
Include
Commissions
in_trades
DataFrame.