[REVIEW] Add slow high-precision mode to KNN #3304
Conversation
Provide mode to perform a second high-precision pass over results returned from brute-force KNN searches which make use of L2-derived metrics. This provides a workaround for issues with numerical instability in L2 distance calculations in FAISS when a query vector is quite close to multiple retrieved samples relative to the typical inter-sample distance.
wphicks
added
feature request
wphicks
added
3 - Ready for Review
wphicks
changed the title
|
Additional detail on the data that first demonstrated the necessity of this new flag is available here: #3195 (comment). |
There was a problem hiding this comment.
Minor suggestions on comments and array output_type handling. One thing I would like to see is some before/after testing added. For example, running once with two_pass_precision=False, then again with two_pass_precision=True and comparing that the output changed. This will help prove that the fix is working as intended.
wphicks
added
4 - Waiting on Reviewer
Sadly, this is not possible to write in an environment-neutral way. Because of how the errors propagate (or not) in the distance approximations, we've seen environments where this issue never comes up and environments where it occurs every time. Poor @cjnolet slogged away at this one for awhile but was unlucky (lucky?) enough to be on a system where it never came up. Even more illustratively, the PR that I used to test for this issue in CI passed with no problem before the fix was in, even though I was consistently seeing local failures. |
Codecov Report
@@ Coverage Diff @@
## branch-0.18 #3304 +/- ##
===============================================
+ Coverage 71.48% 71.66% +0.17%
===============================================
Files 207 210 +3
Lines 16748 16945 +197
===============================================
+ Hits 11973 12144 +171
- Misses 4775 4801 +26
Continue to review full report at Codecov.
|
|
To provide a little more clarity on my last comment, the unit tests introduced here consistently failed in my local environment before the included fix was provided and consistently passed after the fix was introduced. On CI and in other environments, that same unit tests would consistently pass even before the fix was introduced. |
|
rerun tests |
|
Merging in latest mainline to see if that will fix seemingly unrelated CI errors |
JohnZed
removed
the
4 - Waiting on Reviewer
|
rerun tests |
1 similar comment
|
rerun tests |
|
Seems to have been an unrelated error in FAISS. Rerunning tests and will check on specifics. |
|
rerun tests |
|
@JohnZed, I'll be removing the auto-merge labels from all repos shortly. Please make sure to use the new merge comment, |
|
rerun tests |
|
Just updated copyright headers |
|
Merged in branch-0.18 to deal with FAISS error |
wphicks
added
the
5 - Ready to Merge
Provide mode to perform a second high-precision pass over results returned from brute-force KNN searches which make use of L2-derived metrics. This provides a workaround for issues with numerical instability in L2 distance calculations in FAISS when a query vector is quite close to multiple retrieved samples relative to the typical inter-sample distance.
Resolve #3195.