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

fix ignored custom callable in retrieval metric aggregation #2364

Merged
merged 7 commits into from
Feb 12, 2024
Merged

fix ignored custom callable in retrieval metric aggregation #2364

merged 7 commits into from
Feb 12, 2024

Conversation

fschlatt
Copy link
Contributor

@fschlatt fschlatt commented Feb 8, 2024

What does this PR do?

Fixes #2363

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--2364.org.readthedocs.build/en/2364/

@fschlatt fschlatt changed the title Master Fix custom retrieval aggregation Feb 8, 2024
@Borda Borda changed the title Fix custom retrieval aggregation fix ignored custom callable in retrieval metric aggregation Feb 8, 2024
@Borda
Copy link
Member

Borda commented Feb 8, 2024

nice, can you pls add test to cover this bug

@fschlatt
Copy link
Contributor Author

fschlatt commented Feb 8, 2024

Tests already exist. The tests compare the computed metric with a reference metric computed using sklearn. The problem is that the computed and reference metrics are aggregated using the same faulty aggregation function. I would propose to copy the fixed aggregation function into the tests. That seems redundant, though.

Copy link

codecov bot commented Feb 9, 2024

Codecov Report

Merging #2364 (6ac2c21) into master (b6f6e07) will decrease coverage by 34%.
The diff coverage is 100%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #2364     +/-   ##
========================================
- Coverage      69%     35%    -34%     
========================================
  Files         305     305             
  Lines       17164   17164             
========================================
- Hits        11835    5955   -5880     
- Misses       5329   11209   +5880     

@SkafteNicki SkafteNicki added this to the v1.3.x milestone Feb 9, 2024
@SkafteNicki SkafteNicki added the bug / fix Something isn't working label Feb 9, 2024
@SkafteNicki SkafteNicki enabled auto-merge (squash) February 9, 2024 12:09
@mergify mergify bot added the ready label Feb 12, 2024
@SkafteNicki SkafteNicki merged commit dce2368 into Lightning-AI:master Feb 12, 2024
58 checks passed
Borda pushed a commit that referenced this pull request Feb 12, 2024
* fix retrieval aggregation

* fix retrieval tests

* changelog

---------

Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>
(cherry picked from commit dce2368)
Borda pushed a commit that referenced this pull request Feb 12, 2024
* fix retrieval aggregation

* fix retrieval tests

* changelog

---------

Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>
(cherry picked from commit dce2368)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom callable is ignored in retrieval metric aggregation
3 participants