-
Notifications
You must be signed in to change notification settings - Fork 74
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
[breaking] improve API by removing the Quantizer class and adds in memory support #21
Conversation
autofaiss/external/quantize.py
Outdated
|
||
|
||
def quantize( | ||
embeddings_path: str, |
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.
Can you change name to embeddings_or_path and type to Union[str, np.ndarray] ?
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.
yes done
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.
I changed the type but actually I think it's better if we don't change the argument name, it would be an annoying breaking change with no obvious benefit
autofaiss/external/quantize.py
Outdated
print("Recap:") | ||
pp(metric_infos) | ||
|
||
return index |
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.
Can you also return the output path ?
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.
yes
203e0e3
to
14bd0d6
Compare
d26766c
to
2afcb3c
Compare
…ndex_path explicit
reviewed with victor |
No description provided.