Set norm to 1.0 for all-0 vectors #803
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cydrain 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 |
|
/kind improvement |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #803 +/- ##
=========================================
+ Coverage 0 61.66% +61.66%
=========================================
Files 0 84 +84
Lines 0 6149 +6149
=========================================
+ Hits 0 3792 +3792
- Misses 0 2357 +2357 |
|
/hold |
thirdparty/faiss/faiss/IVFlib.cpp
Outdated
| @@ -508,6 +508,7 @@ void ivf_residual_add_from_flat_codes( | |||
| // ok | |||
| index->rq.decode(tmp_code.data(), tmp.data(), 1); | |||
| float norm = fvec_norm_L2sqr(tmp.data(), rq.d); | |||
| norm = (norm == 0.0 ? 1.0 : norm); | |||
There was a problem hiding this comment.
no, because it uses norm for distance = x^2 - 2xy + y^2 calculation, not for a cosine-related code
| @@ -93,6 +93,7 @@ struct AQDistanceComputerLUT : FlatCodesDistanceComputer { | |||
| bias = 0; | |||
| } else { | |||
| bias = fvec_norm_L2sqr(x, d); | |||
| bias = (bias == 0.0 ? 1.0 : bias); | |||
There was a problem hiding this comment.
no, because it uses norm for distance = x^2 - 2xy + y^2 calculation, not for a cosine-related code
| @@ -174,6 +175,7 @@ void search_with_LUT( | |||
| if (!is_IP) { // the LUT function returns ||y||^2 - 2 * <x, y>, need to | |||
| // add ||x||^2 | |||
| bias = fvec_norm_L2sqr(xq + q * d, d); | |||
| bias = (bias == 0.0 ? 1.0 : bias); | |||
There was a problem hiding this comment.
no, because it uses norm for distance = x^2 - 2xy + y^2 calculation, not for a cosine-related code
thirdparty/faiss/faiss/IndexFlat.cpp
Outdated
| @@ -354,6 +354,7 @@ struct FlatL2WithNormsDis : FlatCodesDistanceComputer { | |||
| void set_query(const float* x) override { | |||
| q = x; | |||
| query_l2norm = fvec_norm_L2sqr(q, d); | |||
| query_l2norm = (query_l2norm == 0.0 ? 1.0 : query_l2norm); | |||
There was a problem hiding this comment.
no, because it uses norm for distance = x^2 - 2xy + y^2 calculation, not for a cosine-related code
| @@ -212,6 +212,7 @@ struct AQInvertedListScannerLUT : AQInvertedListScanner { | |||
| AQInvertedListScanner::set_query(query_vector); | |||
| if (!is_IP && !ia.by_residual) { | |||
| distance_bias = fvec_norm_L2sqr(query_vector, ia.d); | |||
| distance_bias = (distance_bias == 0.0 ? 1.0 : distance_bias); | |||
There was a problem hiding this comment.
no, because it uses norm for distance = x^2 - 2xy + y^2 calculation, not for a cosine-related code
| @@ -322,7 +322,8 @@ void AdditiveQuantizer::compute_centroid_norms(float* norms) const { | |||
| #pragma omp for | |||
| for (int64_t i = 0; i < ntotal; i++) { | |||
| decode_64bit(i, tmp.data()); | |||
| norms[i] = fvec_norm_L2sqr(tmp.data(), d); | |||
| float norm = fvec_norm_L2sqr(tmp.data(), d); | |||
| norms[i] = (norm == 0.0 ? 1.0 : norm); | |||
There was a problem hiding this comment.
no, because it uses norm for distance = x^2 - 2xy + y^2 calculation, not for a cosine-related code
| @@ -65,7 +65,9 @@ void fvec_norms_L2( | |||
| size_t nx) { | |||
| #pragma omp parallel for if (nx > 10000) | |||
| for (int64_t i = 0; i < nx; i++) { | |||
| nr[i] = sqrtf(fvec_norm_L2sqr(x + i * d, d)); | |||
| auto norm = fvec_norm_L2sqr(x + i * d, d); | |||
There was a problem hiding this comment.
no, because it might use norm for distance = x^2 - 2xy + y^2 calculation, not for a cosine-related code
| nr[i] = fvec_norm_L2sqr(x + i * d, d); | ||
| for (int64_t i = 0; i < nx; i++) { | ||
| float norm = fvec_norm_L2sqr(x + i * d, d); | ||
| nr[i] = (norm == 0.0 ? 1.0 : norm); |
There was a problem hiding this comment.
no, because it might use norm for distance = x^2 - 2xy + y^2 calculation, not for a cosine-related code
| (y_norms != nullptr) ? | ||
| y_norms[j] : | ||
| sqrtf(fvec_norm_L2sqr(y + j * d, d)); | ||
|
|
||
| norm = (norm == 0.0 ? 1.0 : norm); |
| (y_norms != nullptr) ? | ||
| y_norms[idsi[j]] : | ||
| sqrtf(fvec_norm_L2sqr(y + d * idsi[j], d)); | ||
| norm = (norm == 0.0 ? 1.0 : norm); |
cydrain
force-pushed
the
caiyd_dont_norm_all_zero_vectors_sol2
branch
from
83a7cea to
ae055f2
Compare
cydrain
force-pushed
the
caiyd_dont_norm_all_zero_vectors_sol2
branch
2 times, most recently
from
a3be7c2 to
13416c7
Compare
Signed-off-by: Cai Yudong <[email protected]>
cydrain
force-pushed
the
caiyd_dont_norm_all_zero_vectors_sol2
branch
from
13416c7 to
e328437
Compare
|
/lgtm |
|
@cydrain does it solve milvus-io/milvus#35594 ? |
|
/unhold |
Yes, milvus#35594 will be fixed with this PR |
Issue: milvus-io/milvus#35594