Skip to content
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

fix the issue caused by passing in invalid vectors #361

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

inabao
Copy link
Collaborator

@inabao inabao commented Jan 22, 2025

fix: #360

@inabao inabao added kind/bug Something isn't working version/0.13 labels Jan 22, 2025
@inabao inabao requested a review from LHT129 January 22, 2025 08:19
@inabao inabao self-assigned this Jan 22, 2025
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

@@            Coverage Diff             @@
##             main     #361      +/-   ##
==========================================
+ Coverage   90.94%   91.03%   +0.09%     
==========================================
  Files         133      133              
  Lines        8459     8459              
==========================================
+ Hits         7693     7701       +8     
+ Misses        766      758       -8     
Flag Coverage Δ
cpp 91.03% <ø> (+0.09%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
common 95.75% <ø> (ø)
datacell 91.77% <ø> (ø)
index 91.18% <ø> (+0.17%) ⬆️
simd 81.72% <ø> (+0.09%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b9ddac...dcfca3c. Read the comment docs.

@inabao inabao force-pushed the add-tests-for-nan-vector branch from a58eeda to 83fbed5 Compare January 22, 2025 09:59
extern/diskann/DiskANN/src/pq.cpp Outdated Show resolved Hide resolved
tests/fixtures/test_dataset.cpp Outdated Show resolved Hide resolved
tests/test_index.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@LHT129 LHT129 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jinjiabao.jjb added 2 commits January 22, 2025 19:23
Signed-off-by: jinjiabao.jjb <[email protected]>
@inabao inabao force-pushed the add-tests-for-nan-vector branch from 83fbed5 to dcfca3c Compare January 22, 2025 11:23
Copy link
Collaborator

@jiaweizone jiaweizone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

for (int i = 0; i < sample_size; ++i)
{
auto norm = get_norm(train_data + i * train_dim, train_dim);
if (std::abs(norm) < 1e-6 || std::isnan(norm)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, add validate check

Copy link
Collaborator

@wxyucs wxyucs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@wxyucs wxyucs merged commit 797d7f4 into main Jan 23, 2025
16 checks passed
@wxyucs wxyucs deleted the add-tests-for-nan-vector branch January 23, 2025 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/L version/0.13
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zero vector causes crash on building diskann index with cosine metric
4 participants