make IdVal more generic #505
Conversation
Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zhengbuqian The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| #include <iostream> | ||
|
|
||
| #include "knowhere/file_manager.h" | ||
|
|
||
| namespace knowhere { | ||
|
|
||
| template <typename I, typename T> |
There was a problem hiding this comment.
I tried to put this in utils.h but for unknown reason it just won't compile 🧐
include/knowhere/object.h
Outdated
| } | ||
| inline friend bool | ||
| operator>(const IdVal<I, T>& lhs, const IdVal<I, T>& rhs) { | ||
| return !(lhs < rhs); |
There was a problem hiding this comment.
!(lhs < rhs) implies lhs >= rhs, not lhs > rhs
There was a problem hiding this comment.
good catch!
also added a commit to replace std::pair<float, int64_t> in PrecomputedDistanceIterator with IdVal
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #505 +/- ##
=========================================
+ Coverage 0 72.36% +72.36%
=========================================
Files 0 63 +63
Lines 0 4291 +4291
=========================================
+ Hits 0 3105 +3105
- Misses 0 1186 +1186 |
Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>
buqian-zilliz
force-pushed
the
update-idval
branch
2 times, most recently
from
fd8fd24
to
1923e93
Compare
| @@ -234,7 +236,7 @@ void all_inner_product( | |||
| size_t d, | |||
| size_t nx, | |||
| size_t ny, | |||
| std::vector<std::pair<float, int64_t>>& output, | |||
| std::vector<knowhere::DistId>& output, | |||
There was a problem hiding this comment.
Is it possible to avoid modifying Faiss in this case?
There was a problem hiding this comment.
yes, but we have to then choose between:
PrecomputedDistanceIteratorand bruteforce iterator to still usestd::vector<std::pair<float, int64_t>>PrecomputedDistanceIteratorto useDistIdbut its constructor to copy/convert the argumentstd::vector<std::pair<float, int64_t>>tostd::vector<DistId>
also those modified all_* functions and CollectAllResultHandler are not from the original faiss, they are added by us when adding the bruteforce iterator, so it's not that bad.
wdyt?
There was a problem hiding this comment.
It adds more pain to me whenever I need to propagate the changes from the Faiss baseline. So, I expect and insist that the changes to the faiss core code to be the last resort.
But it should be acceptable if the functions are not from the original faiss.
Could you please check whether you'll be able to put your change on top of #453 and see whether they are compatible? Just check, don't rebase yet :) Thanks
There was a problem hiding this comment.
I just tried rebasing a copy of iterator-new-interface onto merge_faiss_changes_from_baseline_v2a and yes they are compatible. The only conflict is CollectAllResultHandler which is easy to fix.
| @@ -283,7 +285,7 @@ void all_L2sqr( | |||
| size_t d, | |||
| size_t nx, | |||
| size_t ny, | |||
| std::vector<std::pair<float, int64_t>>& output, | |||
| std::vector<knowhere::DistId>& output, | |||
| @@ -305,7 +307,7 @@ void all_cosine( | |||
| size_t d, | |||
| size_t nx, | |||
| size_t ny, | |||
| std::vector<std::pair<float, int64_t>>& output, | |||
| std::vector<knowhere::DistId>& output, | |||
buqian-zilliz
force-pushed
the
update-idval
branch
from
1923e93
to
89266af
Compare
…arator Signed-off-by: Buqian Zheng <zhengbuqian@gmail.com>
buqian-zilliz
force-pushed
the
update-idval
branch
from
89266af
to
198a91e
Compare
|
/lgtm |
/kind improvement