[mlpack] [mlpack/mlpack] using rvalue references to set a given reference tree in FastMKS and DualTreeBoruvka (#799)

[Ryan Curtin]

rcurtin commented on this pull request.

Hi Anuraj,

Thanks for the hard work with this PR.  There are only a few small issues remaining.  When you get these fixed, I'll run valgrind to ensure there are no memory leaks or anything and then I think it is fine to merge.

> @@ -141,6 +184,29 @@ class FastMKS
    * @param tree Tree to use as reference data.

To mark a method deprecated, also add `mlpack_deprecated` to the function definition please.  i.e. `mlpack_deprecated void Train(...)`.

>     * @param tree Tree to use as reference data.
    */
   void Train(Tree* referenceTree);
+  
+  /**
+   * Train the FastMKS model on the given reference tree.  This takes ownership
+   * of the tree, so you do not need to delete it!  This will throw an exception
+   * if the model is searching in naive mode (i.e. if Naive() == true).
+   *
+   * This method will copy the given tree. You can avoid this copy by using the
+   * Train() method that takes a rvalue reference to the tree.
+   *
+   * @param tree Tree to use as reference data.
+   */
+  void Train(Tree& referenceTree);

We can make this `const Tree&` since we are copying the tree.  Also, the comment here is inaccurate---this method does not take ownership of the tree, it copies it.  This comment also applies to other places with arguments like `Tree&` that can be changed to `const Tree&`.

> +   * point with maximum kernel evaluation to point 4 in the query set will be
+   * stored in row 0 and column 4 of the indices matrix.
+   *
+   * This will throw an exception if called while the FastMKS object has
+   * 'single' set to true.
+   *
+   * Be aware that if your tree modifies the original input matrix, the results
+   * here are with respect to the modified input matrix (that is,
+   * queryTree->Dataset()).
+   *
+   * @param queryTree Tree built on query points.
+   * @param k The number of maximum kernels to find.
+   * @param indices Matrix to store resulting indices of max-kernel search in.
+   * @param kernels Matrix to store resulting max-kernel values in.
+   */
+  void Search(Tree& querySet,

Parameter should be called `queryTree` not `querySet`.

> @@ -301,6 +372,20 @@ void FastMKS<KernelType, MatType, TreeType>::Search(
     arma::Mat<size_t>& indices,
     arma::mat& kernels)
 {
+	Search(*queryTree, k, indices, kernels);

The spacing is off here, the call to `Search()` should be indented only two spaces from the opening brackets.

> @@ -420,7 +505,7 @@ void FastMKS<KernelType, MatType, TreeType>::Search(
   // Dual-tree implementation.
   Timer::Stop("computing_products");
 
-  Search(referenceTree, k, indices, kernels);
+  Search(std::move(referenceTree), k, indices, kernels);

There's no need for a move here, this will destroy the model's tree after the call.  Passing `referenceTree` as a `const Tree&` is just fine.

-- 
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/799#pullrequestreview-14182873
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://knife.lugatgt.org/pipermail/mlpack/attachments/20161222/b1afd38c/attachment.html>