<p>In <a href="https://github.com/mlpack/mlpack/pull/747#discussion_r74681976">src/mlpack/core/tree/spill_tree/single_tree_traverser.hpp</a>:</p>
<pre style='color:#555'>> +#include <mlpack/core.hpp>
> +
> +#include "spill_tree.hpp"
> +
> +namespace mlpack {
> +namespace tree {
> +
> +template<typename MetricType,
> + typename StatisticType,
> + typename MatType,
> + template<typename HyperplaneMetricType> class HyperplaneType,
> + template<typename SplitMetricType, typename SplitMatType>
> + class SplitType>
> +template<typename RuleType>
> +class SpillTree<MetricType, StatisticType, MatType, HyperplaneType, SplitType>::
> + SingleTreeTraverser
</pre>
<p>The mlpack tree traversers (single-tree and dual-tree) implement the concepts defined as "pruning single-tree traversals" and "pruning dual-tree traversals" in the tree-independent dual-tree algorithms paper. But in this case the traversals you have here don't actually satisfy that definition because they have defeatist search hard-coded into them. So if the Rules class doesn't explicitly handle that case, then the traversal is technically invalid.</p>
<p>In the scheme of things this is a really minor point, but I think that it's possible that this may confuse someone way down the line who just says "okay, I want to test my Rules class with all the trees and their default traversers" and suddenly it doesn't work for spill trees and their traversers. So, I can think of a couple quick solutions:</p>
<ul>
<li><p>Add a boolean template parameter which, if true, will do the defeatist search as you have implemented it, but it is documented that setting that template parameter to true will make the traverser technically violate the definition.</p></li>
<li><p>Add documentation stating that the traverser technically violates the definition, providing no way to get a "rigorously correct" traversal.</p></li>
</ul>
<p>I am not sure if either of those is the best solution, but I do think it is important that the defeatist search part of this traversal is implemented, otherwise it will be doing two scores when it only needs to do one in those cases. Maybe you have a better idea for how to handle this?</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/747/files/fe090ee13c7cad79e2b7eb8b6690628ba3ead1ed#r74681976">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJ4bFGRatkiRa72gJmx6DL8gQuN9C0gVks5qfVMqgaJpZM4JZzLU">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AJ4bFIbAXqrnudrs0cBW7yVqEYXmJxw-ks5qfVMqgaJpZM4JZzLU.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/747/files/fe090ee13c7cad79e2b7eb8b6690628ba3ead1ed#r74681976"></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 #747: The mlpack tree traversers (single-tree and dual-tree) implement the concepts defined as \"pruning single-tree traversals\" and \"pruning dual-tree traversals\" in the tree-independent dual-tree algorithms paper. But in this case the traversals you have here don't actually satisfy that definition because they have defeatist search hard-coded into them. So if the Rules class doesn't explicitly handle that case, then the traversal is technically invalid.\r\n\r\nIn the scheme of things this is a really minor point, but I think that it's possible that this may confuse someone way down the line who just says \"okay, I want to test my Rules class with all the trees and their default traversers\" and suddenly it doesn't work for spill trees and their traversers. So, I can think of a couple quick solutions:\r\n\r\n * Add a boolean template parameter which, if true, will do the defeatist search as you have implemented it, but it is documented that setting that template parameter to true will make the traverser technically violate the definition.\r\n\r\n * Add documentation stating that the traverser technically violates the definition, providing no way to get a \"rigorously correct\" traversal.\r\n\r\nI am not sure if either of those is the best solution, but I do think it is important that the defeatist search part of this traversal is implemented, otherwise it will be doing two scores when it only needs to do one in those cases. Maybe you have a better idea for how to handle this?"}],"action":{"name":"View Pull Request","url":"https://github.com/mlpack/mlpack/pull/747/files/fe090ee13c7cad79e2b7eb8b6690628ba3ead1ed#r74681976"}}}</script>