-
Notifications
You must be signed in to change notification settings - Fork 797
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
[demo] Add binary & scalar embedding quantization #683
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR! Would you mind separating out the example app from the core quantization logic? We can possibly integrate the example app at a later stage, but I think a good starting point for now is to just provide a bit of sample code. |
Ah, yep forgot to this a while back, here it is! Let me know of any changes you'd like |
Feel free to rebase off main (which includes your other PR: #681) 🤗 |
This is awesome! Maybe a little more explanation in the docs would be good. Like what's the (performance) difference between binary and ubinary here and what's the default right now (I guess none)? @xenova I'm a little confused that there is no option |
yep idea is to not pass the param if you don't plan on using it, its set as a default param / named param https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters I can improve the docs for this for sure |
Thanks for your reply. Found the related line here: if (!['binary', 'ubinary'].includes(precision)) {
throw new Error("The precision must be either 'binary' or 'ubinary'");
} I understand the idea of the paradigm. However, it causes silightly more work for downstream applications. In my case, I wanted to create a dropdown menu for UI users with 3 values:
I would always pass the |
Hmm good call out, the reason I added quantize was to make it explicit you'd be quantizing. It technically does add more complexity from a UI / code perspective Trying to think if quantize yes/no would add another field besides precision, if so then quantizeOptions would need to be created, can't think of anything at the moment. technically we could flatten out the param to just be 'precision' to be super clear in your suggestion:your current scenario makes your UI render the dropdown conditionally based on if the 'quantize embedding' is set to true eg
So basically right now the cases are: cases now: 4
cases if flattend: 3
|
UI
Exactly. One dropdown with all 3 flattened options would be much easier to use than conditionally changing the UI and JSON I need to pass to the function. -- By the way: there are now two |
+1 with do-me |
This PR addressed the below issue, I've also added an example project showcasing the binary embeddings and using hamming distance function as @xenova gave an example of earlier in the below discussion.
Closes [feat] Add binary & scalar embedding quantization support to Transformers.js #681
Let me know if you'd like any changes to this, perhaps we can make another options object in javascript like a QuantizationOptions so it's obvious which parameters are grouped together logically
I also added a texts === undefined check since I ran into an issue earlier where I was accidentally not passing in any texts and was met with an unfriendly error.
For some reason though I don't get the same similarity numbers as Xenova did on his example but the numbers are at least similar
Hopefully the UI can help debug some of my code if there needs to be changes.
Here's an image of it: