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 tie breaking in ndcg metric #2031

Merged
merged 11 commits into from
Sep 8, 2023
Merged

Fix tie breaking in ndcg metric #2031

merged 11 commits into from
Sep 8, 2023

Conversation

SkafteNicki
Copy link
Member

@SkafteNicki SkafteNicki commented Aug 29, 2023

What does this PR do?

Fixes #2022
In the issue it was identified that the overall problem is that our implementation of ndcg currently cannot deal with tie breaks of scores. PR fixes this.

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

@SkafteNicki SkafteNicki added the bug / fix Something isn't working label Aug 29, 2023
@SkafteNicki SkafteNicki added this to the v1.1.x milestone Aug 29, 2023
@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Merging #2031 (e94afaa) into master (163b0aa) will decrease coverage by 51%.
Report is 1 commits behind head on master.
The diff coverage is 37%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #2031     +/-   ##
========================================
- Coverage      87%     36%    -51%     
========================================
  Files         283     285      +2     
  Lines       15934   15993     +59     
========================================
- Hits        13832    5691   -8141     
- Misses       2102   10302   +8200     

@Borda Borda enabled auto-merge (squash) August 29, 2023 07:45
@mergify mergify bot added the ready label Aug 29, 2023
@SkafteNicki
Copy link
Member Author

@lucadiliello could you please review as you are the expert on retrieval?

@lucadiliello
Copy link
Contributor

I think this issue may also affect other retrieval metrics. The problem with other retrieval metrics is that definitions are not really standardized between Wikipedia and slides of university courses. I mean that the general definition is the same, but the behavior in corner cases like this one is not well defined.

I'm currently on vacation, I will try to review the PR once I'm in front of my pc.

@mergify mergify bot added the has conflicts label Sep 1, 2023
@Borda
Copy link
Member

Borda commented Sep 1, 2023

I'm currently on vacation, I will try to review the PR once I'm in front of my pc.

cool, @lucadiliello let us know when you ll check and and ideally if all fine approve this PR :)

@mergify mergify bot removed the has conflicts label Sep 6, 2023
@Borda
Copy link
Member

Borda commented Sep 7, 2023

@lucadiliello pls mind checking it ^^

@lucadiliello
Copy link
Contributor

LGTM

@lucadiliello lucadiliello merged commit 1caaf28 into master Sep 8, 2023
66 checks passed
@lucadiliello lucadiliello deleted the bugfix/ndcg_tie_break branch September 8, 2023 07:13
Borda pushed a commit that referenced this pull request Sep 9, 2023
* fix implementation

* add tests

* chlog

---------

Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 1caaf28)
Borda pushed a commit that referenced this pull request Sep 11, 2023
* fix implementation

* add tests

* chlog

---------

Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 1caaf28)
matsumotosan pushed a commit to matsumotosan/metrics that referenced this pull request Sep 19, 2023
* fix implementation

* add tests

* chlog

---------

Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.

Self-Implemented Metrics Behaved Weirdly in DDP Mode
4 participants