-
Notifications
You must be signed in to change notification settings - Fork 127
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
added top_k argument in the run function of ElasticSearcBM25Retriever #130
Changes from 1 commit
82d2af3
35eb043
c074b0a
02e83ac
ca36def
836cb71
3adb30c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,12 +48,12 @@ def from_dict(cls, data: Dict[str, Any]) -> "ElasticsearchBM25Retriever": | |
return default_from_dict(cls, data) | ||
|
||
@component.output_types(documents=List[Document]) | ||
def run(self, query: str): | ||
def run(self, query: str, top_k: int=None): | ||
docs = self._document_store._bm25_retrieval( | ||
query=query, | ||
filters=self._filters, | ||
fuzziness=self._fuzziness, | ||
top_k=self._top_k, | ||
top_k=self._top_k if top_k == None else top_k, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can simplify this statement with |
||
scale_score=self._scale_score, | ||
) | ||
return {"documents": docs} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,17 +64,18 @@ def from_dict(cls, data: Dict[str, Any]) -> "ElasticsearchEmbeddingRetriever": | |
return default_from_dict(cls, data) | ||
|
||
@component.output_types(documents=List[Document]) | ||
def run(self, query_embedding: List[float]): | ||
def run(self, query_embedding: List[float], top_k:int = None): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, |
||
""" | ||
Retrieve documents using a vector similarity metric. | ||
|
||
:param query_embedding: Embedding of the query. | ||
:param top_k: Maximum number of Documents to return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for fixing the docs! It's out of the scope of this PR, but would you mind adding a similar docstring to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The changes you suggested will be done soon. Question: When you mentioned other retriever components you mean in the main haystack right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sahusiddharth no I mean the bm25 retriever in this integration, see https://github.com/deepset-ai/haystack-core-integrations/blob/main/integrations/elasticsearch/src/elasticsearch_haystack/bm25_retriever.py#L51 |
||
:return: List of Document similar to `query_embedding`. | ||
""" | ||
docs = self._document_store._embedding_retrieval( | ||
query_embedding=query_embedding, | ||
filters=self._filters, | ||
top_k=self._top_k, | ||
top_k=self._top_k if top_k == None else top_k, | ||
num_candidates=self._num_candidates, | ||
) | ||
return {"documents": docs} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
top_k
should be typed asOptional