Classification metrics overhaul: accuracy metrics (2/n) #4838
Conversation
tadejsv
requested review from
ananyahjha93,
awaelchli,
Borda,
justusschock,
nateraw,
SeanNaren,
tchaton,
teddykoker and
williamFalcon
as code owners
|
Hello @tadejsv! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-12-21 13:07:13 UTC |
tadejsv
changed the title
Borda
changed the title
yes, it ready, let me check why it cannot be merged... |
tadejsv
dismissed stale reviews from justusschock, rohitgr7, SkafteNicki, and Borda
via
3bf3c3a
|
@tadejsv mind rebase on |
|
@Borda approvals still seems to be dismissed by new commits |
|
@Borda I already rebased (and I did it again right now). The problem is that I rebased to master in the time period between when 1.2-dev branch was created, and when it was announced in the slack channel. In my opinion that branch should have been rebased to master right before it was announced. It doesn't contain, for example, PR #5134, which is "accidentally" picked up in this PR. If I remove this then tests here will fail, if I disabled DDP tests then another PR will be needed once 1.2-dev is rebased on master to re-enable these tests... |
I know, I am pushing @williamFalcon as I can... @edenlightning @tchaton |
|
@tadejsv it seems we resolved the issue with dismissed approvals, but mind resolve conflicts with |
|
@Borda conflicts resolved, we're ready to go :) |
| This is the case for binary and multi-label logits. | ||
|
|
||
| If preds has an extra dimension as in the case of multi-class scores we perform an argmax on ``dim=1``. | ||
| Accepts all input types listed in :ref:`metrics:Input types`. | ||
|
|
||
| Args: | ||
| threshold: |
There was a problem hiding this comment.
just wondering, should threshold be None too by default?? because it's not used when we provider pred_labels, and use 0.5 by default when we have pred_probs
There was a problem hiding this comment.
Well, I guess this could be done - but in a way that if pred_probs are passed in, None would default to 0.5. otherwise this would be a very disturbing breaking change for people used to using accuracy without extra params
There was a problem hiding this comment.
yeah, that's what meant here 0.5 by default when we have pred_probs
There was a problem hiding this comment.
Got it, will add this in the next PR.
| tensor(0.6667) | ||
| """ | ||
|
|
||
| if top_k is not None and top_k <= 0: |
There was a problem hiding this comment.
We should test top_k is not a float.
| return {'tps': tps, 'sups': sups} | ||
| return class_reduce(tps, sups, sups, class_reduction=class_reduction) | ||
|
|
||
|
|
||
| def _confmat_normalize(cm): |
There was a problem hiding this comment.
Should we provide the normalisation dimension ?
There was a problem hiding this comment.
Some people like to visualise both matrices.
There was a problem hiding this comment.
Sorry, I don't understand. If you are speaking about the confusion matrix - that is not part of this PR (this one is about the accuracy metric).
There was a problem hiding this comment.
LGTM, wait with merge to after release branch gets synced with master tomorrow. Then this PR just needs to updated such that the fix from #5134 is not part of it anymore.
Oh sorry @SkafteNicki, I didn't have your message when I merged. I refreshed the page and saw it. |
|
@tchaton no probs. What should we do about it? Solve merge conflicts when |
This PR is a spin-off from #4835.
What does this PR do?
Metrics (changes and additions)
The metrics below are based on the new input formatting function from #4837
Accuracy
The accuracy metric now get a new
subset_accuracyparameter, to calculate subset accuracy in case of multi-label or multi-dimensional multi-class inputs. This parameter is set toFalseby default, which preserves the default behavior of the old metric.Additionally, a
top_kparameter enables it to be used as TopK accuracy for multi-class inputs (with probabilities) - thanks @rohitgr7 . Thetop_kformat does not yet have an equivalent in sklearn (will in 0.24).HammingDistance (new)
This is equivalent to
hamming_lossfrom sklearn.