Skip to content
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

Shenlei151 flexibility enhance #491

Conversation

shenlei515
Copy link
Contributor

Fixes #481
I adjust a few lines of code in FinRL to make it more flexible for problems that I met with in my own project, though these changes may be aiming for my project, I think it might be useful to extend the feature of FinRL including:
feature extension:

  1. support different costs_pct with different stocks(fix it by modifying finrl/finrl_meta/env_stock_trading/env_stocktrading.py function init)
  2. able to hold stocks when initial instead of only cash(fix it by modifying finrl/finrl_meta/env_stock_trading/env_stocktrading.py function init)
  3. support to define time that trading is forbidden(fix it by modifying finrl/finrl_meta/env_stock_trading/env_stocktrading.py function _buy_stock and function _sell_stock)
  4. able to get state of everyday when trading(fix it by adding finrl/finrl_meta/env_stock_trading/env_stocktrading.py function save_state_memory and finrl/drl_agents/stablebaselines3/models.py function DRL_prediction)
    bug:
    5.fix bug that when buying stocks it didn't consider conmmision, which will cause the cash < 0 (fix it by modifying finrl/finrl_meta/env_stock_trading/env_stocktrading.py function _buy_stock)
    These features are needed in my project, I hope it will help enhance the flexibility of FinRL
    Also, I put part of my project code to explan why and how to use the features that I add in RL_stock.py, I hope it will help you understand the changes I made.

@zhumingpassional
Copy link
Collaborator

Thanks for your codes.

It's great. I suggest to add type hint in all functions. In the new version, type hint will be added.

@YangletLiu
Copy link
Contributor

@shenlei515 some research team contacted me about errors, they have a submission ddl, so I reverted this PR.

@shenlei515
Copy link
Contributor Author

@shenlei515 some research team contacted me about errors, they have a submission ddl, so I reverted this PR.

Yes, I see some errors caused by my changes, I think my change only apply to my situation, maybe it need some modifications to be adapted for other situations. I am sorry for the result caused by my unreasonable PR.

@YangletLiu
Copy link
Contributor

It is OK, not a big issue.
We may need to make the codes more standard so that everybody can contribute more easily.

@zhumingpassional
Copy link
Collaborator

zhumingpassional commented Mar 12, 2022

@shenlei515
Could you please describe the function of RL_stock.py? How to use it? Could you provide an example (code)?
Thanks you very much.

In addition, I think RL_stock.py is not suitable to be in the current folder.

@zhumingpassional
Copy link
Collaborator

zhumingpassional commented Mar 27, 2022

Thanks for your codes.

I have several questions. What's the length of initial_list? What does initial_list mean?

@shenlei515
Copy link
Contributor Author

@zhumingpassional
RL_stock is a example to explain how to use the changed RLlib and why I need to modify the RLlib

First element of initial_list is the cash we have in the beginning, rests of it are the share of stocks we have at first. So the length of initial_list is supposed to be 1+stock_dimension.

@zhumingpassional
Copy link
Collaborator

zhumingpassional commented Mar 28, 2022

@shenlei515

Thanks for your reply.

I suggest initial_list is seperated to two parts (parameters), initial_amount and num_stock_shares, since they have different units/denotations (money, and num of shares).

@shenlei515
Copy link
Contributor Author

@shenlei515

Thanks for your reply.

I suggest initial_list is seperated to two parts (parameters), initial_amount and num_stock_shares, since they have different units (money, and num of shares).

Yes, that make more sense.

@zhumingpassional zhumingpassional self-requested a review March 28, 2022 02:17
@zhumingpassional
Copy link
Collaborator

zhumingpassional commented Mar 28, 2022

@shenlei515 Could you please push a PR to revise it if you have enough time? The first PR focuses on this point (initial_list is seperated to two parts). A PR with small changes is more favorable.

@shenlei515
Copy link
Contributor Author

I have pulled a request to revise it.

@zhumingpassional
Copy link
Collaborator

Merged. Thanks for your valuable codes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some function may extent the flexibility of FinRL
3 participants