[WIP] Support topk classification metrics [ci skip]#3822

Closed

rohitgr7 wants to merge 2 commits intomasterfrom

feature/add_topk_support

Contributor

rohitgr7 commented Oct 3, 2020 •

edited

Loading

What does this PR do?

Add support for topk in classification metrics. Let's see how it goes with tests.
TODO:

Add more tests
Update class metrics

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 your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

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 commented Oct 3, 2020 •

edited

Loading

Hello @rohitgr7! Thanks for updating this PR.

In the file pytorch_lightning/metrics/utils.py:

Line 71:1: E302 expected 2 blank lines, found 1
Line 90:1: E302 expected 2 blank lines, found 1
Line 90:121: E501 line too long (122 > 120 characters)
Line 104:13: W503 line break before binary operator

Comment last updated at 2020-11-21 23:06:17 UTC

rohitgr7 mentioned this pull request

[WIP] Top K accuracy metric #3636

Closed

7 tasks

Contributor

oke-aditya commented Oct 3, 2020

This looks better and cleaner. I guess natively supporting top_k is far more useful.

rohitgr7 marked this pull request as ready for review

October 3, 2020 22:29

mergify bot requested a review from a team

October 3, 2020 22:29

rohitgr7 added feature Metrics labels

rohitgr7 requested review from SkafteNicki and justusschock

October 3, 2020 22:31

codecov bot commented Oct 3, 2020 •

edited

Loading

Codecov Report

Merging #3822 (e2b6916) into master (839813e) will decrease coverage by 15%.
The diff coverage is 78%.

@@           Coverage Diff            @@
##           master   #3822     +/-   ##
========================================
- Coverage      92%     77%    -15%     
========================================
  Files         113     118      +5     
  Lines        8278   11313   +3035     
========================================
+ Hits         7632    8766   +1134     
- Misses        646    2547   +1901

justusschock requested changes

View reviewed changes

pytorch_lightning/metrics/functional/classification.py Outdated

+                      topk: Optional[int] = None,
+              ) -> torch.Tensor:
+                  if (target.ndim > 1) and (topk is not None):
+                      raise ValueError(

Member

justusschock Oct 5, 2020

Can you add a test case for this?

Contributor Author

rohitgr7 Oct 5, 2020

yeah sure, it's in TODO actually.

pytorch_lightning/metrics/functional/classification.py Outdated

+                      topk = 1
+                  if topk > pred.size(1):
+                      raise ValueError(

Member

justusschock Oct 5, 2020

also a test_case for this :)

Contributor Author

rohitgr7 Oct 5, 2020

yeah sure.

pytorch_lightning/metrics/functional/classification.py Outdated Show resolved Hide resolved

pytorch_lightning/metrics/functional/classification.py Outdated

+                      pred: predicted labels or probabilities
                       target: ground truth labels
                       num_classes: number of classes
+                      topk: number of most likely outcomes considered to find the correct label

Member

justusschock Oct 5, 2020

Suggested change

      
                    topk: number of most likely outcomes considered to find the correct label
          
                    topk: number of most likely outcomes considered to find the correct label. Defaults to the usual top1 accuracy

pytorch_lightning/metrics/functional/classification.py Outdated

+                      pred: predicted labels or probabilities
                       target: ground-truth labels
                       num_classes: number of classes
+                      topk: number of most likely outcomes considered to find the correct label

Member

justusschock Oct 5, 2020

Suggested change

      
                    topk: number of most likely outcomes considered to find the correct label
          
                    topk: number of most likely outcomes considered to find the correct label. Defaults to the usual top1.

pytorch_lightning/metrics/functional/classification.py Outdated

+                      pred: predicted labels or probabilities
                       target: ground-truth labels
                       num_classes: number of classes
+                      topk: number of most likely outcomes considered to find the correct label

Member

justusschock Oct 5, 2020

Suggested change

      
                    topk: number of most likely outcomes considered to find the correct label
          
                    topk: number of most likely outcomes considered to find the correct label. Defaults to the usual top1.

pytorch_lightning/metrics/functional/classification.py Outdated

+                      pred: predicted labels or probabilities
                       target: ground-truth labels
                       num_classes: number of classes
+                      topk: number of most likely outcomes considered to find the correct label

Member

justusschock Oct 5, 2020

Suggested change

      
                    topk: number of most likely outcomes considered to find the correct label
          
                    topk: number of most likely outcomes considered to find the correct label. Defaults to the usual top1.

mergify bot requested a review from a team

October 5, 2020 07:56

rohitgr7 changed the title ~~Support topk classification metrics~~ [WIP] Support topk classification metrics

Contributor

mergify bot commented Oct 6, 2020

This pull request is now in conflict... :(

1 similar comment

Contributor

mergify bot commented Oct 11, 2020

This pull request is now in conflict... :(

Contributor

teddykoker commented Oct 14, 2020 •

edited

Loading

Here is a top k accuracy implementation using the new Metrics API if it's helpful:

class AccuracyTopK(pl.metrics.Metric):
    def __init__(self, top_k=1, dist_sync_on_step=False):
        super().__init__(dist_sync_on_step=dist_sync_on_step)
        self.k = top_k
        self.add_state("correct", default=torch.tensor(0.0), dist_reduce_fx="sum")
        self.add_state("total", default=torch.tensor(0.0), dist_reduce_fx="sum")

    def update(self, logits, y):
        _, pred = logits.topk(self.k, dim=1)
        pred = pred.t()
        corr = pred.eq(y.view(1, -1).expand_as(pred))
        self.correct += corr[:self.k].sum()
        self.total += y.numel()

    def compute(self):
        return self.correct.float() / self.total

Contributor

oke-aditya commented Oct 15, 2020

This is correct. I used the same formula earlier. Can we simply keep topk as int instead of tuple ?

Contributor

teddykoker commented Oct 15, 2020

@oke-aditya I think I prefer that as well, especially since self.log(metric) only supports scalar values

stale bot commented Oct 29, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

stale bot added the won't fix label

rohitgr7 removed the won't fix label

Contributor

oke-aditya commented Oct 30, 2020

I guess we can replace .view() by .reshape(). It recently threw an error on torch 1.7. It didn't throw in 1.6 though.
Here is the modfied code.

class AccuracyTopK(pl.metrics.Metric):
    def __init__(self, top_k=1, dist_sync_on_step=False):
        super().__init__(dist_sync_on_step=dist_sync_on_step)
        self.k = top_k
        self.add_state("correct", default=torch.tensor(0.0), dist_reduce_fx="sum")
        self.add_state("total", default=torch.tensor(0.0), dist_reduce_fx="sum")

    def update(self, logits, y):
        _, pred = logits.topk(self.k, dim=1, True, True)
        pred = pred.t()
        corr = pred.eq(y.reshape(1, -1).expand_as(pred))
        self.correct += corr[:self.k].reshape(-1).float().sum()
        self.total += y.numel()

    def compute(self):
        return self.correct.float() / self.total

Contributor Author

rohitgr7 commented Oct 31, 2020

@oke-aditya thanks. I'll complete this one in a few days. The metrics package has been changed and I haven't checked it out yet.

Contributor

oke-aditya commented Oct 31, 2020

Great. 😀


          try with new class metric interface

0fa9697

rohitgr7 force-pushed the feature/add_topk_support branch from eb8687d to 0fa9697 Compare

November 1, 2020 18:08

rohitgr7 requested review from Borda, ananyahjha93, awaelchli and nateraw as code owners

November 1, 2020 18:08

rohitgr7 requested review from SeanNaren, tchaton, teddykoker and williamFalcon as code owners

November 1, 2020 18:08

rohitgr7 removed the request for review from a team

November 1, 2020 18:09

stale bot commented Nov 15, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

stale bot added the won't fix label

rohitgr7 removed the won't fix label

rohitgr7 marked this pull request as draft

November 21, 2020 18:17

rohitgr7 changed the title ~~[WIP] Support topk classification metrics~~ [WIP] Support topk classification metrics [ci skip]

rohitgr7 force-pushed the feature/add_topk_support branch from 3061742 to 0fa9697 Compare

November 21, 2020 22:59

ref

e2b6916

rohitgr7 mentioned this pull request

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

Closed

zhutmost commented Nov 27, 2020

Looks that AccuracyTopK is not available in the current version? Will it be in the next version?

Contributor Author

rohitgr7 commented Nov 27, 2020

Hi @zhutmost it will be added in #4838

will close this one after #4838 gets merged. Although you can use the implementation of AccuracyTopK from above for now.

Borda requested a review from justusschock

November 30, 2020 17:31

Borda added this to the 1.1 milestone

Collaborator

Borda commented Nov 30, 2020

@rohitgr7 @justusschock how is it going here, ready to review/merge? 🐰

Contributor Author

rohitgr7 commented Nov 30, 2020

closing this, will be added in #4837 and follow up PRs.

rohitgr7 closed this

rohitgr7 deleted the feature/add_topk_support branch

November 30, 2020 18:44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

SkafteNicki Awaiting requested review from SkafteNicki

ananyahjha93 Awaiting requested review from ananyahjha93

awaelchli Awaiting requested review from awaelchli

Borda Awaiting requested review from Borda

nateraw Awaiting requested review from nateraw

SeanNaren Awaiting requested review from SeanNaren

tchaton Awaiting requested review from tchaton tchaton is a code owner

teddykoker Awaiting requested review from teddykoker

williamFalcon Awaiting requested review from williamFalcon

justusschock Awaiting requested review from justusschock justusschock is a code owner

ethanwharris Awaiting requested review from ethanwharris ethanwharris will be requested when the pull request is marked ready for review ethanwharris is a code owner

Labels

feature