<p>I'm really sorry this took so long.  The end of the GSoC application period was pretty overwhelming and then I went on vacation for a week, so only now am I able to take a look at this in-depth.</p>

<p>Here is some background based on what I remember of this during GSoC 2014.  The rectangle tree and its variants are made for dynamic point sets where insertions and deletions are common, but this is a little bit at odds with the other trees in mlpack, which tend to be "build it once and don't insert or delete points from the dataset".  In order to support this design decision, the original idea was to have each node hold its own arma::mat (this is called <code>localDataset</code>) and then points could easily be inserted or deleted by just calling <code>insert_cols()</code> or <code>shed_cols()</code> on the local datasets.  But I am not sure this ever really happened in earnest, so I think maybe the <code>Insert()</code> and <code>Delete()</code> functions do not work correctly.  This is a thing I would eventually like to look into, but have not yet had time to.  I don't think the local dataset is even used; instead I think each node holds a <code>std::vector&lt;size_t&gt;</code>
  that holds the indices of the points that are in that node.  If you are interested in solving that problem and making the Insert()/Delete() support meaningful, I can discuss it more if you like.</p>

<p>The other issue was the necessity for a shallow copy constructor, because sometimes the insertion procedure necessitates creating a new root node.  But if we are constructing the tree like</p>

<pre><code>RTreeType* tree = new RTreeType(dataset);
</code></pre>

<p>we can't have <code>tree</code> point to an intermediate level of the tree; it must point to the root.  So we need to make a shallow copy of the current <code>tree</code> node, then make that shallow copy a child of the current <code>tree</code> node and update all the data in the <code>tree</code> node.</p>

<p>I think you are right that you uncovered a problem there, and I think that your solution in <a href="https://github.com/mlpack/mlpack/commit/121bbafe14eba62873f20271ea59612b7b5bcae9" class="commit-link"><tt>121bbaf</tt></a> is the right solution.  The situation where <code>deepCopy = true</code> is the default copy constructor behavior, and would be used if you wanted to copy the tree entirely:</p>

<pre><code>RTreeType myTree(dataset);
RTreeType myTreeCopy(myTree /* defaults to a deep copy */);
</code></pre>

<p>For the second issue, the <code>const MatType* dataset</code> only makes sense if the tree should not be modifying the dataset.  We should just remove the non-const function <code>Dataset()</code>, to be honest.</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">&mdash;<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly or <a href="https://github.com/mlpack/mlpack/pull/556#issuecomment-205622329">view it on GitHub</a><img alt="" height="1" src="https://github.com/notifications/beacon/AJ4bFN26aWKK6jBh_rCjmU45m-9DkuZPks5p0dSagaJpZM4HsSai.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/556#issuecomment-205622329"></link>
  <meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>