-
Notifications
You must be signed in to change notification settings - Fork 199
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
Compute Sortino ratio (#67) #108
Conversation
dd62507
to
694d67c
Compare
Addition of Sortino ratio to FinQuant
694d67c
to
bcbd442
Compare
Amazing, I didn't realize that so much work was already done to add the Sortino Ratio to FinQuant. Thank you so much for your work @aft90 and @fmilthaler! I will happily go into the details and complete the feature implementation as soon as possible. Sorry to bother you @fmilthaler, but it looks like some checks (build 3.11) were not successful. I tried to resolve some conflicts which appeared after pulling and pushing an empty commit as a test, and I hope I haven't done any damage : ). Do you have any idea how to solve these? All the best |
@fmilthaler Ok, it is all fixed now. |
Sorry, I couldn't check it out yesterday, but seems like you got it solved now. Well done :) |
@PietropaoloFrisoni Apologies for not telling you about this. Anyways, make your changes in |
Thank you, Frank. I was actually going to ask you what was going on XD. Before next week I'll most probably complete this PR (I would like to perform some sanity checks on the Sortino Ratio with other sources first). All the best |
Ultimately, I implemented the Sortino ratio described in this document. Different documents and tutorials seem to implement slightly different versions of the Sortino ratio. Performing the same test with the portfolio described in this example using @fmilthaler Please let me know if you like the current order in which the portfolio parameters are displayed or if there is anything you want to add/remove/change : ) All the best |
Excellent, I'll take a more detailled look at it tomorrow. |
Yep, I keep forgetting about updating the examples. I'll add it tomorrow 👍 |
Other than the docstring comment (which you actually did correctly, strictly speaking), this looks good to me. Will approve and merge as soon as the comment is re-added. |
One additional request: Could you add this line to
|
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.
As mentioned in the comments with more details:
- please revert the change of removing part of the docstring, and
- update the list of contributors
This partially reverts commit 7bd24f2.
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.
Since I was part of this PR, I cannot formally approve, but unofficial I do. Ready to be merged.
Addition of Sortino ratio to FinQuant