-
Notifications
You must be signed in to change notification settings - Fork 33
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
Update faiss version in both Dockerfiles and any required API updates #251
Conversation
Target CPP Coverage: 64.1637% Target Python Coverage: 98.02% |
Target CPP Coverage: 64.1609% Target Python Coverage: 98.02% |
Target CPP Coverage: 64.1609% Target Python Coverage: 98.02% |
Target CPP Coverage: 64.1609% Target Python Coverage: 98.02% |
Target CPP Coverage: 64.1609% Target Python Coverage: 98.02% |
Target CPP Coverage: 64.1609% Target Python Coverage: 98.02% |
Target CPP Coverage: 64.1609% Target Python Coverage: 98.02% |
Target CPP Coverage: 64.1609% Target Python Coverage: 98.02% |
Target CPP Coverage: 64.1609% Target Python Coverage: 98.02% |
@s-gobriel, code changes look good to me. You can mark as |
Target CPP Coverage: 64.1609% Target Python Coverage: 98.02% |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing leaping out at me, only one comment is some magic numbers we might want to take care of, but I dont think that should block merging given this was hanging around a bit on accident.
@@ -391,6 +391,9 @@ FaissHNSWFlatDescriptorSet::FaissHNSWFlatDescriptorSet( | |||
if (metric == L2) { | |||
_index = new faiss::IndexHNSWFlat(dim, hnsw_M, faiss::METRIC_L2); | |||
((faiss::IndexHNSWFlat *)_index)->hnsw.efConstruction = 96; | |||
} else if (metric == IP) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic Numbers, not going to block review but do we want to flag for moving to named var/constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed in #263
Closes #222
Updating Faiss from v1.7.4 to v1.9.0
Add any necessary API updates