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

Add parameter top_k to RetrievalMAP #1501

Merged
merged 47 commits into from
Feb 24, 2023
Merged

Add parameter top_k to RetrievalMAP #1501

merged 47 commits into from
Feb 24, 2023

Conversation

Bomme
Copy link
Contributor

@Bomme Bomme commented Feb 12, 2023

What does this PR do?

Fixes #1491

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 🙃

e.g. MAP@10, i.e. consider only the top k elements for each query.
@codecov
Copy link

codecov bot commented Feb 12, 2023

Codecov Report

Merging #1501 (eb2f12e) into master (6bc249d) will decrease coverage by 53%.
The diff coverage is 85%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1501     +/-   ##
========================================
- Coverage      88%     36%    -53%     
========================================
  Files         216     216             
  Lines       11334   11344     +10     
========================================
- Hits        10025    4047   -5978     
- Misses       1309    7297   +5988     

Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

Great PR :]

@SkafteNicki
Copy link
Member

@lucadiliello please review :)

@SkafteNicki SkafteNicki added the enhancement New feature or request label Feb 13, 2023
@SkafteNicki SkafteNicki added this to the v0.12 milestone Feb 13, 2023
lucadiliello

This comment was marked as duplicate.

Copy link
Contributor

@lucadiliello lucadiliello left a comment

Choose a reason for hiding this comment

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

LGTM. Some tests are failing but I don't understand why.

@Borda Borda changed the title Add parameter k to RetrievalMAP Add parameter top_k to RetrievalMAP Feb 13, 2023
@Borda Borda enabled auto-merge (squash) February 13, 2023 17:50
@mergify mergify bot removed the has conflicts label Feb 20, 2023
@lucadiliello
Copy link
Contributor

I will do a last review of the failing tests tomorrow before merging.

@lucadiliello
Copy link
Contributor

Since we are left only with RetrievalMRR without support for top_k parameter, should we add support for it in this PR?

@mergify mergify bot added the ready label Feb 23, 2023
@SkafteNicki
Copy link
Member

Since we are left only with RetrievalMRR without support for top_k parameter, should we add support for it in this PR?

@lucadiliello Feel free to do so :)

@Borda Borda enabled auto-merge (squash) February 23, 2023 16:48
@Borda Borda merged commit 40dd1f2 into Lightning-AI:master Feb 24, 2023
@Bomme Bomme deleted the map_at_k branch March 14, 2023 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce k parameter in RetrievalMAP to compute at cut-off k
5 participants