-
Notifications
You must be signed in to change notification settings - Fork 362
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
feat: support IVF_HNSW_SQ #1284
Conversation
Signed-off-by: BubbleCal <bubble-cal@outlook.com>
leave docs and python to future PRs, or it's too large |
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.
Some minor suggestions. I think we should decide on usize
/ u64
before merging (as changing that would be a breaking change) but the other suggestion about the comments we can figure out later.
rust/lancedb/src/index/vector.rs
Outdated
/// The number of neighbors to select for each vector in the HNSW graph. | ||
/// The default value is 20. | ||
pub fn m(mut self, m: usize) -> Self { | ||
self.m = m; | ||
self | ||
} | ||
|
||
/// The number of candidates to evaluate during the construction of the HNSW graph. | ||
/// The default value is 300. | ||
pub fn ef_construction(mut self, ef_construction: usize) -> Self { | ||
self.ef_construction = ef_construction; | ||
self | ||
} |
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.
We can add this in a future PR (when we extend this to node and python perhaps) but we will want these parameters to include:
- What happens if this is set incorrectly?
- When should the user adjust the default?
rust/lancedb/src/index/vector.rs
Outdated
pub(crate) m: usize, | ||
pub(crate) ef_construction: usize, |
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.
Can we use u64
instead of usize
? We can convert to usize
internally. I'd rather avoid usize
in our public API since it is slightly more confusing to think about.
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.
sure, let me change it to u32
cause we don't need such great value of u64
Signed-off-by: BubbleCal <bubble-cal@outlook.com>
No description provided.