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

feat: adding Maximum Margin Relevance Ranker #8554

Merged
merged 42 commits into from
Nov 22, 2024

Conversation

davidsbatista
Copy link
Contributor

@davidsbatista davidsbatista commented Nov 18, 2024

Related Issues

Proposed Changes:

  • I've added a new strategy to rank the documents based on diversity. The function implements the Maximum Marginal Relevance (MMR) scoring and uses it for ranking the documents based on similarity with the query and diversity from the already selected documents (see: PDF)

  • Added to new init parameters, to decide which diversity strategy is used, and another one to set the threshold between similarity with the query and document diversity.

  • Extracted some common code from both strategies into a new private function.

How did you test it?

  • Manual verification, plus local tests run successfully

Notes for the reviewer

  • I'm trying to debug and understand why the CI coverage tool doesn't account for the new test I've added

Checklist

@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Nov 18, 2024
@davidsbatista davidsbatista changed the title Adding mmr score ranker feat: adding Maximum Margin Relevance Ranker Nov 18, 2024
@coveralls
Copy link
Collaborator

coveralls commented Nov 18, 2024

Pull Request Test Coverage Report for Build 11974762094

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 6 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.004%) to 90.286%

Files with Coverage Reduction New Missed Lines %
components/rankers/sentence_transformers_diversity.py 6 95.86%
Totals Coverage Status
Change from base Build 11974755029: 0.004%
Covered Lines: 7937
Relevant Lines: 8791

💛 - Coveralls

@davidsbatista davidsbatista marked this pull request as ready for review November 18, 2024 16:42
@davidsbatista davidsbatista requested review from a team as code owners November 18, 2024 16:42
@davidsbatista davidsbatista requested review from dfokina, anakin87 and vblagoje and removed request for a team November 18, 2024 16:42
Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

Nice addition!

I had a first look and left some comments.
Let's also add some unit tests.

Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

I will let @shadeMe take a final look, but LGTM

@anakin87
Copy link
Member

Merge main to remove linting failures.

@davidsbatista davidsbatista enabled auto-merge (squash) November 22, 2024 14:47
@davidsbatista davidsbatista merged commit b5a2fad into main Nov 22, 2024
18 checks passed
@davidsbatista davidsbatista deleted the adding-MMR-score-ranker branch November 22, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding Maximum Margin Relevance Ranker
6 participants