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

Allow logit input in classification metrics #200

Merged
merged 40 commits into from
May 12, 2021
Merged

Allow logit input in classification metrics #200

merged 40 commits into from
May 12, 2021

Conversation

SkafteNicki
Copy link
Member

@SkafteNicki SkafteNicki commented Apr 26, 2021

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Fixes #74
Fixes #60
Fixes Lightning-Universe/lightning-bolts#587

Allow many classification metrics to now work with raw model predictions/logits/unnormalized scores.
List of metrics that gain support and are tested

  • Accuracy
  • Precision
  • Recall
  • F1
  • FBeta
  • StatsScores
  • Hamming
  • ConfusionMatrix

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@pep8speaks
Copy link

pep8speaks commented Apr 26, 2021

Hello @SkafteNicki! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-12 09:02:37 UTC

@codecov
Copy link

codecov bot commented Apr 26, 2021

Codecov Report

Merging #200 (53103e2) into master (b3e42b9) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
- Coverage   96.82%   96.80%   -0.03%     
==========================================
  Files         184       92      -92     
  Lines        6024     3005    -3019     
==========================================
- Hits         5833     2909    -2924     
+ Misses        191       96      -95     
Flag Coverage Δ
Linux 79.10% <ø> (+0.05%) ⬆️
Windows 79.10% <ø> (+0.05%) ⬆️
cpu 96.80% <ø> (-0.01%) ⬇️
gpu ?
macOS 96.80% <ø> (-0.01%) ⬇️
pytest 96.80% <ø> (-0.03%) ⬇️
python3.6 95.74% <ø> (-0.01%) ⬇️
python3.8 96.80% <ø> (?)
python3.9 96.70% <ø> (-0.01%) ⬇️
torch1.3.1 95.74% <ø> (-0.01%) ⬇️
torch1.4.0 95.87% <ø> (?)
torch1.8.1 96.70% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
torchmetrics/classification/accuracy.py 96.15% <ø> (ø)
torchmetrics/classification/confusion_matrix.py 100.00% <ø> (ø)
torchmetrics/classification/f_beta.py 100.00% <ø> (ø)
torchmetrics/classification/hamming_distance.py 100.00% <ø> (ø)
torchmetrics/classification/precision_recall.py 100.00% <ø> (ø)
torchmetrics/classification/stat_scores.py 100.00% <ø> (ø)
torchmetrics/functional/classification/accuracy.py 93.93% <ø> (ø)
...rics/functional/classification/confusion_matrix.py 100.00% <ø> (ø)
torchmetrics/functional/classification/f_beta.py 100.00% <ø> (ø)
...rics/functional/classification/hamming_distance.py 100.00% <ø> (ø)
... and 95 more

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 b3e42b9...53103e2. Read the comment docs.

@Borda Borda marked this pull request as draft April 26, 2021 22:12
CHANGELOG.md Outdated Show resolved Hide resolved
@SkafteNicki SkafteNicki changed the title [WIP] Allow logit input in classification metrics Allow logit input in classification metrics Apr 27, 2021
@SkafteNicki SkafteNicki marked this pull request as ready for review April 27, 2021 12:07
@SkafteNicki SkafteNicki added the Priority Critical task/issue label Apr 27, 2021
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mergify mergify bot added the has conflicts label May 4, 2021
@mergify mergify bot removed the has conflicts label May 4, 2021
@Borda
Copy link
Member

Borda commented May 4, 2021

how about if we preserve the proablility expectation in case threshold is a number and set it as logits for threshold None?

@SkafteNicki thoughts? 🐰

@mergify mergify bot added the has conflicts label May 4, 2021
@maximsch2
Copy link
Contributor

For logits we need to have a threshold as well to convert to binary predictions though?

@mergify mergify bot removed the has conflicts label May 6, 2021
@Borda
Copy link
Member

Borda commented May 7, 2021

another thought shall add bool arg resticted_probability?

@SkafteNicki
Copy link
Member Author

IMO we keep it simple for now by just adding this functionality by removing the explicit check. It will have the minimal change required for code.

Then we probably should re-discuss the concept of task type (started here #186 by @maximsch2) such that we can make it more clear when threshold applies, hopefully remove the multiclass parameter (which I am not a fan of) ect. When that is clear, we can redo classification for version 0.X (hopefully for the last time)

@Borda Borda added the ready label May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants