<p><b>@rcurtin</b> requested changes on this pull request.</p>
<p>The changes look good, thank you for taking the time to do this. But there are still memory leaks (you can see that the tests crash when Travis runs) that we need to address. Also, there are two more things:</p>
<ul>
<li>
<p>We need to test new functions that we have added. These include:</p>
<ul>
<li><code>DualTreeBoruvka<>::DualTreeBoruvka(Tree&)</code></li>
<li><code>DualTreeBoruvka<>::DualTreeBoruvka(Tree&&)</code></li>
<li><code>FastMKS<>::FastMKS(Tree&)</code></li>
<li><code>FastMKS<>::FastMKS(Tree&&)</code></li>
<li><code>FastMKS<>::Train(Tree&&)</code></li>
</ul>
</li>
</ul>
<p>You can take a look at how similar functions are tested for <code>NeighborSearch</code> in <code>knn_test.cpp</code>, for instance.</p>
<ul>
<li>You should also update the tests <code>fastmks_test.cpp</code> and <code>emst_test.cpp</code> to use the non-deprecated API. This will reduce a lot of warnings.</li>
</ul><hr>
<p>In <a href="https://github.com/mlpack/mlpack/pull/799#pullrequestreview-3902975">src/mlpack/methods/emst/dtb.hpp</a>:</p>
<pre style='color:#555'>> @@ -142,6 +142,8 @@ class DualTreeBoruvka
* naive computation, construct a tree with all the points in one leaf (i.e.
* leafSize = number of points).
*
+ * Deprecated.
+ *
</pre>
<p>If we add a comment like this, we should also point a user towards what new method they should be using.</p>
<hr>
<p>In <a href="https://github.com/mlpack/mlpack/pull/799#pullrequestreview-3902975">src/mlpack/methods/emst/dtb.hpp</a>:</p>
<pre style='color:#555'>> @@ -190,8 +238,17 @@ class DualTreeBoruvka
/**
* This function resets the values in the nodes of the tree nearest neighbor
* distance, and checks for fully connected nodes.
+ *
+ * Deprecated.
</pre>
<p>This is an internal function so we don't need to preserve reverse compatibility and we can just change the signature. So you can go ahead and remove this.</p>
<hr>
<p>In <a href="https://github.com/mlpack/mlpack/pull/799#pullrequestreview-3902975">src/mlpack/methods/fastmks/fastmks.hpp</a>:</p>
<pre style='color:#555'>> @@ -105,13 +105,56 @@ class FastMKS
* can be specified. Brute-force search is not available with this
* constructor since a tree is given (use one of the other constructors).
*
+ * This method won't take ownership of the given tree. There is no copying of
+ * the data matrices in this constructor (because tree-building is not
+ * necessary), so this is the constructor to use when copies absolutely must
+ * be avoided.
+ *
+ * Deprecated.
</pre>
<p>The same comment about pointing users towards the correct function applies here also.</p>
<hr>
<p>In <a href="https://github.com/mlpack/mlpack/pull/799#pullrequestreview-3902975">src/mlpack/methods/fastmks/fastmks.hpp</a>:</p>
<pre style='color:#555'>> @@ -141,6 +184,29 @@ class FastMKS
* @param tree Tree to use as reference data.
</pre>
<p>We should mark this function deprecated.</p>
<hr>
<p>In <a href="https://github.com/mlpack/mlpack/pull/799#pullrequestreview-3902975">src/mlpack/methods/fastmks/fastmks.hpp</a>:</p>
<pre style='color:#555'>> @@ -184,6 +250,8 @@ class FastMKS
* here are with respect to the modified input matrix (that is,
* queryTree->Dataset()).
*
+ * Deprecated.
+ *
</pre>
<p>The same comment about pointing users towards the correct function applies here also.</p>
<hr>
<p>In <a href="https://github.com/mlpack/mlpack/pull/799#pullrequestreview-3902975">src/mlpack/methods/fastmks/fastmks_impl.hpp</a>:</p>
<pre style='color:#555'>> @@ -107,6 +107,44 @@ FastMKS<KernelType, MatType, TreeType>::FastMKS(Tree* referenceTree,
// Nothing to do.
}
+// One dataset, pre-built tree.
+template<typename KernelType,
+ typename MatType,
+ template<typename TreeMetricType,
+ typename TreeStatType,
+ typename TreeMatType> class TreeType>
+FastMKS<KernelType, MatType, TreeType>::FastMKS(Tree& referenceTree,
+ const bool singleMode) :
+ referenceSet(&this->referenceTree->Dataset()),
</pre>
<p>We can't set <code>referenceSet</code> until the tree is set. At the time of initialization here, <code>this->referenceTree</code> is not set, so we are setting <code>referenceSet</code> to something invalid. This is probably the cause of your invalid memory access in the tests.</p>
<hr>
<p>In <a href="https://github.com/mlpack/mlpack/pull/799#pullrequestreview-3902975">src/mlpack/methods/fastmks/fastmks_impl.hpp</a>:</p>
<pre style='color:#555'>> @@ -185,10 +223,60 @@ void FastMKS<KernelType, MatType, TreeType>::Train(Tree* tree)
this->metric = metric::IPMetric<KernelType>(tree->Metric().Kernel());
this->setOwner = false;
- if (treeOwner && referenceTree)
- delete referenceTree;
+ if (treeOwner && this->referenceTree)
+ delete this->referenceTree;
</pre>
<p>The <code>this-></code> here is unnecessary. Actually it looks like even the <code>this-></code> above this if statement are unnecessary too.</p>
<hr>
<p>In <a href="https://github.com/mlpack/mlpack/pull/799#pullrequestreview-3902975">src/mlpack/methods/fastmks/fastmks_impl.hpp</a>:</p>
<pre style='color:#555'>> +
+template<typename KernelType,
+ typename MatType,
+ template<typename TreeMetricType,
+ typename TreeStatType,
+ typename TreeMatType> class TreeType>
+void FastMKS<KernelType, MatType, TreeType>::Train(Tree& tree)
+{
+ if (naive)
+ throw std::invalid_argument("cannot call FastMKS::Train() with a tree when "
+ "in naive search mode");
+
+ if (setOwner)
+ delete this->referenceSet;
+
+ this->referenceSet = &this->tree->Dataset();
</pre>
<p>What is <code>&this->tree</code>? This probably does not compile... consider updating <code>Train(Tree*)</code> to simply call this method so that when you test <code>Train(Tree*)</code>, this is called too. Also, the heavy use of <code>this-></code> in this function is not necessary.</p>
<hr>
<p>In <a href="https://github.com/mlpack/mlpack/pull/799#pullrequestreview-3902975">src/mlpack/methods/fastmks/fastmks_impl.hpp</a>:</p>
<pre style='color:#555'>> +void FastMKS<KernelType, MatType, TreeType>::Train(Tree&& tree)
+{
+ if (naive)
+ throw std::invalid_argument("cannot call FastMKS::Train() with a tree when "
+ "in naive search mode");
+
+ if (setOwner)
+ delete this->referenceSet;
+
+ this->referenceTree = new Tree(std::move(tree));
+ this->referenceSet = &this->referenceTree->Dataset();
+ this->metric = metric::IPMetric<KernelType>(tree.Metric().Kernel());
+ this->setOwner = false;
+
+ if (treeOwner && this->referenceTree)
+ delete this->referenceTree;
</pre>
<p>This will delete the tree after it is moved!</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/mlpack/mlpack/pull/799#pullrequestreview-3902975">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJ4bFOEzuGtgyPSzlStsWYZlWM8085wYks5qzQW0gaJpZM4KPMkZ">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AJ4bFOc_FsPa8_7kcX5akLctaFo7eTr2ks5qzQW0gaJpZM4KPMkZ.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
<link itemprop="url" href="https://github.com/mlpack/mlpack/pull/799#pullrequestreview-3902975"></link>
<meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/mlpack/mlpack","title":"mlpack/mlpack","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/mlpack/mlpack"}},"updates":{"snippets":[{"icon":"PERSON","message":"@rcurtin requested changes on #799"}],"action":{"name":"View Pull Request","url":"https://github.com/mlpack/mlpack/pull/799#pullrequestreview-3902975"}}}</script>