Implement MultioutputWrapper #510

an1lam · 2021-09-09T01:04:11Z

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?

Implements the MultioutputWrapper feature described in #508. Summarizing, MultioutputWrapper takes a base metric and a num_outputs param and returns a metric that will compute shape[output_dim] many scalars and return them in a list. The idea is to support multioutput mode for metrics like SpearmanCorrcoef that don't support it natively.

for more information, see https://pre-commit.ci

an1lam · 2021-09-09T01:33:39Z

I'm not sure why the build is failing but it seems to be totally independent of my change & instead has to do with Docker. It looks like it's also failing on #506, so I'm assuming this is not caused by my change?

SkafteNicki

Few comments, PR is looking good already :]

torchmetrics/wrappers/multioutput.py

tests/wrappers/test_multioutput.py

torchmetrics/wrappers/multioutput.py

- In order to handle cases like `BinnedAveragePrecision`, add `squeeze_outputs` argument which defaults to true and change `compute()`'s return type to a list. - Add two examples to the docs.

for more information, see https://pre-commit.ci

codecov · 2021-09-09T14:22:49Z

Codecov Report

Merging #510 (ea45a8f) into master (9ef98b4) will decrease coverage by 1%.
The diff coverage is 90%.

@@          Coverage Diff          @@
##           master   #510   +/-   ##
=====================================
- Coverage      95%    95%   -1%     
=====================================
  Files         132    133    +1     
  Lines        4660   4717   +57     
=====================================
+ Hits         4444   4467   +23     
- Misses        216    250   +34

an1lam · 2021-09-10T01:19:10Z

@SkafteNicki not exactly sure at what point I move this out of draft mode/whether I am the one supposed to do it. Let me know!

SkafteNicki · 2021-09-10T08:34:30Z

@an1lam Changed it to review status, to get all test running.

SkafteNicki

I am basically fine with the PR as it is, but it would be great to further improve the testing (I doubt it will show any problems as this is just a wrapper around other metrics) :]

tests/wrappers/test_multioutput.py

SkafteNicki · 2021-09-21T14:32:52Z

Hi @an1lam, please check your examples, they do not seem to render correctly:

an1lam · 2021-09-21T15:59:51Z

@SkafteNicki good catch, sorry. I fixed them:

SkafteNicki

LGTM :]

SkafteNicki · 2021-09-21T17:25:11Z

GPU test are failing due to this issue: #531
Because you have the metric as a child module when doing the testing the self.device attribute does not get proper update due to the above issue.

an1lam · 2021-09-21T17:41:06Z

@SkafteNicki do you think I should wait until you merge #542 then (and merge master afterwards) to merge?

SkafteNicki · 2021-09-21T17:43:15Z

@an1lam yes that PR should fix the last test that fails here :]

torchmetrics/wrappers/multioutput.py

an1lam and others added 6 commits September 8, 2021 18:37

Implement MultioutputWrapper

d9b477f

Format docstrings properly

d6f3f15

Update docs & exports

bf57891

[pre-commit.ci] auto fixes from pre-commit.com hooks

d98f918

for more information, see https://pre-commit.ci

Add wrapper to __all__

373859e

Address deepsource flagged issues

6597784

SkafteNicki mentioned this pull request Sep 9, 2021

Implement MultioutputWrapper #509

Closed

4 tasks

SkafteNicki reviewed Sep 9, 2021

View reviewed changes

torchmetrics/wrappers/multioutput.py Show resolved Hide resolved

torchmetrics/wrappers/multioutput.py Outdated Show resolved Hide resolved

tests/wrappers/test_multioutput.py Outdated Show resolved Hide resolved

torchmetrics/wrappers/multioutput.py Outdated Show resolved Hide resolved

an1lam and others added 6 commits September 9, 2021 09:27

Address PR comments

7f713f0

- In order to handle cases like `BinnedAveragePrecision`, add `squeeze_outputs` argument which defaults to true and change `compute()`'s return type to a list. - Add two examples to the docs.

[pre-commit.ci] auto fixes from pre-commit.com hooks

6219c10

for more information, see https://pre-commit.ci

Update docs & make squeeze_outputs actually work

fa02473

Update tests to be randomized and parametrized

5cc7924

[pre-commit.ci] auto fixes from pre-commit.com hooks

424275f

for more information, see https://pre-commit.ci

Fix typechecking error

932414a

Borda assigned SkafteNicki Sep 9, 2021

Merge branch 'master' into master

fddbcaf

SkafteNicki marked this pull request as ready for review September 10, 2021 08:33

SkafteNicki requested review from ananyahjha93, Borda, edenlightning, ethanwharris, justusschock, SeanNaren and tchaton as code owners September 10, 2021 08:33

changelog

cf9cb67

SkafteNicki reviewed Sep 10, 2021

View reviewed changes

tests/wrappers/test_multioutput.py Outdated Show resolved Hide resolved

an1lam and others added 2 commits September 21, 2021 10:07

Remove movedim call for compatibility

7c89fed

Merge branch 'master' into master

acad18d

an1lam added 2 commits September 21, 2021 11:23

Fix additional isnan incompat error

c5b0a3a

Fix docs formatting

49bdda9

Merge branch 'master' into master

ab0beee

SkafteNicki reviewed Sep 21, 2021

View reviewed changes

SkafteNicki approved these changes Sep 21, 2021

View reviewed changes

mergify bot added the ready label Sep 21, 2021

Merge branch 'master' into master

8b2fa0b

Borda reviewed Sep 22, 2021

View reviewed changes

torchmetrics/wrappers/multioutput.py Outdated Show resolved Hide resolved

Apply suggestions from code review

b0c0782

Borda reviewed Sep 22, 2021

View reviewed changes

torchmetrics/wrappers/multioutput.py Outdated Show resolved Hide resolved

Apply suggestions from code review

0230923

Borda reviewed Sep 22, 2021

View reviewed changes

torchmetrics/wrappers/multioutput.py Outdated Show resolved Hide resolved

Apply suggestions from code review

4624470

Borda approved these changes Sep 22, 2021

View reviewed changes

Borda enabled auto-merge (squash) September 22, 2021 08:44

mergify bot and others added 3 commits September 22, 2021 09:38

Merge branch 'master' into master

86fd51c

long

f0f1851

Merge branch 'master' into master

b39c46f

SkafteNicki added this to the v0.6 milestone Sep 24, 2021

fix device

ea45a8f

Borda merged commit 5893793 into Lightning-AI:master Sep 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement MultioutputWrapper #510

Implement MultioutputWrapper #510

an1lam commented Sep 9, 2021 •

edited

Loading

an1lam commented Sep 9, 2021

SkafteNicki left a comment

codecov bot commented Sep 9, 2021 •

edited

Loading

an1lam commented Sep 10, 2021

SkafteNicki commented Sep 10, 2021

SkafteNicki left a comment

SkafteNicki commented Sep 21, 2021

an1lam commented Sep 21, 2021

SkafteNicki left a comment

SkafteNicki commented Sep 21, 2021

an1lam commented Sep 21, 2021

SkafteNicki commented Sep 21, 2021

Implement MultioutputWrapper #510

Implement MultioutputWrapper #510

Conversation

an1lam commented Sep 9, 2021 • edited Loading

Before submitting

What does this PR do?

an1lam commented Sep 9, 2021

SkafteNicki left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 9, 2021 • edited Loading

Codecov Report

an1lam commented Sep 10, 2021

SkafteNicki commented Sep 10, 2021

SkafteNicki left a comment

Choose a reason for hiding this comment

SkafteNicki commented Sep 21, 2021

an1lam commented Sep 21, 2021

SkafteNicki left a comment

Choose a reason for hiding this comment

SkafteNicki commented Sep 21, 2021

an1lam commented Sep 21, 2021

SkafteNicki commented Sep 21, 2021

an1lam commented Sep 9, 2021 •

edited

Loading

codecov bot commented Sep 9, 2021 •

edited

Loading