[mlpack-git] [mlpack/mlpack] Use move semantics to set a given reference tree in NeighborSearch class. (#765)

Ryan Curtin notifications at github.com
Tue Aug 23 17:53:53 EDT 2016

Ok, I like the approach you have proposed.  We avoid exposing the mappings problem to the user, and instead if they pass a tree that has been mapped, it is their responsibility to apply the mappings.  My only suggestion is that we make it `Train(const TreeType& tree)`, so that the user knows their tree will not be modified but instead the tree will be copied.

Long ago, before we had C++11 which gave us rvalue references, this would have been a much trickier question, but now, you are right, we can just use `Train(TreeType&& tree)` so that the user can allow the `NeighborSearch` object to take ownership of the tree.

But, there is still one use case I want to consider that this approach doesn't address... what if the user wants to create their own tree that `NeighborSearch` can use, and they don't want to copy the tree when they call `Train()`, but they then want to use that tree for something else?  Trees can be very large, so, if e.g. a user wanted to use a tree with `KNN` then with `KFN`, they should be able to do this without needing to copy the tree.

I guess it is possible to provide

TreeType&& ReferenceTree() { return std::move(*referenceTree); }

and then a user can do

TreeType getMyTreeBack = std::move(knn.ReferenceTree());

What do you think?  Is there a better way to do this?  It seems a bit odd to make the user go to weird lengths like this:

// Build custom tree and train the model.
TreeType myTree(myData);
KNN knn;

arma::mat distances;
arma::Mat<size_t> neighbors;
knn.Search(3, neighbors, distances);

// Create a KFN model too and reuse the tree.
KFN kfn;
// I'm ignoring the fact that we have to reset the statistic for now...
arma::mat kfnDistances;
arma::Mat<size_t> kfnNeighbors;
kfn.Search(3, kfnNeighbors, kfnDistances);

That seems a bit awkward, but it's consistent with the rest of the code, so I am not sure if there is any better way to do it... let me know if you have any other ideas.

I'll try and find time to do a more detailed review of the code shortly.

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mailman.cc.gatech.edu/pipermail/mlpack-git/attachments/20160823/dae387d5/attachment-0001.html>

More information about the mlpack-git mailing list