-
Notifications
You must be signed in to change notification settings - Fork 71
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
fix: cosine distance #231
fix: cosine distance #231
Conversation
kemingy
commented
Jan 5, 2024
- refer to feat: Compatibility with pgvector #227
Signed-off-by: Keming <kemingyang@tensorchord.ai>
How about PQ and SQ distance? |
Signed-off-by: Keming <kemingyang@tensorchord.ai>
Fixed in the new commit. PTAL |
Can we just use |
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 suggest add a display_distance function to the G
trait and use it for displaying.
I don't have the plan to replace the squared L2 distance. |
Please resolve the conflicts.
It will add more overhead to the compute, but we do not see the benefits. Thus let's keep no sqrt for now. |
fixed |
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.
Please fix:
https://github.com/tensorchord/pgvecto.rs/blob/a7a9a592c1ef8db0c41ff106b62c85359c10a3b1/README.md?plain=1#L109
https://github.com/tensorchord/pgvecto.rs/blob/a7a9a592c1ef8db0c41ff106b62c85359c10a3b1/README.md?plain=1#L118
https://github.com/tensorchord/pgvecto.rs/blob/a7a9a592c1ef8db0c41ff106b62c85359c10a3b1/crates/service/src/instance/metadata.rs#L21
I think this is the better way. We can add sqrt to PostgresSQL operator. If user didn't use L2 instance in the query, no extra cost will be introduced? And we can make it consistent with pgvector at all. @usamoi Will the cost of sqrt happen when user doesn't need distance in the query? |
No, there will be no extra cost. What I cares is consistency: we use |
Cosine distance has to be fixed because it's defined as |
We didn't implement |