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
ML based Pairs Selection #5
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added comments to the code part. Quite a lot of code you've added here 🙂
Now will go over the unit tests and the sphinx docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great documentation! 👍
Will cover a few ways how it can be improvd over the call.
…he Data Importer Class
…and-thames/arbitragelab into ml_based_pairs_selection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great implementation! 👍
Left a few comments on the code. Will check the documentation now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good docs 🙂
Please split lines in sphinx docs so they are not more than 120-160 characters.
I might add more comments once I go over the Research notebook and use cases.
…and-thames/arbitragelab into ml_based_pairs_selection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thank you for fixing previous comments 👍
After a few minor adjustments will approve this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adjusting the code! Approving this PR. 🙂
Ready for review @Jackal08.
Just thought that we might want to add a quick note to the docs that the pair selection step might take some time to calculate. |
…and-thames/arbitragelab into ml_based_pairs_selection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments that need correction and then I will approve. Excellent work with this. Very happy.
Also:
You get the following 2 deprecation warnings:
The pprint_val function was deprecated in Matplotlib 3.1 and will be removed in 3.3.
return formatter.pprint_val(x)
The line2d_seg_dist function was deprecated in Matplotlib 3.1 and will be removed in 3.3.
i, (p0, p1) in enumerate(edges)]
Can we please fix this?
Approved. @PanPip, please merge and delete branch. |
Description
Implementation in this module are based on the book by Simão Moraes Sarmento and Nuno Horta "A Machine Learning based Pairs Trading Investment Strategy".
Type of Change
How Has This Been Tested?
This unit test file was created to test the added functions:
Test Configuration:
Checklist: