Dice score as metric #1021

MrShevan · 2022-05-10T07:53:59Z

What does this PR do?

Fixes #992

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?

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 🙃

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

…o dice_metric_as_state_scores

justusschock

Hey, looking pretty good already! Thanks a lot for your implementation!
Left some minor comments. Most of them are just to use immutable types if possible :)

The only one that concerns me a bit is the one about testing :)

torchmetrics/classification/dice.py

torchmetrics/functional/classification/dice.py

torchmetrics/classification/dice.py

tests/classification/test_dice.py

codecov · 2022-05-10T08:04:52Z

Codecov Report

Merging #1021 (508fbe1) into master (6b96ebf) will decrease coverage by 0%.
The diff coverage is 80%.

@@          Coverage Diff           @@
##           master   #1021   +/-   ##
======================================
- Coverage      95%     95%   -0%     
======================================
  Files         180     181    +1     
  Lines        7828    7873   +45     
======================================
+ Hits         7435    7468   +33     
- Misses        393     405   +12

justusschock · 2022-05-10T08:12:25Z

Also: should we update the jaccard implementation to rely on this implementation? Calculating them like this should be faster than computing a whole conf map and J = D/(2-D)

cc @SkafteNicki

SkafteNicki · 2022-05-10T08:19:48Z

@justusschock I would be fine on relying on the Jaccard implementation for this, however I think we should be careful extrapolating the J = D/(2-D) to all cases. I am not completely sure that this formula holds when you take class averaging, multidim averaging etc. into account. That at least need to be checked.

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

…o dice_metric_as_state_scores

MrShevan · 2022-05-11T10:00:56Z

@justusschock, May I ask you for help?) I see that all failing checks caused by installing of dependencies. Could this be related to using scipy? Should I add something in PR to avoid it?

justusschock · 2022-05-11T10:08:05Z

@Borda mind having a look on failing audio deps?

torchmetrics/classification/dice.py

torchmetrics/functional/classification/dice.py

type fix Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

docs/source/classification/dice.rst

justusschock · 2022-05-15T08:20:30Z

One more thing left to do from my side and then it's ready to go :)

Can you raise a deprecationwarning in the older implementation asking users to switch to the newer one?

…o dice_metric_as_state_scores

MrShevan · 2022-05-15T11:48:16Z

@justusschock thank you for your comment :) I've added deprecation warning.

torchmetrics/functional/classification/dice.py

add DeprecationWarning Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>

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

torchmetrics/classification/dice.py

torchmetrics/functional/classification/dice.py

torchmetrics/classification/dice.py

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

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

…o dice_metric_as_state_scores # Conflicts: # torchmetrics/functional/classification/dice.py

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

justusschock · 2022-05-16T08:26:05Z

@Borda there are plenty of tests that did not run on this PR. Why did you merge it like that?

MrShevan · 2022-05-16T09:13:30Z

@justusschock Did I forget something? I can open a new one and add the necessary tests. This will be inside v0.9

justusschock · 2022-05-16T09:22:30Z

No, it's fine now, I restarted the tests and they passed, they just weren't finished when this was merged :D

MrShevan · 2022-05-16T09:28:04Z

Oh, i got it :) thanks for all your advice @Borda @justusschock @SkafteNicki! I get a lot of experience working on your project

Borda · 2022-05-16T09:50:22Z

Why did you merge it like that?

I did auto-merge so all suppose to be fine at that time... or what I missed?

justusschock · 2022-05-16T10:10:53Z

Almost all our CI configs are not strictly required, but they didn't even run (canceled for some reason)

dice metric as statescores

bf2fad2

MrShevan requested review from Borda, SkafteNicki, justusschock, tchaton, ethanwharris and SeanNaren as code owners May 10, 2022 07:53

pre-commit-ci bot and others added 3 commits May 10, 2022 07:54

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

0050886

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

doctest fix

b78f6ae

Merge remote-tracking branch 'origin/dice_metric_as_state_scores' int…

ac77a9d

…o dice_metric_as_state_scores

justusschock reviewed May 10, 2022

View reviewed changes

Apply suggestions from code review

411dc10

Merge branch 'master' into dice_metric_as_state_scores

cd532cb

Borda added the enhancement New feature or request label May 10, 2022

Anton Shevtsov and others added 5 commits May 10, 2022 23:32

test dice from scipy

f6c8e78

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

25470d8

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

fix doctest

dbb2306

Merge remote-tracking branch 'origin/dice_metric_as_state_scores' int…

fd7ecd4

…o dice_metric_as_state_scores

fix docs

064f91c

SkafteNicki added this to the v0.9 milestone May 11, 2022

Merge branch 'master' into dice_metric_as_state_scores

70ce63a

Borda enabled auto-merge (squash) May 12, 2022 14:54

Merge branch 'master' into dice_metric_as_state_scores

2011192

Borda reviewed May 12, 2022

View reviewed changes

torchmetrics/classification/dice.py Outdated Show resolved Hide resolved

torchmetrics/functional/classification/dice.py Show resolved Hide resolved

Update torchmetrics/classification/dice.py

ef7a841

type fix Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

fix too short sphinx error

9f16c51

MrShevan commented May 14, 2022

View reviewed changes

docs/source/classification/dice.rst Outdated Show resolved Hide resolved

Merge branch 'master' into dice_metric_as_state_scores

aecf684

Anton Shevtsov added 3 commits May 15, 2022 13:19

add deprecation warning for dice_score

0fcfc3b

Merge remote-tracking branch 'origin/dice_metric_as_state_scores' int…

7f69ce8

…o dice_metric_as_state_scores

fix doc

c40f262

justusschock approved these changes May 15, 2022

View reviewed changes

torchmetrics/functional/classification/dice.py Outdated Show resolved Hide resolved

mergify bot added the ready label May 15, 2022

MrShevan and others added 2 commits May 15, 2022 16:50

Update torchmetrics/functional/classification/dice.py

19a40f9

add DeprecationWarning Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>

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

a126af2

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

Borda reviewed May 15, 2022

View reviewed changes

MrShevan and others added 9 commits May 15, 2022 17:15

Update torchmetrics/functional/classification/dice.py

4ef2cb1

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Update torchmetrics/functional/classification/dice.py

cfb6870

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Update torchmetrics/classification/dice.py

fe4c0d4

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

Update torchmetrics/classification/dice.py

e3d37f1

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>

replace dice_score logic to dice func

a97b712

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

618da06

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

fix pep

7785894

Merge remote-tracking branch 'origin/dice_metric_as_state_scores' int…

a26a4bb

…o dice_metric_as_state_scores # Conflicts: # torchmetrics/functional/classification/dice.py

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

508fbe1

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

Borda approved these changes May 16, 2022

View reviewed changes

Borda merged commit af5af72 into Lightning-AI:master May 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dice score as metric #1021

Dice score as metric #1021

MrShevan commented May 10, 2022 •

edited

justusschock left a comment

codecov bot commented May 10, 2022 •

edited

justusschock commented May 10, 2022

SkafteNicki commented May 10, 2022

MrShevan commented May 11, 2022

justusschock commented May 11, 2022

justusschock commented May 15, 2022

MrShevan commented May 15, 2022

justusschock commented May 16, 2022

MrShevan commented May 16, 2022

justusschock commented May 16, 2022

MrShevan commented May 16, 2022

Borda commented May 16, 2022

justusschock commented May 16, 2022

Dice score as metric #1021

Dice score as metric #1021

Conversation

MrShevan commented May 10, 2022 • edited

What does this PR do?

Before submitting

PR review

Did you have fun?

justusschock left a comment

Choose a reason for hiding this comment

codecov bot commented May 10, 2022 • edited

Codecov Report

justusschock commented May 10, 2022

SkafteNicki commented May 10, 2022

MrShevan commented May 11, 2022

justusschock commented May 11, 2022

justusschock commented May 15, 2022

MrShevan commented May 15, 2022

justusschock commented May 16, 2022

MrShevan commented May 16, 2022

justusschock commented May 16, 2022

MrShevan commented May 16, 2022

Borda commented May 16, 2022

justusschock commented May 16, 2022

MrShevan commented May 10, 2022 •

edited

codecov bot commented May 10, 2022 •

edited