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

Merged
merged 26 commits into from
Jan 13, 2025

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 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 41%. Comparing base (276b90b) to head (db62975).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #2841     +/-   ##
========================================
- Coverage      68%     41%    -27%     
========================================
  Files         332     332             
  Lines       18966   18970      +4     
========================================
- Hits        12955    7742   -5213     
- Misses       6011   11228   +5217     

@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
@Borda Borda enabled auto-merge (squash) January 13, 2025 13:42
@Borda Borda merged commit 5b0e9b8 into Lightning-AI:master Jan 13, 2025
63 of 64 checks passed
@baskrahmer baskrahmer deleted the optional_num_classes branch January 13, 2025 17:50
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