<p>Aside from my single comment on the code, this looks great and I agree it is ready to merge. I think maybe I want to get 3.0.0 done soon since we are creating a lot of ugly reverse compatibility code! :)</p>

<p>One concern I do have is that right now the <code>BinarySpaceTree::GetNearestChild()</code> and <code>BinarySpaceTree::GetFurthestChild()</code> functions loop over both children and perform two distance calculations. We can optimize this by calculating which half space the point falls into. Should we wait until the refactoring for random projection trees is done until doing that? If so I am happy to merge now, I just want to make sure that happens eventually.</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, <a href="https://github.com/mlpack/mlpack/pull/762#issuecomment-241017179">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJ4bFFnBeyUlI7iDS09PS1Qbwmi1pIHTks5qha6KgaJpZM4JlKcS">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AJ4bFIgkgs4wovcbcHU0HE_3qqejiCY6ks5qha6KgaJpZM4JlKcS.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/762#issuecomment-241017179"></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 #762: Aside from my single comment on the code, this looks great and I agree it is ready to merge. I think maybe I want to get 3.0.0 done soon since we are creating a lot of ugly reverse compatibility code! :)\r\n\r\nOne concern I do have is that right now the `BinarySpaceTree::GetNearestChild()` and `BinarySpaceTree::GetFurthestChild()` functions loop over both children and perform two distance calculations. We can optimize this by calculating which half space the point falls into. Should we wait until the refactoring for random projection trees is done until doing that? If so I am happy to merge now, I just want to make sure that happens eventually."}],"action":{"name":"View Pull Request","url":"https://github.com/mlpack/mlpack/pull/762#issuecomment-241017179"}}}</script>