Skip to content

Remove temlate interface searchKnn #225

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

Merged
merged 5 commits into from
Dec 14, 2020
Merged

Conversation

orrorcol
Copy link
Contributor

In issue #146, i add a templae interface

template <typename Comp>
std::vector<std::pair<dist_t, labeltype>> searchKnn(const void*, size_t, Comp) {
}

But it cannot be overrided. So I removed the template and make it a virtual function.

@orrorcol orrorcol closed this Jun 27, 2020
@orrorcol orrorcol reopened this Jun 27, 2020
@orrorcol orrorcol changed the title Merge pull request #219 from nmslib/develop Remove temlate interface searchKnn Jun 27, 2020
@orrorcol
Copy link
Contributor Author

orrorcol commented Dec 2, 2020

Why is this pr not merged?

@yurymalkov
Copy link
Member

Thanks for the PR! And sorry for an extremely late response.
One thing it seems it breaks the bruteforce C++ code (we do not have tests for it). This can be easily fixed by moving the function to the interface (as it does not depend on the algorithm).
Secondly, I think having two methods with the same name, but different semantics is not a good move. I think renaming the function to explicitly represent the difference knnSearchFillCloserFirst might help with that.

@orrorcol
Copy link
Contributor Author

orrorcol commented Dec 7, 2020

Thank you for your reply. So we should only keep the following interface

virtual std::priority_queue<std::pair<dist_t, labeltype >> searchKnn(const void *, size_t) const = 0;

And add interfaces like knnSearchFillCloserFirst.
Am I right?

@yurymalkov
Copy link
Member

Yes. That would work!
It would be nice if the interface would have the same semantics (input, output types)

@orrorcol
Copy link
Contributor Author

image
@yurymalkov which interface do you prefer. One returns a vector while another returns a priority_queue.

@yurymalkov
Copy link
Member

Hi @uestc-lfs , after some thinking I agree the vector option should be actually better as it is more flexible and can be used both ways by the user.
Probably even a single order option would be enough for practical applications (CloserFirst). Please let me know what you think.

@orrorcol
Copy link
Contributor Author

Hi @uestc-lfs , after some thinking I agree the vector option should be actually better as it is more flexible and can be used both ways by the user.
Probably even a single order option would be enough for practical applications (CloserFirst). Please let me know what you think.

I agree

@orrorcol
Copy link
Contributor Author

@yurymalkov I have finished my code. How do you like it?

@yurymalkov
Copy link
Member

Great! Thank you so much! And apologies that it took so long!

@yurymalkov yurymalkov merged commit 52da3d2 into nmslib:develop Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants