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

Annotate redundant functions #41

Merged
merged 10 commits into from Jan 7, 2022

Conversation

cthoyt
Copy link
Contributor

@cthoyt cthoyt commented Jan 5, 2022

Summary

As a follow-up to #33, this PR adds a duplicate_of annotation to the rexmex.utils.Annotator class and begins annotating which functions are duplicate of each other.

Caveat I'm not really happy with the direction of which is the "duplicate" in many cases, e.g., where miss_rate is the "canonical" one and "false_negative_rate" is the duplicate

  • Code passes all tests
  • Unit tests provided for these changes
  • Documentation and docstrings added for these changes
  • Check all functions are annotated, and correctly

Changes

  • Add new annotation duplicate_of to rexmex.utils.Annotator
  • Annotate duplicate functions (e.g., precision_score is a duplicate of positive_predictive_value)
  • Pins pandas<=1.3.5 since they just put the 1.4 release candidate up on PyPI this morning and it doesn't work with the conda env in the GHA workflow

I'm not really happy with this since the "canonical" implementation of some use the more esoteric name
@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2022

Codecov Report

Merging #41 (3b105a5) into main (8fe5de1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #41   +/-   ##
=======================================
  Coverage   99.77%   99.77%           
=======================================
  Files          16       16           
  Lines         886      894    +8     
=======================================
+ Hits          884      892    +8     
  Misses          2        2           
Impacted Files Coverage Δ
rexmex/metrics/classification.py 100.00% <100.00%> (ø)
rexmex/utils.py 100.00% <100.00%> (ø)
tests/unit/test_metrics.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8fe5de1...3b105a5. Read the comment docs.

@cthoyt
Copy link
Contributor Author

cthoyt commented Jan 6, 2022

@benedekrozemberczki what do you think about changing the way the functions are annotated as redundant?

@benedekrozemberczki
Copy link
Contributor

@cthoyt Feel free to switch around the naming and choose the standard function and annotate the others as you see fit.

@cthoyt cthoyt marked this pull request as ready for review January 6, 2022 12:20
@cthoyt
Copy link
Contributor Author

cthoyt commented Jan 6, 2022

@benedekrozemberczki sorry that the diff is a bit messy - I did some reorganizing of the order of the function definitions. I opted to use the most descriptive names (e.g., use true positive rate over sensitivity) as the defaults.

This PR also now introduces a helper function in the Annotator class to reduce the annotations in the duplicate functions by looking them up from the canonical one.

There's one bizarre quirk I noticed - the recall_score implementation from scikit-learn requires binarization while its counterparts in rexmex (true_positive_rate, etc.) do not.

This PR is ready for review.

@benedekrozemberczki
Copy link
Contributor

Dear @cthoyt,

This looks amazing - thank you for all of the work. For now, we will release 0.1.0. and do a bit of promotion next week for this.

Bests,

Benedek

@benedekrozemberczki benedekrozemberczki merged commit d4673cc into AstraZeneca:main Jan 7, 2022
@cthoyt cthoyt deleted the annotate-duplicates branch January 7, 2022 09:29
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.

None yet

3 participants