AI4Finance-Foundation / FinRL-Meta

FinRL­-Meta: Dynamic datasets and market environments for FinRL.
https://ai4finance.org
MIT License
1.26k stars 584 forks source link

Delete obsolete duplicates in FinRL vs FinRL-Meta #144

Open segatrade opened 2 years ago

segatrade commented 2 years ago

1) I cannot understand why there is FinRL and FinRL-Meta. Why you don't merge them? Duplicates:

https://github.com/AI4Finance-Foundation/FinRL/blob/master/tutorials/3-Practical/FinRL_MultiCrypto_Trading.ipynb

https://github.com/AI4Finance-Foundation/FinRL-Meta/blob/master/tutorials_notebook/3-Practical/FinRL_MultiCrypto_Trading.ipynb

https://github.com/AI4Finance-Foundation/FinRL-Meta/blob/master/tutorials_python/3-Practical/FinRL_MultiCrypto_Trading.py seems empty

Tutorial FinRL: Seems it doesn't use FinRL. Than why it's in FinRL repo, if it use only FinRL-Meta? DataProcessor in FinRL doesn't support Binance.

For what is better to use FinRL and for what FinRL-Meta? FinRL is obsolete (vs FinRL-Meta) and I can just ignore it and use FinRL-Meta only? Why do you support 2 similar repos with similar structure, but one is more updated than another?

By your info in README.md https://github.com/AI4Finance-Foundation/FinRL I think that I need FinRL, but seems FinRL-Meta is actual and better version of FinRL. FinRL obsolete (vs FinRL-Meta)

2) CryptoEnv there is in tutorial, but also it's available from finrl_meta.env_crypto_trading.env_multiple_crypto import CryptoEnv . Which one is more updated and better to use? https://github.com/AI4Finance-Foundation/FinRL/blob/master/finrl/finrl_meta/env_cryptocurrency_trading/env_multiple_crypto.py

https://github.com/AI4Finance-Foundation/FinRL-Meta/blob/master/finrl_meta/env_crypto_trading/env_multiple_crypto.py

Why doesn't delete it from tutorial and just import from actual repo?

YangletLiu commented 2 years ago

https://github.com/AI4Finance-Foundation/FinRL/blob/master/tutorials/FinRL_QES_Wolfe%20Research_Nov_08_2021.pdf

segatrade commented 2 years ago

Maybe delete duplicates, because it's make code dirty, obsolete and more difficult for beginners? https://github.com/AI4Finance-Foundation/FinRL/blob/master/tutorials/3-Practical/FinRL_MultiCrypto_Trading.ipynb

https://github.com/AI4Finance-Foundation/FinRL-Meta/blob/master/tutorials_notebook/3-Practical/FinRL_MultiCrypto_Trading.ipynb

https://github.com/AI4Finance-Foundation/FinRL/blob/master/finrl/finrl_meta/env_cryptocurrency_trading/env_multiple_crypto.py

https://github.com/AI4Finance-Foundation/FinRL-Meta/blob/master/finrl_meta/env_crypto_trading/env_multiple_crypto.py Actual version in FinRL-Meta as I understand

YangletLiu commented 2 years ago

Our Codes are actively updating, according to the roadmap. Later, they will be better.

segatrade commented 2 years ago

We just need to clean step by step. Delete all duplicates, because they with time start create confusion and chaos. We can always import them from one project to another I'm trying to run crypto tutorial via .py files without .ipynb. It's gives a lot of bugs Also this duplicate: https://github.com/AI4Finance-Foundation/FinRL/blob/master/finrl/agents/stablebaselines3/models.py https://github.com/AI4Finance-Foundation/FinRL-Meta/blob/master/agents/stablebaselines3_models.py

for FinRL: from finrl import config : /lib/python3.8/site-packages/finrl/config.py for FinRL-Meta: from finrl_meta import config : /lib/python3.8/site-packages/finrl_meta/finrl_meta/config.py seems we need to move agents and main.py etc inside finrl_meta to make it /lib/python3.8/site-packages/finrl_meta/config.py Probably this is connected https://github.com/AI4Finance-Foundation/FinRL-Meta/issues/142

YangletLiu commented 2 years ago

Actually, "Duplicates" are made on purpose, since I am pushing new principles of open-source projects: 1). Project matrix, e.g., FinRL, ElegantRL, FinRL-Meta, FinRL-Podracer, etc.; 2. Plug-and-play, e.g., keep moving stable/robust codes from one project to another one; 3. Each project follows the Onion model to organize its open-source community members.

segatrade commented 2 years ago

2 more duplicates in one repo: https://github.com/AI4Finance-Foundation/FinRL-Meta/blob/master/tutorials/2-Advance/MultiCrypto_Trading/MultiCrypto_Trading.ipynb

https://github.com/AI4Finance-Foundation/FinRL-Meta/blob/master/tutorials/3-Practical/FinRL_MultiCrypto_Trading/FinRL_MultiCrypto_Trading.ipynb

If do you think my feedback is useless please tell me, I will not write.

YangletLiu commented 2 years ago

Here, it is teamwork. So, any comment is welcome. It takes some time to reflect on the codes.

Initially, your comments included several words "FinRL obsolete", so I do not know how to respond.

segatrade commented 2 years ago

Thank you I mean FinRL obsolete vs FinRL-Meta. Added (vs FinRL-Meta) in old comments. In FinRL-Meta some files have new commits and updates vs FinRL

eyast commented 2 years ago

Hi all - I think that there is room for improvement, and that the area of respecting namespaces is one we should consider for the project. Personally, I suggest that Finrl-meta contains all the environment related code: no agents, no pytorch, and just market data

eyast commented 2 years ago

2 more duplicates in one repo: https://github.com/AI4Finance-Foundation/FinRL-Meta/blob/master/tutorials/2-Advance/MultiCrypto_Trading/MultiCrypto_Trading.ipynb

https://github.com/AI4Finance-Foundation/FinRL-Meta/blob/master/tutorials/3-Practical/FinRL_MultiCrypto_Trading/FinRL_MultiCrypto_Trading.ipynb

If do you think my feedback is useless please tell me, I will not write.

I dont think it's useless at all - on the contrary, thanks for your feedback. It's appreciated. We will revert back

zhumingpassional commented 2 years ago

@eyast I agree with you. remove duplicates.