-
Notifications
You must be signed in to change notification settings - Fork 423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dice score as metric #1021
Dice score as metric #1021
Conversation
for more information, see https://pre-commit.ci
…o dice_metric_as_state_scores
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
Codecov Report
@@ Coverage Diff @@
## master #1021 +/- ##
======================================
- Coverage 95% 95% -0%
======================================
Files 180 181 +1
Lines 7828 7873 +45
======================================
+ Hits 7435 7468 +33
- Misses 393 405 +12 |
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 |
@justusschock I would be fine on relying on the |
for more information, see https://pre-commit.ci
…o dice_metric_as_state_scores
@justusschock, May I ask you for help?) I see that all failing checks caused by |
@Borda mind having a look on failing audio deps? |
type fix Co-authored-by: Jirka Borovec <[email protected]>
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
@justusschock thank you for your comment :) I've added deprecation warning. |
add DeprecationWarning Co-authored-by: Justus Schock <[email protected]>
for more information, see https://pre-commit.ci
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Jirka Borovec <[email protected]>
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
@Borda there are plenty of tests that did not run on this PR. Why did you merge it like that? |
@justusschock Did I forget something? I can open a new one and add the necessary tests. This will be inside v0.9 |
No, it's fine now, I restarted the tests and they passed, they just weren't finished when this was merged :D |
Oh, i got it :) thanks for all your advice @Borda @justusschock @SkafteNicki! I get a lot of experience working on your project |
I did auto-merge so all suppose to be fine at that time... or what I missed? |
Almost all our CI configs are not strictly required, but they didn't even run (canceled for some reason) |
What does this PR do?
Fixes #992
Before submitting
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 🙃