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

feat: add ppv function and update alias docstrings #756

Closed
wants to merge 0 commits into from

Conversation

arshiaar
Copy link
Contributor

Docstrings

  • [x ] Docstrings complete and follow Napoleon (google) style
  • [x ] Reference to paper/webpage is in docstring. The preferred referencing style for journal articles is APA (7th edition)

Documentation

Copy link
Collaborator

@Steph-Chong Steph-Chong left a 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.

src/scores/categorical/contingency_impl.py Outdated Show resolved Hide resolved
src/scores/categorical/contingency_impl.py Outdated Show resolved Hide resolved
src/scores/categorical/contingency_impl.py Outdated Show resolved Hide resolved
src/scores/categorical/contingency_impl.py Outdated Show resolved Hide resolved
src/scores/categorical/contingency_impl.py Outdated Show resolved Hide resolved
src/scores/categorical/contingency_impl.py Outdated Show resolved Hide resolved
src/scores/categorical/contingency_impl.py Outdated Show resolved Hide resolved
src/scores/categorical/contingency_impl.py Outdated Show resolved Hide resolved
@arshiaar
Copy link
Contributor Author

arshiaar commented Nov 25, 2024

Implements issue #681

Copy link
Collaborator

@Steph-Chong Steph-Chong left a 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

docs/included.md Outdated Show resolved Hide resolved
@Steph-Chong
Copy link
Collaborator

@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 :-).

@Steph-Chong
Copy link
Collaborator

Steph-Chong commented Nov 25, 2024

@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:
image

(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:
image
[Updated with correct screenshot]

Let either @tennlee or me know if my explanation isn't clear enough

Copy link
Collaborator

@Steph-Chong Steph-Chong left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants