[BUGFIX] allow sell in limit-up case and allow buy in limit-down case in topk strategy #1407
Conversation
This file contains 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
github-actions
bot
added
the
waiting for triage
Believe it's just some arbitrary number. The excess return is expected to change when trading logic changes.
you-n-g
reviewed
Jan 4, 2023
| @@ -216,7 +217,7 @@ def filter_stock(li): | |||
| buy = today[: len(sell) + self.topk - len(last)] | |||
| for code in current_stock_list: | |||
| if not self.trade_exchange.is_stock_tradable( | |||
| stock_id=code, start_time=trade_start_time, end_time=trade_end_time | |||
| stock_id=code, start_time=trade_start_time, end_time=trade_end_time, direction=OrderDir.SELL | |||
There was a problem hiding this comment.
Thanks for your contribution. @qianyun210603
Not selling limit-up stocks and buying limit-down could be considered as a part of the strategy in A-stock.
Although current implementation seems a little weird, but it is a reasonable results.
Your version is better. But if we merge it currently, all the previous results should be updated to make the code and results consistent (it will take a long time to update all the results).
Would it be better if we add it as a feature (a parameter in the init method) and keep the previous version as the default behavior?
(Besides, TODOs to switch to your version should be added in the docs/docstrings)
There was a problem hiding this comment.
Added a flag in TopkDropoutStrategy defaults to True to keep previous behaviour as suggested.
|
Thanks! It looks great now! |
you-n-g
added
enhancement
qianyun210603
added a commit
to qianyun210603/qlib
that referenced
this pull request
Mar 23, 2023
… in topk strategy (microsoft#1407) * 1) check limit_up/down should consider direction; 2) fix some typo, typehint etc * fix error * Update test_all_pipeline.py Believe it's just some arbitrary number. The excess return is expected to change when trading logic changes. * add flag forbid_all_trade_at_limit to keep previous behivour for backward compatibility
qianyun210603
added a commit
to qianyun210603/qlib
that referenced
this pull request
Mar 23, 2023
… in topk strategy (microsoft#1407) * 1) check limit_up/down should consider direction; 2) fix some typo, typehint etc * fix error * Update test_all_pipeline.py Believe it's just some arbitrary number. The excess return is expected to change when trading logic changes. * add flag forbid_all_trade_at_limit to keep previous behivour for backward compatibility
qianyun210603
added a commit
to qianyun210603/qlib
that referenced
this pull request
Mar 23, 2023
… in topk strategy (microsoft#1407) * 1) check limit_up/down should consider direction; 2) fix some typo, typehint etc * fix error * Update test_all_pipeline.py Believe it's just some arbitrary number. The excess return is expected to change when trading logic changes. * add flag forbid_all_trade_at_limit to keep previous behivour for backward compatibility
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
Allow sell in limit-up case and allow buy in limit-down case
Also fixed some typo/typehints.
Motivation and Context
To be consistent with real world, in which we can buy at limit-down (跌停) and sell at limit up (涨停).
How Has This Been Tested?
pytest qlib/tests/test_all_pipeline.pyunder upper directory ofqlib.Screenshots of Test Results (if appropriate):
Types of changes