torchmetrics

torchmetrics copied to clipboard

What does this PR do?

Fixes #1089 . Didn't manage to make it work for multidimensional multilabel scenarios. However, that shouldn't a very common use case.

Before submitting

• [x] Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• [x] Did you read the contributor guideline, Pull Request section?
• [x] Did you make sure to update the docs?
• [x] 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 🙃

Jun 15 '22 16:06 jlcsilva

I beilve that this shall be automatically resolved in cmd by git merge upstream/master

Jun 17 '22 13:06 Borda

I beilve that this shall be automatically resolved in cmd by git merge upstream/master

I tried that, but got merge: upstream/master - not something we can merge. Any tips? I searched the error, but I'm a bit of a git rookie, so I'm afraid I'll break stuff instead of fixing it 😅

Jun 17 '22 15:06 jlcsilva

@jlcsilva shall be resolved now, pls check if I did not make a mistake... :rabbit:

Jun 20 '22 09:06 Borda

Codecov Report

Merging #1090 (bd375c0) into master (82ab513) will decrease coverage by 35%. The diff coverage is 87%.

@@           Coverage Diff            @@
##           master   #1090     +/-   ##
========================================
- Coverage      69%     33%    -35%     
========================================
  Files         307     311      +4     
  Lines       17427   17527    +100     
========================================
- Hits        11989    5849   -6140     
- Misses       5438   11678   +6240     

Jun 20 '22 12:06 codecov[bot]

@jlcsilva shall be resolved now, pls check if I did not make a mistake... 🐰

Seems ok to me.

Also, I know why the tests are failing, but I'm unsure about what to do. In the file tests/unittests/classification/inputs.py I changed the data type of some input tensors, such as _input_binary, from int to float, to match the description here. Since the tests are failing, I'm not sure what's wrong, the documentation or the code. Any thoughts from someone with a better overall view of the repo?

Jun 22 '22 16:06 jlcsilva

Hi @jlcsilva, when looking at the changes in the PR there also seems to be some changes to other metrics. Any particular reason for this?

Jun 28 '22 11:06 SkafteNicki

@SkafteNicki I believe they are coming from resolving the merge conflicts

Jun 28 '22 14:06 justusschock

Hi @jlcsilva, when looking at the changes in the PR there also seems to be some changes to other metrics. Any particular reason for this?

In the StatScore class I added the None option for the reduce argument, which I needed for my generalized dice score implementation, and changed its documentation and that of its subclasses accordingly. Additionally, I corrected some typos and added some missing information to these classes' documentation.

Finally, I changes the data type of some test inputs to match that in the general input type documentation page, and added new multidimensional input tensors. To me, this seems to be what is making the code fail the tests. Currently, I'm out of office, but I'll check it as soon as I go back to work

Jun 28 '22 21:06 jlcsilva

Hey @SkafteNicki, No problem! I've also been away, so wasn't able to help. I noticed the changes when I came back and pulled the repo again. I'm actually a bit busy today and next week, but let me know if there is something I can help with :)

Aug 05 '22 13:08 jlcsilva

@jlcsilva @SkafteNicki, mind checking the collisions after the refactor was merged... :)

Sep 13 '22 22:09 Borda

@jlcsilva @stancld, mind checking the collisions after the refactor was merged... :otter:

Oct 05 '22 10:10 Borda

@Borda I'll do rebase now.

Oct 05 '22 10:10 stancld

ℹ️ It will be needed to add test cases after refactoring tests as a part of the classification refactor. (I can do it on Friday, I guess)

Oct 05 '22 10:10 stancld

Going to take over this PR now so we hopefully can land it within the next week :]

Nov 06 '22 12:11 SkafteNicki

Going to take over this PR now so we hopefully can land it within the next week :]

hi @SkafteNicki how is it going here? :)

Dec 23 '22 05:12 Borda

@justusschock, how much need to be done? :chipmunk:

Jan 30 '23 16:01 Borda

@SkafteNicki could you pls re-check as you had some requested changes... :chipmunk:

Apr 26 '23 12:04 Borda

For future reference, we should compare against this implementation: https://github.com/Project-MONAI/MONAI/blob/dev/monai/metrics/generalized_dice.py

May 17 '23 12:05 SkafteNicki

@jlcsilva, any chance you could work on this one, in particular, add some tests... :flags:

Aug 23 '23 21:08 Borda

seems we hit some internal Gh issue, will be restarted later...

Jan 09 '24 13:01 Borda