<p>Ok, I like the approach you have proposed.  We avoid exposing the mappings problem to the user, and instead if they pass a tree that has been mapped, it is their responsibility to apply the mappings.  My only suggestion is that we make it <code>Train(const TreeType&amp; tree)</code>, so that the user knows their tree will not be modified but instead the tree will be copied.</p>

<p>Long ago, before we had C++11 which gave us rvalue references, this would have been a much trickier question, but now, you are right, we can just use <code>Train(TreeType&amp;&amp; tree)</code> so that the user can allow the <code>NeighborSearch</code> object to take ownership of the tree.</p>

<p>But, there is still one use case I want to consider that this approach doesn't address... what if the user wants to create their own tree that <code>NeighborSearch</code> can use, and they don't want to copy the tree when they call <code>Train()</code>, but they then want to use that tree for something else?  Trees can be very large, so, if e.g. a user wanted to use a tree with <code>KNN</code> then with <code>KFN</code>, they should be able to do this without needing to copy the tree.</p>

<p>I guess it is possible to provide</p>

<p>``<br>
TreeType&amp;&amp; ReferenceTree() { return std::move(*referenceTree); }</p>

<pre><code>
and then a user can do

</code></pre>

<p>TreeType getMyTreeBack = std::move(knn.ReferenceTree());</p>

<pre><code>
What do you think?  Is there a better way to do this?  It seems a bit odd to make the user go to weird lengths like this:

</code></pre>

<p>// Build custom tree and train the model.<br>
TreeType myTree(myData);<br>
KNN knn;<br>
knn.Train(std::move(myTree));</p>

<p>arma::mat distances;<br>
arma::Mat neighbors;<br>
knn.Search(3, neighbors, distances);</p>

<p>// Create a KFN model too and reuse the tree.<br>
KFN kfn;<br>
kfn.Train(std::move(knn.ReferenceTree()));<br>
// I'm ignoring the fact that we have to reset the statistic for now...<br>
arma::mat kfnDistances;<br>
arma::Mat kfnNeighbors;<br>
kfn.Search(3, kfnNeighbors, kfnDistances);</p>

<pre><code>
That seems a bit awkward, but it's consistent with the rest of the code, so I am not sure if there is any better way to do it... let me know if you have any other ideas.

I'll try and find time to do a more detailed review of the code shortly.
</code></pre>

<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, <a href="https://github.com/mlpack/mlpack/pull/765#issuecomment-241892195">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJ4bFC-IGRG7WdZkbZpNhkJVyAJFrR7pks5qi2vxgaJpZM4JoBeZ">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AJ4bFKsHjXQ8jP5qZgyMpmmIN768xUJMks5qi2vxgaJpZM4JoBeZ.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-241892195"></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 in #765: Ok, I like the approach you have proposed.  We avoid exposing the mappings problem to the user, and instead if they pass a tree that has been mapped, it is their responsibility to apply the mappings.  My only suggestion is that we make it `Train(const TreeType\u0026 tree)`, so that the user knows their tree will not be modified but instead the tree will be copied.\r\n\r\nLong ago, before we had C++11 which gave us rvalue references, this would have been a much trickier question, but now, you are right, we can just use `Train(TreeType\u0026\u0026 tree)` so that the user can allow the `NeighborSearch` object to take ownership of the tree.\r\n\r\nBut, there is still one use case I want to consider that this approach doesn't address... what if the user wants to create their own tree that `NeighborSearch` can use, and they don't want to copy the tree when they call `Train()`, but they then want to use that tree for something else?  Trees can be very large, so, if e.g. a user wanted to use a tree with `KNN` then with `KFN`, they should be able to do this without needing to copy the tree.\r\n\r\nI guess it is possible to provide\r\n\r\n``\r\nTreeType\u0026\u0026 ReferenceTree() { return std::move(*referenceTree); }\r\n```\r\n\r\nand then a user can do\r\n\r\n```\r\nTreeType getMyTreeBack = std::move(knn.ReferenceTree());\r\n```\r\n\r\nWhat do you think?  Is there a better way to do this?  It seems a bit odd to make the user go to weird lengths like this:\r\n\r\n```\r\n// Build custom tree and train the model.\r\nTreeType myTree(myData);\r\nKNN knn;\r\nknn.Train(std::move(myTree));\r\n\r\narma::mat distances;\r\narma::Mat\u003csize_t\u003e neighbors;\r\nknn.Search(3, neighbors, distances);\r\n\r\n// Create a KFN model too and reuse the tree.\r\nKFN kfn;\r\nkfn.Train(std::move(knn.ReferenceTree()));\r\n// I'm ignoring the fact that we have to reset the statistic for now...\r\narma::mat kfnDistances;\r\narma::Mat\u003csize_t\u003e kfnNeighbors;\r\nkfn.Search(3, kfnNeighbors, kfnDistances);\r\n```\r\n\r\nThat seems a bit awkward, but it's consistent with the rest of the code, so I am not sure if there is any better way to do it... let me know if you have any other ideas.\r\n\r\nI'll try and find time to do a more detailed review of the code shortly."}],"action":{"name":"View Pull Request","url":"https://github.com/mlpack/mlpack/pull/765#issuecomment-241892195"}}}</script>