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

[MarcosPividori]

@rcurtin thanks for your comments!

The main problem that I am trying to solve here is this:
When we set a reference tree, that tree will *become part of the state* of `NeighborSearch` class, because `NeighborSearch` class will *hold a pointer* to that tree. This is the key point.
So, I think the usage of pointer or references in this case is not the best approach.
For example, we could face problems like:
```
  KNN knn;

  arma::mat dataset = arma::randu<arma::mat>(5, 100);
  std::vector<size_t> oldFromNewReferences;
  KNN::Tree* tree = new KNN::Tree(dataset, oldFromNewReferences);

  knn.Train(tree);

  delete tree;

  arma::Mat<size_t> neighbors;
  arma::mat distances;
  knn.Search(10, neighbors, distances); // Segmentation Fault.
```

So, I think that if we want to set a given reference tree in the `NeighborSearch` class, as it will become part of its state, we should provide 2 options:
 + Pass a lvalue reference, so the tree will be copied.
```
void Train(Tree& referenceTree); // Copy the reference tree.
```
 + Pass a rvalue reference, so the tree will be moved.
```
void Train(Tree&& referenceTree); // Move the reference tree.
```
In both cases the `NeighborSearch` class will take ownership of the tree that it is holding a pointer to.
It would be similar to the approach of the c++ standard library for containers.
For example, for `std::vector`:
```
void push_back (const value_type& val);  // copies the value.
void push_back (value_type&& val); // moves the value.
```

As you suggests, we could assume that in both cases, as the tree was provided by the user, the `NeighborSearch` class shouldn't take care of the mappings.

Another optional approach, that I think is not necessary, is to provide a new method:
```
void SetMappings(std::vector<size_t>& oldFromNew); // The mappings will be copied
void SetMappings(std::vector<size_t>&& oldFromNew); // The mappings will be moved.
```
So, if users want the points to be mapped, they can set the mapping.

But I think this is not necessary, assuming that if users created their own reference tree, they should care of the mappings on their own.


So, I suggest to continue with this approach:
+ Provide the methods.
```
void Train(Tree& referenceTree); // Copy the reference tree.
void Train(Tree&& referenceTree); // Move the reference tree.
```
+ Remove the boolean flag `ownsTree`, because it will be always true.
+ Add a boolean flag `considerMappings`. When the `NeighborSearch` class created the tree from a given dataset, it will set: `considerMappings = true`.
When a reference tree was provided by the user, it will set: `considerMappings = false`.

What do you think?

Thanks!

-- 
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-241228690
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mailman.cc.gatech.edu/pipermail/mlpack-git/attachments/20160820/7f1fe44c/attachment.html>