-
Notifications
You must be signed in to change notification settings - Fork 1.5k
add similarity threshold capability to VectorStoreChatMemoryAdvisor #3454
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
base: main
Are you sure you want to change the base?
add similarity threshold capability to VectorStoreChatMemoryAdvisor #3454
Conversation
…visor Signed-off-by: Peter Keeler <[email protected]>
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.
It would be great if these minor issues could be fixed.
Assert.notNull(systemPromptTemplate, "systemPromptTemplate cannot be null"); | ||
Assert.isTrue(defaultTopK > 0, "topK must be greater than 0"); | ||
Assert.isTrue(defaultSimilarityThreshold >= 0, "similarityThreshold must be equal to or greater than 0"); |
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.
According to the assertion of methodorg.springframework.ai.vectorstore.SearchRequest.Builder#similarityThreshold
.
The defaultSimilarityThreshold
should be defaultSimilarityThreshold >= 0 && defaultSimilarityThreshold <= 1
Assert.isTrue(defaultSimilarityThreshold >= 0, "similarityThreshold must be equal to or greater than 0"); | |
Assert.isTrue(defaultSimilarityThreshold >= 0 && defaultSimilarityThreshold <= 1, "similarityThreshold must be in [0,1] range"); |
* retrieve | ||
* @return this builder | ||
*/ | ||
public Builder defaultSimilarityThreshold(Double defaultSimilarityThreshold) { |
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.
Maybe we don't need to use the Double wrapper class here?
@@ -221,6 +237,8 @@ public static class Builder { | |||
|
|||
private Integer defaultTopK = DEFAULT_TOP_K; | |||
|
|||
private Double defaultSimilarityThreshold = DEFAULT_SIMILARITY_THRESHOLD; |
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.
Same comment as above.
This advisor did not have support for setting the similarity threshold, so it defaulted to returning every document regardless of similarity. In a conversation where the user has changed to a different topic the result was returning a bunch of documents with no relevance to the user's latest input.
I experimented with setting a value greater than 0 and liked the results because it would begin to include results more relevant to the conversation. I thought it would be nice to contribute the change back.
I left the default at 0 so as not to change the earlier behavior.