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

[Ryan Curtin]

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;
knn.Train(std::move(myTree));

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

// Create a KFN model too and reuse the tree.
KFN kfn;
kfn.Train(std::move(knn.ReferenceTree()));
// 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:
https://github.com/mlpack/mlpack/pull/765#issuecomment-241892195
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mailman.cc.gatech.edu/pipermail/mlpack-git/attachments/20160823/dae387d5/attachment-0001.html>