Skip to content
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

Make num_classes optional, in case of micro averaging #2841

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

baskrahmer
Copy link
Collaborator

@baskrahmer baskrahmer commented Nov 22, 2024

What does this PR do?

Fixes #2837

Before submitting
  • Was this discussed/agreed 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 🙃


📚 Documentation preview 📚: https://torchmetrics--2841.org.readthedocs.build/en/2841/

@Borda
Copy link
Member

Borda commented Nov 27, 2024

@baskrahmer how much is missing here?

@baskrahmer
Copy link
Collaborator Author

@baskrahmer how much is missing here?

I just need to check why the CI is failing.

@baskrahmer baskrahmer marked this pull request as ready for review December 8, 2024 12:05
Copy link

codecov bot commented Dec 8, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 41%. Comparing base (e690bbd) to head (03f70ae).

❗ There is a different number of reports uploaded between BASE (e690bbd) and HEAD (03f70ae). Click for more details.

HEAD has 74 uploads less than BASE
Flag BASE (e690bbd) HEAD (03f70ae)
gpu 3 0
unittest 3 0
torch2.0.1+cpu 6 3
python3.10 18 9
Windows 6 3
cpu 34 17
macOS 8 4
torch2.0.1 4 2
torch2.6.0+cpu 4 2
python3.11 8 4
torch2.6.0 2 1
python3.12 6 3
torch2.5.0 2 1
torch2.5.0+cpu 6 3
Linux 20 10
torch2.4.1+cpu 4 2
torch2.1.2+cpu 2 1
torch2.2.2+cpu 2 1
torch2.3.1+cpu 2 1
python3.9 2 1
Additional details and impacted files
@@           Coverage Diff            @@
##           master   #2841     +/-   ##
========================================
- Coverage      69%     41%    -28%     
========================================
  Files         346     332     -14     
  Lines       19142   18968    -174     
========================================
- Hits        13230    7741   -5489     
- Misses       5912   11227   +5315     

@mergify mergify bot added the ready label Dec 8, 2024
@Borda Borda changed the title Make num_classes optional, in case of micro averaging Make num_classes optional, in case of micro averaging Dec 11, 2024
Copy link
Contributor

@lantiga lantiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@lantiga
Copy link
Contributor

lantiga commented Dec 21, 2024

Actually, shouldn't we check whether num_classes is not passed when microaveraging is not None and produce an informative error in that case? Otherwise we silently default to 1 and that wouldn't be desirable IMO.

@mergify mergify bot removed the has conflicts label Dec 21, 2024
@@ -307,7 +307,7 @@ class MulticlassStatScores(_AbstractStatScores):

def __init__(
self,
num_classes: int,
num_classes: Optional[int] = None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Successfully merging this pull request may close these issues.

num_classes should not be required when using micro averaging
3 participants