Extend the test coverage of FaissIVFIndex #13300
Conversation
|
This pull request was exported from Phabricator. Differential Revision: D68233815 |
|
|
||
| if (std::from_chars(key.data(), key.data() + key.size(), id).ec != | ||
| std::errc()) { | ||
| return -1; |
There was a problem hiding this comment.
nit: wondering if we would want to use a different value for this case
There was a problem hiding this comment.
Our database vectors are numbered from 0 to 4095, so I think -1 works for signaling error for the purposes of the test, right?
There was a problem hiding this comment.
Ah sorry, I should have elaborated here little bit. What I meant is the following
The default value we set for ids vector below is -1. std::vector<faiss::idx_t> ids(neighbors, -1);
If ith query search in FAISS gets nothing. But for whatever reason, we somehow have a key in the DB for ith iteration, but std::from_chars() failed for that key to get the id.
In that case ASSERT_EQ(get_id(), ids[num_found]); will not throw. (This test would still fail for num_found comparison in this case I guess)
This is going to be a rare or nearly impossible scenario, but just wanted to explain what I meant by my nit comment.
There was a problem hiding this comment.
That case would have been caught by ASSERT_EQ(num_found, num_found_cmp); with the earlier code but I added a couple more assertions to make this clearer
Summary: The patch adds a new unit test for `FaissIVFIndex` that compares its results with a regular in-memory FAISS index. Specifically, it trains two identical IVF indices using the same training vectors, passes the ownership of one to `FaissIVFIndex`, adds the same set of database vectors to both, and then queries them using the same query vectors (with a variety of values for number of neighbors and number of probes). Reviewed By: jaykorean Differential Revision: D68233815
ltamasi
force-pushed
the
export-D68233815
branch
from
e74c44c to
995aecc
Compare
|
This pull request was exported from Phabricator. Differential Revision: D68233815 |
Summary: The patch adds a new unit test for `FaissIVFIndex` that compares its results with a regular in-memory FAISS index. Specifically, it trains two identical IVF indices using the same training vectors, passes the ownership of one to `FaissIVFIndex`, adds the same set of database vectors to both, and then queries them using the same query vectors (with a variety of values for number of neighbors and number of probes). Reviewed By: jaykorean Differential Revision: D68233815
ltamasi
force-pushed
the
export-D68233815
branch
from
995aecc to
9392978
Compare
|
This pull request was exported from Phabricator. Differential Revision: D68233815 |
|
This pull request has been merged in 77d4663. |
Summary: The patch adds a new unit test for
FaissIVFIndexthat compares its results with a regular in-memory FAISS index. Specifically, it trains two identical IVF indices using the same training vectors, passes the ownership of one toFaissIVFIndex, adds the same set of database vectors to both, and then queries them using the same query vectors (with a variety of values for number of neighbors and number of probes).Differential Revision: D68233815