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: Adds all_terms_must_match parameter to BM25Retriever at runtime #3627

Conversation

ugm2
Copy link
Contributor

@ugm2 ugm2 commented Nov 25, 2022

Related Issues

Proposed Changes:

Adds all_terms_must_match as a runtime parameter to BM25Retriever. This allows the user to switch between partial or full matching of words without having to recreate the Node or the Pipeline.

How did you test it?

I added a unit test where I test passing the parameter to both the init and the run functions.

Checklist

@ugm2 ugm2 requested a review from a team as a code owner November 25, 2022 08:37
@ugm2 ugm2 requested review from bogdankostic and removed request for a team November 25, 2022 08:37
Signed-off-by: Unai Garay <unaigaraymaestre@gmail.com>
@ZanSara ZanSara requested review from ZanSara and mayankjobanputra and removed request for bogdankostic and ZanSara November 25, 2022 10:25
@mayankjobanputra
Copy link
Collaborator

@ugm2 Thanks for opening the PR. I will get back to you on this in a day :)

haystack/nodes/retriever/sparse.py Outdated Show resolved Hide resolved
haystack/nodes/retriever/sparse.py Outdated Show resolved Hide resolved
haystack/nodes/retriever/sparse.py Show resolved Hide resolved
haystack/nodes/retriever/sparse.py Show resolved Hide resolved
Copy link
Contributor Author

@ugm2 ugm2 left a comment

Choose a reason for hiding this comment

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

Comments addressed, but mypy keeps failing. Also, I've merged haystack's main branch into mine

haystack/nodes/retriever/sparse.py Outdated Show resolved Hide resolved
haystack/nodes/retriever/sparse.py Show resolved Hide resolved
haystack/nodes/retriever/sparse.py Show resolved Hide resolved
haystack/nodes/retriever/sparse.py Outdated Show resolved Hide resolved
@mayankjobanputra
Copy link
Collaborator

Comments addressed, but mypy keeps failing. Also, I've merged haystack's main branch into mine

I am looking into it! :)

Copy link
Contributor

@agnieszka-m agnieszka-m left a comment

Choose a reason for hiding this comment

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

HI! Just some changes to align with our tone of voice guidelines :)

haystack/nodes/retriever/sparse.py Outdated Show resolved Hide resolved
haystack/nodes/retriever/sparse.py Outdated Show resolved Hide resolved
haystack/nodes/retriever/sparse.py Outdated Show resolved Hide resolved
haystack/nodes/retriever/sparse.py Outdated Show resolved Hide resolved
haystack/nodes/retriever/sparse.py Outdated Show resolved Hide resolved
haystack/nodes/retriever/sparse.py Outdated Show resolved Hide resolved
ugm2 and others added 5 commits December 7, 2022 22:12
Co-authored-by: Agnieszka Marzec <97166305+agnieszka-m@users.noreply.github.com>
Co-authored-by: Agnieszka Marzec <97166305+agnieszka-m@users.noreply.github.com>
Co-authored-by: Agnieszka Marzec <97166305+agnieszka-m@users.noreply.github.com>
Co-authored-by: Agnieszka Marzec <97166305+agnieszka-m@users.noreply.github.com>
Co-authored-by: Agnieszka Marzec <97166305+agnieszka-m@users.noreply.github.com>
Co-authored-by: Agnieszka Marzec <97166305+agnieszka-m@users.noreply.github.com>
@ugm2 ugm2 requested review from mayankjobanputra and agnieszka-m and removed request for mayankjobanputra and agnieszka-m December 7, 2022 21:20
Copy link
Collaborator

@mayankjobanputra mayankjobanputra left a comment

Choose a reason for hiding this comment

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

Kudos @ugm2 !

LGTM!

@ugm2 ugm2 requested a review from agnieszka-m December 8, 2022 07:39
Copy link
Contributor

@agnieszka-m agnieszka-m left a comment

Choose a reason for hiding this comment

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

LGTM!

@mayankjobanputra mayankjobanputra merged commit 77cea8b into deepset-ai:main Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:retriever type:feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

all_terms_must_match in BM25Retriever should be able to be passed at runtime too
3 participants