-
Notifications
You must be signed in to change notification settings - Fork 52
adds support for int8 and uint8 dtypes in vectorizers #279
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
Conversation
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.
Looks like we beat you to merge here and you might need to rebase
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.
looking good, left a few notes
@@ -19,4 +19,5 @@ vectorizer: | |||
model: sentence-transformers/all-mpnet-base-v2 | |||
routing_config: | |||
max_k: 2 | |||
aggregation_method: avg |
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.
@rbs333 this is related to what I DM'd you about regarding the deprecated distance_threshold
arg
@@ -316,6 +320,7 @@ async def aembed_many( | |||
] | |||
return embeddings | |||
|
|||
@deprecated_argument("dtype") |
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.
😂 This looks like it was fun
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.
Looks good! 👍
eadb932
This PR includes support for two integer dtypes, and also markes the 'dtype' kwarg in embedding methods as deprecated.