-
Notifications
You must be signed in to change notification settings - Fork 262
Logging Module + Dynamic Buy/Sell Prop #363
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
|
This might be a large change so comments, requests for change are welcome! :) |
rafmacalaba
left a comment
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.
lgtm
|
has no idea what I'm doing lgtm!! 😅 I'll defer to your judgement |
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.
Hey @jbdelmundo , thanks so much for this PR. BaseStrategy looks 5 times cleaner, and the buy / sell proportions look very interesting 😄
Let's start with the initial comments that I just gave, most importantly regarding the new "execution types" being implemented, and the inconsistency between the types used for create_buy_order vs create_sell_order.
My recommendation is to include all three execution types for both:
close- next time step closeopen- next time step open (this is the preferredexecution_typethough requires anopencolumn)market- current time step close
Note: There's current handling that makes open the same as market in the case that open is chosen, but the column doesn't exist in the input dateframe.
| stop loss percent | ||
| stop_trail : float | ||
| stop trail percent | ||
| execution_type : str {`market`,`close`} (default=`market`) |
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.
@jbdelmundo Any specific reason why you decided to leave out the open option that was previously implemented? 😄
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.
Actually I am having trouble making the backtest use the next opening price.
I have tried using Market and Close orders in backtrader but it seems both use the closing price.
For Close orders, it uses the next day (or bar) close price, and for Market orders, it uses the current closing price
I am using the Ternary Strategy to manually provide buy and sell signals and review them on the order history
| ) | ||
|
|
||
| elif stock_value > 0: | ||
| # Buy on tomorrow's closing price |
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.
@jbdelmundo Yeah, I think best to still include the open type of execution. Technically, next day open is the most recommended approach to implementing the buy and sell; although, we use current / next day close as a proxy when next day open is not available.
| ) | ||
|
|
||
| # Buy based on the opening price of the next closing day (only works "open" data exists in the dataset) | ||
| else: |
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.
@jbdelmundo I notice that for create_buy_order the execution types are market, and close, while for create_short_sell_order, you stuck to the original close, and open execution types. Is there a reason for why they're not consistent?
Co-authored-by: Lorenzo Ampil <lorenzojulioampil@gmail.com>
|
Reopened here. |
Description
Added the following changes:
BaseStrategyprint()inside theBaseStrategyself.logging.log()buy_signal()andsell_signal()can now return(signal, buy/sell_prop)instead of just boolean (still backwards compatible)trade_historyFalse(no action)Checklist