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.

Sign up for GitHub

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

Jump to bottom

Add Crypto dataset from coingecko #733

Merged
merged 12 commits into from
Dec 31, 2021
Merged

Add Crypto dataset from coingecko #733

merged 12 commits into from
Dec 31, 2021

Conversation

b4thesunrise
Copy link
Contributor

Description

Add collector in scripts to add crypto data

Motivation and Context

#716

solve the lack of crypto data, which is a need for many people(this need has been raised in gitter also)

How Has This Been Tested?

  • [] Pass the test by running: pytest qlib/tests/test_all_pipeline.py under upper directory of qlib.
  • If you are adding a new feature, test on your own test scripts.
    ran the scripts in readme successfully

Screenshots of Test Results (if appropriate):

  1. Pipeline test:
  2. Your own tests:

Types of changes

  • Fix bugs
  • Add new feature
  • Update documentation

@you-n-g you-n-g requested a review from zhupr December 11, 2021 04:04
zhupr
zhupr reviewed Dec 11, 2021
View reviewed changes
scripts/data_collector/crypto/README.md Outdated Show resolved Hide resolved
scripts/data_collector/crypto/collector.py Outdated Show resolved Hide resolved
@zhupr
Copy link
Collaborator

zhupr commented Dec 11, 2021

@b4thesunrise
Thanks for your contribution.
Crypto is region free and can be removed CryptoXXXCN

@b4thesunrise
Copy link
Contributor Author

@b4thesunrise Thanks for your contribution. Crypto is region free and can be removed CryptoXXXCN

thanks for your help! will remove it in later commit.

@b4thesunrise
Copy link
Contributor Author

b4thesunrise commented Dec 11, 2021 •
edited
Loading

commented

have already implement the change, please give it a review :)

zhupr
zhupr reviewed Dec 12, 2021
View reviewed changes
scripts/data_collector/crypto/collector.py Outdated Show resolved Hide resolved
scripts/data_collector/crypto/collector.py Outdated Show resolved Hide resolved
scripts/data_collector/crypto/collector.py Show resolved Hide resolved
scripts/data_collector/crypto/collector.py Outdated Show resolved Hide resolved
scripts/data_collector/crypto/collector.py Outdated Show resolved Hide resolved
scripts/data_collector/crypto/collector.py Outdated Show resolved Hide resolved
scripts/data_collector/crypto/collector.py Outdated Show resolved Hide resolved
scripts/data_collector/crypto/collector.py Outdated Show resolved Hide resolved
scripts/data_collector/crypto/README.md Outdated Show resolved Hide resolved
@zhupr
Copy link
Collaborator

zhupr commented Dec 12, 2021

commented

have already implement the change, please give it a review :)

Hi, currently CryptoCollector can only be used in Data retrieval, does not support Backtest, if users need to support backtest, they need to meet "Data-dependent component: Backtest", reference: https://github.com/microsoft/qlib/tree/main/scripts/data_collector#description-of-dataset

@you-n-g
Copy link
Collaborator

you-n-g commented Dec 12, 2021

@b4thesunrise
I think the following 2 choices could make it clearer for users.

  1. Describe the content and limitation (e.g. it don't support backtesting) of the dataset in the documentation.
  2. Implement a complete version of the dataset to make all the data dependant components runnable

You can choose either of them as you wish.
Thanks.

@b4thesunrise
Copy link
Contributor Author

commented

have already implement the change, please give it a review :)

Hi, currently CryptoCollector can only be used in Data retrieval, does not support Backtest, if users need to support backtest, they need to meet "Data-dependent component: Backtest", reference: https://github.com/microsoft/qlib/tree/main/scripts/data_collector#description-of-dataset

Hi, so to support backtest, the data need to support all the features of $close/$open/$low/$high/$volume/$change/$factor, don't know if I understand it correctly? If true, the currently data source do not support OHLC and I will choose to describe the limitation in the file.

@you-n-g
Copy link
Collaborator

you-n-g commented Dec 19, 2021

@b4thesunrise Yes. You understand it correctly.
Please add some docs to describe the limitation of the data source.

@b4thesunrise
Copy link
Contributor Author

@b4thesunrise Yes. You understand it correctly. Please add some docs to describe the limitation of the data source.

Got it, so busy for these two weeks, plan to update the docs and new code version at 12.30.

@b4thesunrise
Copy link
Contributor Author

@b4thesunrise Yes. You understand it correctly. Please add some docs to describe the limitation of the data source.
Have already clean the cn symbols and change the documentation, please give a review:)

@you-n-g
Copy link
Collaborator

you-n-g commented Dec 31, 2021

It looks great!
Thanks so much!

@you-n-g you-n-g merged commit 3e79a08 into microsoft:main Dec 31, 2021
@you-n-g you-n-g added the enhancement New feature or request label Jan 12, 2022
qianyun210603 pushed a commit to qianyun210603/qlib that referenced this pull request Mar 23, 2023
@b4thesunrise
Add Crypto dataset from coingecko (microsoft#733)
5692cc2 
* add crypto symbols collectors

* add crypto data collector

* add crypto symbols collectors

* add crypto data collector

* solver region and source problem

* fix merge

* fix merge

* clean all cn information

Co-authored-by: DefangCui <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhupr zhupr zhupr left review comments

Assignees

@Wangwuyi123 Wangwuyi123

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@b4thesunrise @zhupr @you-n-g @Wangwuyi123