Classification metrics overhaul: accuracy metrics (2/n) #4838

tadejsv · 2020-11-24T21:12:09Z

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_accuracy parameter, to calculate subset accuracy in case of multi-label or multi-dimensional multi-class inputs. This parameter is set to False by default, which preserves the default behavior of the old metric.

Additionally, a top_k parameter enables it to be used as TopK accuracy for multi-class inputs (with probabilities) - thanks @rohitgr7 . The top_k format does not yet have an equivalent in sklearn (will in 0.24).

HammingDistance (new)

This is equivalent to hamming_loss from sklearn.

pep8speaks · 2020-11-24T21:12:17Z

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

Borda · 2020-12-17T10:18:24Z

@Borda any chance we can get this merged :) ?

yes, it ready, let me check why it cannot be merged...
@justusschock @teddykoker mind re-review? (your approvals was reseted by the system)

notebooks/04-transformers-text-classification.ipynb

notebooks/05-trainer-flags-overview.ipynb

Borda · 2020-12-17T12:23:13Z

@tadejsv mind rebase on release/1.2-dev to remove all the wrong edits?

SkafteNicki · 2020-12-17T12:31:51Z

@Borda approvals still seems to be dismissed by new commits

…ics_accuracy

tadejsv · 2020-12-17T13:12:33Z

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

Borda · 2020-12-17T14:43:38Z

@Borda approvals still seems to be dismissed by new commits

I know, I am pushing @williamFalcon as I can... @edenlightning @tchaton

Borda · 2020-12-21T10:19:36Z

@tadejsv it seems we resolved the issue with dismissed approvals, but mind resolve conflicts with release/1.2-dev so we can go... 🐰
@justusschock @teddykoker @ananthsub @SkafteNicki mind have a quick look, I believe this was once already approved :]

…ics_accuracy

tadejsv · 2020-12-21T12:22:36Z

@Borda conflicts resolved, we're ready to go :)

pytorch_lightning/metrics/classification/hamming_distance.py

pytorch_lightning/metrics/classification/accuracy.py

pytorch_lightning/metrics/functional/accuracy.py

pytorch_lightning/metrics/functional/hamming_distance.py

Borda

lgtm

rohitgr7

Just one comment. Else LGTM. Nice work!!

CHANGELOG.md

rohitgr7 · 2020-12-21T13:03:50Z

pytorch_lightning/metrics/classification/accuracy.py

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


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

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

yeah, that's what meant here 0.5 by default when we have pred_probs

Got it, will add this in the next PR.

tchaton

Looks great ! minor comments

tchaton · 2020-12-21T13:13:22Z

pytorch_lightning/metrics/functional/accuracy.py

+        tensor(0.6667)
+    """
+
+    if top_k is not None and top_k <= 0:


We should test top_k is not a float.

tchaton · 2020-12-21T13:15:00Z

pytorch_lightning/metrics/functional/classification.py

-        return {'tps': tps, 'sups': sups}
-    return class_reduce(tps, sups, sups, class_reduction=class_reduction)
-
-
 def _confmat_normalize(cm):


Should we provide the normalisation dimension ?

Some people like to visualise both matrices.

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

SkafteNicki

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.

tchaton · 2020-12-21T15:43:53Z

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.

SkafteNicki · 2020-12-21T16:16:26Z

@tchaton no probs. What should we do about it? Solve merge conflicts when master is merged into release/1.2-dev or revert this PR?

tadejsv added 3 commits November 24, 2020 20:48

Add stuff

6959ea0

Change metrics documentation layout

0679015

Add stuff

35627b5

tadejsv requested review from ananyahjha93, awaelchli, Borda, justusschock, nateraw, SeanNaren, tchaton, teddykoker and williamFalcon as code owners November 24, 2020 21:12

tadejsv added 7 commits November 24, 2020 22:41

Change testing utils

55fdaaf

Merge branch 'cls_metrics_input_formatting' into cls_metrics_accuracy

35f8320

Replace len(*.shape) with *.ndim

5cbf56a

More descriptive error message for input formatting

9c33d0b

Replace movedim with permute

6562205

Merge branch 'cls_metrics_input_formatting' into cls_metrics_accuracy

b97aef2

PEP 8 compliance

cbbc769

tadejsv changed the title ~~Classification metrics overhaul: accuracy metrics (2a/n)~~ Classification metrics overhaul: accuracy metrics (2/n) Nov 24, 2020

tadejsv mentioned this pull request Nov 24, 2020

Classification metrics overhaul: stat scores (3/n) #4839

Merged

Division with float

f45fc81

SkafteNicki added feature Is an improvement or enhancement Metrics labels Nov 25, 2020

SkafteNicki added this to the 1.1 milestone Nov 25, 2020

Borda changed the title ~~Classification metrics overhaul: accuracy metrics (2/n)~~ [blocked by #4837] Classification metrics overhaul: accuracy metrics (2/n) [wip] Nov 25, 2020

Borda mentioned this pull request Nov 25, 2020

[WIP] Classification metrics overhaul 1/3 [wip] #4835

Closed

Style changes in error messages

a04a71e

justusschock previously approved these changes Dec 17, 2020

View reviewed changes

notebooks/04-transformers-text-classification.ipynb Outdated Show resolved Hide resolved

notebooks/05-trainer-flags-overview.ipynb Outdated Show resolved Hide resolved

Fix unwanted notebooks change

3bf3c3a

tadejsv dismissed stale reviews from justusschock, rohitgr7, SkafteNicki, and Borda via 3bf3c3a December 17, 2020 10:39

Merge remote-tracking branch 'upstream/release/1.2-dev' into cls_metr…

4412c9c

…ics_accuracy

Merge remote-tracking branch 'upstream/release/1.2-dev' into cls_metr…

a9a4847

…ics_accuracy

justusschock approved these changes Dec 21, 2020

View reviewed changes

Fix too long line in hamming_distance

44135c7

Borda reviewed Dec 21, 2020

View reviewed changes

Borda added 3 commits December 21, 2020 13:43

Apply suggestions from code review

908e60f

Apply suggestions from code review

23a997e

protect

b3e458d

Borda approved these changes Dec 21, 2020

View reviewed changes

rohitgr7 approved these changes Dec 21, 2020

View reviewed changes

Update CHANGELOG.md

92f5f83

tchaton reviewed Dec 21, 2020

View reviewed changes

SkafteNicki approved these changes Dec 21, 2020

View reviewed changes

tchaton merged commit ccffc34 into Lightning-AI:release/1.2-dev Dec 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Classification metrics overhaul: accuracy metrics (2/n) #4838

Classification metrics overhaul: accuracy metrics (2/n) #4838

tadejsv commented Nov 24, 2020 •

edited

Loading

pep8speaks commented Nov 24, 2020 •

edited

Loading

Borda commented Dec 17, 2020

Borda commented Dec 17, 2020

SkafteNicki commented Dec 17, 2020

tadejsv commented Dec 17, 2020

Borda commented Dec 17, 2020

Borda commented Dec 21, 2020

tadejsv commented Dec 21, 2020

Borda left a comment

rohitgr7 left a comment

rohitgr7 Dec 21, 2020 •

edited

Loading

tadejsv Dec 21, 2020

rohitgr7 Dec 21, 2020 •

edited

Loading

tadejsv Dec 21, 2020

tchaton left a comment

tchaton Dec 21, 2020

tchaton Dec 21, 2020

tadejsv Dec 21, 2020

tchaton Dec 21, 2020

tadejsv Dec 21, 2020

SkafteNicki left a comment

tchaton commented Dec 21, 2020

SkafteNicki commented Dec 21, 2020

Classification metrics overhaul: accuracy metrics (2/n) #4838

Classification metrics overhaul: accuracy metrics (2/n) #4838

Conversation

tadejsv commented Nov 24, 2020 • edited Loading

What does this PR do?

Metrics (changes and additions)

Accuracy

HammingDistance (new)

pep8speaks commented Nov 24, 2020 • edited Loading

Comment last updated at 2020-12-21 13:07:13 UTC

Borda commented Dec 17, 2020

Borda commented Dec 17, 2020

SkafteNicki commented Dec 17, 2020

tadejsv commented Dec 17, 2020

Borda commented Dec 17, 2020

Borda commented Dec 21, 2020

tadejsv commented Dec 21, 2020

Borda left a comment

Choose a reason for hiding this comment

rohitgr7 left a comment

Choose a reason for hiding this comment

rohitgr7 Dec 21, 2020 • edited Loading

Choose a reason for hiding this comment

tadejsv Dec 21, 2020

Choose a reason for hiding this comment

rohitgr7 Dec 21, 2020 • edited Loading

Choose a reason for hiding this comment

tadejsv Dec 21, 2020

Choose a reason for hiding this comment

tchaton left a comment

Choose a reason for hiding this comment

tchaton Dec 21, 2020

Choose a reason for hiding this comment

tchaton Dec 21, 2020

Choose a reason for hiding this comment

tadejsv Dec 21, 2020

Choose a reason for hiding this comment

tchaton Dec 21, 2020

Choose a reason for hiding this comment

tadejsv Dec 21, 2020

Choose a reason for hiding this comment

SkafteNicki left a comment

Choose a reason for hiding this comment

tchaton commented Dec 21, 2020

SkafteNicki commented Dec 21, 2020

tadejsv commented Nov 24, 2020 •

edited

Loading

pep8speaks commented Nov 24, 2020 •

edited

Loading

rohitgr7 Dec 21, 2020 •

edited

Loading

rohitgr7 Dec 21, 2020 •

edited

Loading