-
Notifications
You must be signed in to change notification settings - Fork 468
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
add fscore, reorganize mod and change error in doc. #2648
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2648 +/- ##
==========================================
+ Coverage 81.84% 83.14% +1.29%
==========================================
Files 838 840 +2
Lines 107487 107985 +498
==========================================
+ Hits 87975 89784 +1809
+ Misses 19512 18201 -1311 ☔ View full report in Codecov by Sentry. |
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.
Thanks for contributing yet another metric 😄
Minor comment regarding the name, otherwise should be good to go!
| Accuracy | Calculate the accuracy in percentage | | ||
| TopKAccuracy | Calculate the top-k accuracy in percentage | | ||
| Precision | Calculate precision in percentage | | ||
| Recall | Calculate recall in percentage | | ||
| FScore | Calculate f<sub>β</sub>score in percentage | |
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.
Suggestion: rename to FBetaScore
. It seems that many people refer to F-score as F-beta score (as it is parameterized by beta).
See sklearn for example: https://scikit-learn.org/stable/modules/generated/sklearn.metrics.fbeta_score.html
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.
Also, can you fix the trailing spaces that misalign the right table border 😅
And the F in Fβ score should be capitalized (and maybe add a space like I did for readability?)
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.
Yeah I was torn and chose the smallest name, but I also agree with this, will change!
Ah yes will format the table didn't realize it was misaligned.
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.
While I agree the F should be capitalized, it feels like it isn't consistent with the other metric names, should I also change the others or just capitalize the F?
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.
I think it's consistent. The F-score is always capitalized (based on its origin), ROC is an acronym for Receiver Operating Characteristic. Other names are not capitalized because they're really just nouns (precision, recall, accuracy). I guess there could be a debate over top-k vs Top-k, but even wikipedia doesn't capitalize it 😅
All good, it's been fulfilling to contribute! 🌟 I have now plans to adapt the existing metrics (accuracy+topkaccuracy, auroc and hamming) to the same format as the ones I've implemented. (this will be breaking code, which I'm not fully sure on how to handle) |
Not sure what do yo have in mind? We try to limit breaking changes, but sometimes that's not possible. Depends on the objective I guess!
I think missing features are mostly captured in issues. If a feature request is not already filed, you can open an issue for the request and the discussion can start there 🙂 For other general dev discussions and thoughts, this usually happens on discord. |
Pull Request Template
Checklist
run-checks all
script has been executed.Related Issues/PRs
#544
Changes
Add FβScoreMetric
Reorganize
metric\mod
to mach existing order.Testing
Added tests for new metric