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

Move mlpack_allknn and mlpack_allkfn, but preserve old names #532

Closed
rcurtin opened this issue Mar 1, 2016 · 8 comments · Fixed by #618
Closed

Move mlpack_allknn and mlpack_allkfn, but preserve old names #532

rcurtin opened this issue Mar 1, 2016 · 8 comments · Fixed by #618

Comments

@rcurtin
Copy link
Member

rcurtin commented Mar 1, 2016

The name mlpack_allknn is misleading: "allknn" means "all k-nearest-neighbors", but that implies that the query and reference sets are the same. But that is not necessarily true. So mlpack_knn would be a more apt name. The same is true for mlpack_allkfn and mlpack_kfn.

These should be renamed in the CMake configuration, but also we want to preserve the old names as per https://github.com/mlpack/mlpack/blob/master/UPDATING.txt , so that a user can still use mlpack_allknn and mlpack_allkfn if their scripts depend on it. (We can remove this backwards compatibility in either 2.1.0 or 3.0.0, I am not sure which yet.)

On Linux or UNIX-like systems, this is probably done easiest with a symlink, but on Windows I don't think this is possible so it may be necessary to make copies of the programs to preserve this support.

A patch here should be a modification to the project's CMake configuration and to the documentation throughout the project (i.e. the tutorials, the programs themselves, anywhere that references 'allknn' or 'allkfn').

@ibrahim5253
Copy link

I would like to work on this.

@rcurtin
Copy link
Member Author

rcurtin commented Mar 2, 2016

Sure, go ahead. It'd probably be a good idea to assign the ticket to yourself while.you work on it. :)

@ibrahim5253
Copy link

Sorry but I didn't get you. Assign a ticket? That can be naive but I am a newbie. Apologies.

@rcurtin
Copy link
Member Author

rcurtin commented Mar 2, 2016

Oh, maybe that is not an available option for you. At least in my interface, I can set the labels and milestone and "assignee" of the ticket, but I guess maybe this is only because I'm a repo admin. Anyway, don't worry about that then, sorry for the confusion.

@ibrahim5253
Copy link

I have modified 'src/mlpack/methods/neighbor_search/CMakeLists.txt' as required but am not sure how to accomplish backward compatibility. I am on Linux. Can you please guide me in the right direction?

@rcurtin
Copy link
Member Author

rcurtin commented Mar 2, 2016

Read the CMake documentation and find out whether or not it is possible to create a symlink when on systems that support symlinks, and then do that. If that isn't possible, then copying the executable is the best option.

The CMake documentation is quite comprehensive:
https://cmake.org/cmake/help/v3.5/
https://cmake.org/cmake/help/v3.5/manual/cmake-commands.7.html

@theaverageguy
Copy link

By copying the executable you mean? Could you give an example?

@rcurtin
Copy link
Member Author

rcurtin commented Mar 24, 2016

I mean that you should make a copy of the executable with a different name. Read the CMake documentation I linked to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants