fix(strategy): fix TopkDropoutStrategy to use actual sell order count#2223
Open
lingbai-kong wants to merge 1 commit into
Open
fix(strategy): fix TopkDropoutStrategy to use actual sell order count#2223lingbai-kong wants to merge 1 commit into
lingbai-kong wants to merge 1 commit into
Conversation
Fix issue microsoft#809 where TopkDropoutStrategy uses len(sell) (planned sell list) instead of len(sell_order_list) (actual executed sell orders) when calculating buy list size. This causes portfolio size to exceed topk when some sell orders fail due to tradability or hold_threshold checks. Changes: - Move buy list calculation after sell orders are generated - Use len(sell_order_list) instead of len(sell) - Add max(0, ...) to prevent negative buy list length
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes an issue in
TopkDropoutStrategywhere the number of stocks in the portfolio could exceed thetopksetting. The root cause is that the strategy was using the length of the planned sell list (len(sell)) instead of the actual number of successfully executed sell orders (len(sell_order_list)) when calculating how many stocks to buy.The Fix
Original code (qlib/qlib/contrib/strategy/signal_strategy.py):
Fixed code:
Key changes:
len(sell_order_list)(actual executed sells) instead oflen(sell)(planned sells)max(0, ...)to prevent negative index when appropriateMotivation and Context
Fixes issue #809 (#809).
The problem occurs when some sell orders fail due to:
is_stock_tradable()returns False)hold_thresholdconstraints (stock not held long enough)The original code assumed all planned sells would succeed, leading to buying more stocks than needed and exceeding the
topklimit.How Has This Been Tested?
Test Script
Test Results (ORIGINAL vs FIXED comparison)
Analysis
The comparison clearly demonstrates the bug and its fix:
In 3 out of 4 test scenarios, the original code causes the portfolio to exceed
topk. The fixed code correctly maintainstopk = 3in all scenarios.Screenshots of Test Results (if appropriate):
test_topk_fix_real.pyTypes of changes