<p><a href="https://github.com/rcurtin" class="user-mention">@rcurtin</a> thanks for your comments!</p>
<p>The main problem that I am trying to solve here is this:<br>
When we set a reference tree, that tree will <em>become part of the state</em> of <code>NeighborSearch</code> class, because <code>NeighborSearch</code> class will <em>hold a pointer</em> to that tree. This is the key point.<br>
So, I think the usage of pointer or references in this case is not the best approach.<br>
For example, we could face problems like:</p>
<pre><code> 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.
</code></pre>
<p>So, I think that if we want to set a given reference tree in the <code>NeighborSearch</code> class, as it will become part of its state, we should provide 2 options:</p>
<ul>
<li>Pass a lvalue reference, so the tree will be copied.</li>
</ul>
<pre><code>void Train(Tree& referenceTree); // Copy the reference tree.
</code></pre>
<ul>
<li>Pass a rvalue reference, so the tree will be moved.</li>
</ul>
<pre><code>void Train(Tree&& referenceTree); // Move the reference tree.
</code></pre>
<p>In both cases the <code>NeighborSearch</code> class will take ownership of the tree that it is holding a pointer to.<br>
It would be similar to the approach of the c++ standard library for containers.<br>
For example, for <code>std::vector</code>:</p>
<pre><code>void push_back (const value_type& val); // copies the value.
void push_back (value_type&& val); // moves the value.
</code></pre>
<p>As you suggests, we could assume that in both cases, as the tree was provided by the user, the <code>NeighborSearch</code> class shouldn't take care of the mappings.</p>
<p>Another optional approach, that I think is not necessary, is to provide a new method:</p>
<pre><code>void SetMappings(std::vector<size_t>& oldFromNew); // The mappings will be copied
void SetMappings(std::vector<size_t>&& oldFromNew); // The mappings will be moved.
</code></pre>
<p>So, if users want the points to be mapped, they can set the mapping.</p>
<p>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.</p>
<p>So, I suggest to continue with this approach:</p>
<ul>
<li>Provide the methods.</li>
</ul>
<pre><code>void Train(Tree& referenceTree); // Copy the reference tree.
void Train(Tree&& referenceTree); // Move the reference tree.
</code></pre>
<ul>
<li>Remove the boolean flag <code>ownsTree</code>, because it will be always true.</li>
<li>Add a boolean flag <code>considerMappings</code>. When the <code>NeighborSearch</code> class created the tree from a given dataset, it will set: <code>considerMappings = true</code>.
When a reference tree was provided by the user, it will set: <code>considerMappings = false</code>.</li>
</ul>
<p>What do you think?</p>
<p>Thanks!</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/765#issuecomment-241228690">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJ4bFJ6TzU25CUGHvCu9SblG-L5EBzpVks5qh4V1gaJpZM4JoBeZ">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AJ4bFMHEq6utQMcP6TT8D5cwE4Ocv8buks5qh4V1gaJpZM4JoBeZ.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/765#issuecomment-241228690"></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":"@MarcosPividori in #765: @rcurtin thanks for your comments!\r\n\r\nThe main problem that I am trying to solve here is this:\r\nWhen 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.\r\nSo, I think the usage of pointer or references in this case is not the best approach.\r\nFor example, we could face problems like:\r\n```\r\n KNN knn;\r\n\r\n arma::mat dataset = arma::randu\u003carma::mat\u003e(5, 100);\r\n std::vector\u003csize_t\u003e oldFromNewReferences;\r\n KNN::Tree* tree = new KNN::Tree(dataset, oldFromNewReferences);\r\n\r\n knn.Train(tree);\r\n\r\n delete tree;\r\n\r\n arma::Mat\u003csize_t\u003e neighbors;\r\n arma::mat distances;\r\n knn.Search(10, neighbors, distances); // Segmentation Fault.\r\n```\r\n\r\nSo, 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:\r\n + Pass a lvalue reference, so the tree will be copied.\r\n```\r\nvoid Train(Tree\u0026 referenceTree); // Copy the reference tree.\r\n```\r\n + Pass a rvalue reference, so the tree will be moved.\r\n```\r\nvoid Train(Tree\u0026\u0026 referenceTree); // Move the reference tree.\r\n```\r\nIn both cases the `NeighborSearch` class will take ownership of the tree that it is holding a pointer to.\r\nIt would be similar to the approach of the c++ standard library for containers.\r\nFor example, for `std::vector`:\r\n```\r\nvoid push_back (const value_type\u0026 val); // copies the value.\r\nvoid push_back (value_type\u0026\u0026 val); // moves the value.\r\n```\r\n\r\nAs 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.\r\n\r\nAnother optional approach, that I think is not necessary, is to provide a new method:\r\n```\r\nvoid SetMappings(std::vector\u003csize_t\u003e\u0026 oldFromNew); // The mappings will be copied\r\nvoid SetMappings(std::vector\u003csize_t\u003e\u0026\u0026 oldFromNew); // The mappings will be moved.\r\n```\r\nSo, if users want the points to be mapped, they can set the mapping.\r\n\r\nBut I think this is not necessary, assuming that if users created their own reference tree, they should care of the mappings on their own.\r\n\r\n\r\nSo, I suggest to continue with this approach:\r\n+ Provide the methods.\r\n```\r\nvoid Train(Tree\u0026 referenceTree); // Copy the reference tree.\r\nvoid Train(Tree\u0026\u0026 referenceTree); // Move the reference tree.\r\n```\r\n+ Remove the boolean flag `ownsTree`, because it will be always true.\r\n+ Add a boolean flag `considerMappings`. When the `NeighborSearch` class created the tree from a given dataset, it will set: `considerMappings = true`.\r\nWhen a reference tree was provided by the user, it will set: `considerMappings = false`.\r\n\r\nWhat do you think?\r\n\r\nThanks!"}],"action":{"name":"View Pull Request","url":"https://github.com/mlpack/mlpack/pull/765#issuecomment-241228690"}}}</script>