[mlpack-git] [mlpack/mlpack] Octree (#790)

[lozhnikov]

lozhnikov commented on this pull request.

I've taken a quick look at the code. The code looks fine except two very minor issues. I'll look more thoroughly later.

> +    const VecType& point,
+    typename boost::enable_if<IsVector<VecType>>::type*) const
+{
+  return bound.RangeDistance(point);
+}
+
+//! Serialize the tree.
+template<typename MetricType, typename StatisticType, typename MatType>
+template<typename Archive>
+void Octree<MetricType, StatisticType, MatType>::Serialize(
+    Archive& ar,
+    const unsigned int /* version */)
+{
+  using data::CreateNVP;
+
+  // 

It seems you have forgotten to remove `//`:)

> +template<typename MetricType, typename StatisticType, typename MatType>
+void Octree<MetricType, StatisticType, MatType>::SplitNode(
+    const arma::vec& center,
+    const double width,
+    const size_t maxLeafSize)
+{
+  // No need to split if we have fewer than the maximum number of points in this
+  // node.
+  if (count <= maxLeafSize)
+    return;
+
+  // We must split the dataset by sequentially creating each of the children.
+  // We do this in two steps: first we make a pass to count the number of points
+  // that will fall into each child; then in the second pass we rearrange the
+  // points and create the children.
+  arma::Col<size_t> childCounts(std::pow(2, dataset->n_rows),

I think that `((size_t) 2) << dataset->n_rows` is better than `std::pow()`, how do you think?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/mlpack/mlpack/pull/790#pullrequestreview-1455123
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mailman.cc.gatech.edu/pipermail/mlpack-git/attachments/20160925/925a5ad1/attachment.html>