Added support for shorting in the drift rebalancer; fixed a bug in limit price calculation. #611
Conversation
…ttempt to get status of broker orders during trading iteration
brettelliot
temporarily deployed
to
unit-tests
GitHub Actions
Inactive
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary by Korbit AI
Code Execution Comments
- Update
rebalanceto prevent unintended shorting and incorporate mechanisms for borrowing, tracking, and risk management in short selling.
Files scanned
| File Path | Reviewed |
|---|---|
| lumibot/example_strategies/classic_60_40.py | ✅ |
| lumibot/strategies/drift_rebalancer.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ✅ Issue Categories
Category Enabled Naming ✅ Database Operations ✅ Documentation ✅ Logging ✅ Error Handling ✅ Systems and Environment ✅ Objects and Data Structures ✅ Readability and Maintainability ✅ Asynchronous Processing ✅ Design Patterns ✅ Third-Party Libraries ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
| # Sell everything | ||
| symbol = row["symbol"] | ||
| quantity = row["current_quantity"] | ||
| last_price = Decimal(self.strategy.get_last_price(symbol)) | ||
| limit_price = self.calculate_limit_price(last_price=last_price, side="sell") | ||
| self.place_limit_order(symbol=symbol, quantity=quantity, limit_price=limit_price, side="sell") | ||
| elif row["absolute_drift"] < 0: | ||
| if quantity > 0 or (quantity == 0 and self.shorting): |
There was a problem hiding this comment.
Review condition for placing a sell order in the rebalance method.
Tell me more
In the rebalance method of LimitOrderRebalanceLogic, the condition for placing a sell order when row['drift'] == -1 allows shorting even when the current quantity is zero. This may lead to unintended shorting. Consider modifying the condition to only allow shorting when self.shorting is True and the current quantity is greater than zero, or when explicitly intending to open a new short position.
| self.fill_sleeptime = self.parameters.get("fill_sleeptime", 15) | ||
| self.target_weights = {k: Decimal(v) for k, v in self.parameters["target_weights"].items()} | ||
| self.shorting = self.parameters.get("shorting", False) |
There was a problem hiding this comment.
Shorting functionality lacks risk management and tracking mechanisms.
Tell me more
The code allows shorting if the 'shorting' parameter is set to True, but it does not properly handle the risks and complexities associated with short selling. Short selling involves borrowing shares and selling them, with the expectation of buying them back at a lower price later. However, the code does not include any mechanisms to borrow shares, track borrowed positions, or handle the potential for unlimited losses if the shorted asset's price increases. To properly implement shorting, additional logic should be added to handle borrowing shares, tracking borrowed positions, and managing the risks associated with short selling. This may involve integrating with a broker's API to borrow shares, implementing risk management measures such as stop-loss orders, and properly accounting for the borrowed shares in the portfolio calculations. Without these considerations, allowing shorting in the current implementation could lead to incorrect portfolio tracking and potential financial losses.
brettelliot
temporarily deployed
to
unit-tests
GitHub Actions
Inactive
…ike; remove logger in strategy
brettelliot
temporarily deployed
to
unit-tests
GitHub Actions
Inactive
Description by Korbit AI
What change is being made?
Add support for shorting in the drift rebalancer and fix a bug in limit price calculation.
Why are these changes being made?
Shorting capability has been implemented to extend the flexibility of the strategy, enabling both long and short positions to be taken based on market opportunities. The fix in limit price calculation resolves a bug in slippage handling to ensure more accurate trade execution, adhering to the target slippage percentage. Adjustments include replacing 'absolute_drift' with 'drift', modifications to parameter handling, and updates to the strategy initialization and execution logic.