IVF-Flat: make adaptive-centers behavior optional #1019
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
achirkin
added
3 - Ready for Review
feature request
|
NB: breaking change (changed default behavior and the signature of one of the constructors). |
cjnolet
reviewed
Nov 15, 2022
| index(const handle_t& handle, | ||
| raft::distance::DistanceType metric, | ||
| uint32_t n_lists, | ||
| bool adaptive_centers, |
There was a problem hiding this comment.
Rather than making this an option on the index params object itself, why not just make it an optional argument on the extend() function? That would enable the user to determine whether it should be done outside of the creation of the index. It's really not an index option, but a design detail based on a specific use-case / usage-pattern.
I was thinking of something like extend(....., update_centers=false);
There was a problem hiding this comment.
Although I see it sounds more intuitive to keep the parameter in the extend, I'd disagree for two reasons:
- The update-centers routine does not go through the whole data (which would be costly), but only aggregates newly added data. For this to work, the
centers()must always be up-to-date. Hence, we cannot allow doing an "updating"extendafter a "non-updating" one. Also from the logical point of view, this would break both alternative invariants (centers are neither constant nor up-to-date). - I hope to preserve the public API the same for all our (ANN) models:
buildgets all parameters through the struct,extendnever accepts extra parameters (and gets them from the index). Besides, thebuildfunction would need to takeupdate_centersas well, because it optionally callsextend.
cjnolet
requested changes
Nov 16, 2022
Co-authored-by: Corey J. Nolet <[email protected]>
tfeher
approved these changes
Nov 16, 2022
|
@gpucibot merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add an indexing parameter
adaptive_centers(false by default) to control whetherindex.centers()should be kept up-to-date with the cluster data or remain immutable.