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

Add PRef operator (#988) #1000

Merged
merged 5 commits into from
Mar 24, 2022
Merged

Conversation

Chaoyingz
Copy link
Contributor

Description

Added PRef operator to support querying pit data for a specified reporting period.

Motivation and Context

See #988 .

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.

Screenshots of Test Results (if appropriate):

  1. Pipeline test:
  2. Your own tests:
    image

Types of changes

  • Fix bugs
  • Add new feature
  • Update documentation

@you-n-g
Copy link
Collaborator

you-n-g commented Mar 22, 2022

Hi, @Chaoyingz
This implementation looks really clean and smart!
But I think I misled you by giving the example

I come up with another idea to make it implemented with less code (PRef may not be necessary. We just need to enhance the PFeature). Besides, the interface will more consistent (Using P to convert period time to observation time )
Chaoyingz#2

Looking forward to your comments about it

@Chaoyingz
Copy link
Contributor Author

Chaoyingz commented Mar 23, 2022

Hi, @you-n-g
I think PFeature should be a feature rather than an expression, no additional parameters should be added to the feature.
How about extending an existing P expression directly? For Example:
https://github.com/Chaoyingz/qlib/blob/fabdebd03d86a46b8cc62d3560fc599d034d48ea/scripts/data_collector/pit/test_pit.py#L211-L231

@you-n-g
Copy link
Collaborator

you-n-g commented Mar 23, 2022

@Chaoyingz
I think both implementations are good enough but not perfect.

  • The previous version will make PFeature more complex
  • The current version will make load function more complex.

We are trying to introduce minimal complexity (fewer methods, fewer arguments in the interface) into the framework when adding new features.
Instead, if the complexity is added to a concrete subclass, it would keep the framework clean.

And I come up with another option just now. How about adding a subclass for PFeature which support an extra period argument? (e.g. changing PF to such a subclass from an alias)

I think all the above solutions are good enough. You can choose the one you like :)

@Chaoyingz Chaoyingz force-pushed the support-pref-operator branch from fabdebd to 7bc8426 Compare March 23, 2022 07:46
@Chaoyingz
Copy link
Contributor Author

Chaoyingz commented Mar 23, 2022

@you-n-g
I think PFeature is a feature (corresponding to the file name), and P is an operator. If you want to build a feature dynamically, it is better to use an operator. So PRef is the better choice.

I think the next step should be to adjust PFeature to make it completely independent from P, let PFeature return the data first, and then the P operator or PRef operator to process the returned data, instead of giving him parameters when getting PFeature data.

@you-n-g
Copy link
Collaborator

you-n-g commented Mar 23, 2022

Hi, @Chaoyingz
I agree with you
Please add more docs for load https://github.com/microsoft/qlib/blob/main/qlib/data/base.py#L157 to describe the period argument.

I think your next step looks very reasonable.
By such design, maybe the performance of PIT be boosted a lot.

@Chaoyingz
Copy link
Contributor Author

@you-n-g 👌

@you-n-g
Copy link
Collaborator

you-n-g commented Mar 24, 2022

It looks great!
Besides, 1000 is really a nice number :P

@you-n-g you-n-g merged commit 9dd5e07 into microsoft:main Mar 24, 2022
@Chaoyingz Chaoyingz deleted the support-pref-operator branch March 24, 2022 07:46
@you-n-g you-n-g added the enhancement New feature or request label Apr 24, 2022
@Chaoyingz Chaoyingz mentioned this pull request Aug 10, 2022
5 tasks
qianyun210603 pushed a commit to qianyun210603/qlib that referenced this pull request Mar 23, 2023
* Add PRef operator (microsoft#988)

* Fix type annotations

* Add test_pref_operator test case field

* Add note to PITProvider

* Add period parameter comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants