-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat: add ppv function and update alias docstrings #756
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.
@arshiaar thanks very much for this pull request. It looks great.
I have made some incredibly minor feedback (that, for the most part, doesn't even matter terribly).
Also, just a heads up, that I sometimes do my code reviews in batches. So it is possible I will come back later and do another round of review.
Implements issue #681 |
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.
Some feedback on the updates to included.md
@arshiaar, you may already be across this (in which case apologies for this comment) - I believe test coverage will also need to be added. That goes beyond my skill set, so @tennlee will be best placed to provide any guidance on that. And, if you already have a plan for this, then apologies for this comment :-). |
@arshiaar thanks for those updates. Two more pieces of feedback (which I couldn't leave as code comments). (1) In the "Success Ratio" section of included.md, you need to add "Positive Predictive Value" in the brackets after "Precision". Here is how it is currently rendering: (2) In the "Precision" section of included.md, you need to add "Positive Predictive Value" in the brackets after "Success Ratio" Here is how it is currently rendering: Let either @tennlee or me know if my explanation isn't clear enough |
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.
These changes look good to me
Docstrings
Documentation