-
Notifications
You must be signed in to change notification settings - Fork 220
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
perf: transpose the PQ codes to improve search performance #3120
Conversation
Signed-off-by: BubbleCal <[email protected]>
Signed-off-by: BubbleCal <[email protected]>
Signed-off-by: BubbleCal <[email protected]>
Signed-off-by: BubbleCal <[email protected]>
Signed-off-by: BubbleCal <[email protected]>
Signed-off-by: BubbleCal <[email protected]>
Signed-off-by: BubbleCal <[email protected]>
Signed-off-by: BubbleCal <[email protected]>
Signed-off-by: BubbleCal <[email protected]>
Signed-off-by: BubbleCal <[email protected]>
Signed-off-by: BubbleCal <[email protected]>
Signed-off-by: BubbleCal <[email protected]>
Signed-off-by: BubbleCal <[email protected]>
Signed-off-by: BubbleCal <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3120 +/- ##
==========================================
+ Coverage 77.12% 77.13% +0.01%
==========================================
Files 240 240
Lines 80652 80759 +107
Branches 80652 80759 +107
==========================================
+ Hits 62203 62296 +93
Misses 15279 15279
- Partials 3170 3184 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Very cool trick! It looks like transpose is a configurable option of sorts (maybe just at lower levels). Is it possible to setup a unit test that calculates distances with transpose and calculates distances without transpose and verifies they are the same?
#[case(4, DistanceType::Cosine, 0.9)] | ||
#[case(4, DistanceType::Dot, 0.9)] |
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.
Why did these results change? Shouldn't transposing not affect the calculation?
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.
not related to transposing
I think IVF_PQ can always reach this recall, these tests were added when working on HNSW, which would result in low recall with the other distance types, looks like I copied HNSW's tests and forgot to modify the thresholds for IVF_PQ
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.
added a test to verify the results are the same as without transposing
Signed-off-by: BubbleCal <[email protected]>
30% improved: