From a1a2dae175b77f72c46486f3121cd4eab2ddfacc Mon Sep 17 00:00:00 2001 From: piyush-jaiswal Date: Sun, 4 Dec 2016 00:41:23 +0530 Subject: [PATCH 1/4] Copy, move constructors and copy assignment operator added --- src/mlpack/methods/det/dtree.hpp | 16 +++++ src/mlpack/methods/det/dtree_impl.hpp | 94 +++++++++++++++++++++++++++ 2 files changed, 110 insertions(+) diff --git a/src/mlpack/methods/det/dtree.hpp b/src/mlpack/methods/det/dtree.hpp index d910af7167e..36930098256 100644 --- a/src/mlpack/methods/det/dtree.hpp +++ b/src/mlpack/methods/det/dtree.hpp @@ -58,6 +58,22 @@ class DTree */ DTree(); + /** + * Create a copied density estimation tree. + */ + DTree(const DTree& toBeCopied); + + /** + * Move a density estimation tree into another. + */ + DTree(const DTree&& toBeMoved); + + /** + * Move a density estimation tree into another using copy assignment operator. + */ + DTree& operator=(const DTree& toBeCopied); + + /** * Create a density estimation tree with the given bounds and the given number * of total points. Children will not be created. diff --git a/src/mlpack/methods/det/dtree_impl.hpp b/src/mlpack/methods/det/dtree_impl.hpp index fae801d623d..0bc9235bb6b 100644 --- a/src/mlpack/methods/det/dtree_impl.hpp +++ b/src/mlpack/methods/det/dtree_impl.hpp @@ -165,6 +165,100 @@ DTree::DTree() : right(NULL) { /* Nothing to do. */ } +//Copy constructor +template +DTree::DTree(const DTree& toBeCopied) : + start(toBeCopied.start), + end(toBeCopied.end), + splitDim(toBeCopied.splitDim), + splitValue(toBeCopied.splitValue), + logNegError(toBeCopied.logNegError), + subtreeLeavesLogNegError(toBeCopied.subtreeLeavesLogNegError), + subtreeLeaves(toBeCopied.subtreeLeaves), + root(toBeCopied.root), + ratio(toBeCopied.ratio), + logVolume(toBeCopied.logVolume), + bucketTag(toBeCopied.bucketTag), + alphaUpper(toBeCopied.alphaUpper), + left(toBeCopied.left), + right(toBeCopied.right) +{ /* Nothing to do. */ } + +//Move constructor +template +DTree::DTree(const DTree&& toBeMoved) : + start(toBeMoved.start), + end(toBeMoved.end), + splitDim(toBeMoved.splitDim), + splitValue(toBeMoved.splitValue), + logNegError(toBeMoved.logNegError), + subtreeLeavesLogNegError(toBeMoved.subtreeLeavesLogNegError), + subtreeLeaves(toBeMoved.subtreeLeaves), + root(toBeMoved.root), + ratio(toBeMoved.ratio), + logVolume(toBeMoved.logVolume), + bucketTag(toBeMoved.bucketTag), + alphaUpper(toBeMoved.alphaUpper), + left(toBeMoved.left), + right(toBeMoved.right) +{ + toBeMoved.start = 0; + toBeMoved.end = 0; + toBeMoved.splitDim = size_t(-1); + toBeMoved.splitValue = std::numeric_limits::max(); + toBeMoved.logNegError = -DBL_MAX; + toBeMoved.subtreeLeavesLogNegError = -DBL_MAX; + toBeMoved.subtreeLeaves = 0; + toBeMoved.root = true; + toBeMoved.ratio = 1.0; + toBeMoved.logVolume = -DBL_MAX; + toBeMoved.bucketTag = -1; + toBeMoved.alphaUpper = 0.0; + toBeMoved.left = NULL; + toBeMoved.right = NULL; +} + +//Copy Assignment operator +template +DTree& DTree::operator=(const DTree& toBeCopied) +{ + if(this != &toBeCopied) + { + //Freeing the existing resource + delete[] start; + delete[] end; + delete[] splitDim; + delete[] splitValue; + delete[] logNegError; + delete[] subtreeLeavesLogNegError; + delete[] subtreeLeaves; + delete[] root; + delete[] ratio; + delete[] logVolume; + delete[] bucketTag; + delete[] alphaUpper; + delete[] left; + delete[] right; + + //Copy the data from source to destination + start = toBeCopied.start; + end = toBeCopied.end; + splitDim = toBeCopied.splitDim; + splitValue = toBeCopied.splitValue; + logNegError = toBeCopied.logNegError; + subtreeLeavesLogNegError = toBeCopied.subtreeLeavesLogNegError; + subtreeLeaves = toBeCopied.subtreeLeaves; + root = toBeCopied.root; + ratio = toBeCopied.ratio; + logVolume = toBeCopied.logVolume; + bucketTag = toBeCopied.bucketTag; + alphaUpper = toBeCopied.alphaUpper; + left = toBeCopied.left; + right = toBeCopied.right; + } + + return *this; +} // Root node initializers From 51209c6ffc51856d16594b256321f7cace4940a5 Mon Sep 17 00:00:00 2001 From: piyush-jaiswal Date: Tue, 6 Dec 2016 19:32:45 +0530 Subject: [PATCH 2/4] Updated maxVal, minVal values in the constructors and wrote test cases for the constructors --- src/mlpack/methods/det/dtree.hpp | 6 +-- src/mlpack/methods/det/dtree_impl.hpp | 35 +++---------- src/mlpack/tests/det_test.cpp | 72 +++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 30 deletions(-) diff --git a/src/mlpack/methods/det/dtree.hpp b/src/mlpack/methods/det/dtree.hpp index 36930098256..984b90358d4 100644 --- a/src/mlpack/methods/det/dtree.hpp +++ b/src/mlpack/methods/det/dtree.hpp @@ -52,7 +52,7 @@ class DTree typedef typename MatType::elem_type ElemType; typedef typename MatType::vec_type VecType; typedef typename arma::Col StatType; - + /** * Create an empty density estimation tree. */ @@ -66,7 +66,7 @@ class DTree /** * Move a density estimation tree into another. */ - DTree(const DTree&& toBeMoved); + DTree(DTree&& toBeMoved); /** * Move a density estimation tree into another using copy assignment operator. @@ -95,7 +95,7 @@ class DTree * @param data Dataset to build tree on. */ DTree(MatType& data); - + /** * Create a child node of a density estimation tree given the bounding box * specified by maxVals and minVals, using the size given in start and end and diff --git a/src/mlpack/methods/det/dtree_impl.hpp b/src/mlpack/methods/det/dtree_impl.hpp index 0bc9235bb6b..461baca80c0 100644 --- a/src/mlpack/methods/det/dtree_impl.hpp +++ b/src/mlpack/methods/det/dtree_impl.hpp @@ -170,6 +170,8 @@ template DTree::DTree(const DTree& toBeCopied) : start(toBeCopied.start), end(toBeCopied.end), + maxVals(toBeCopied.maxVals), + minVals(toBeCopied.minVals), splitDim(toBeCopied.splitDim), splitValue(toBeCopied.splitValue), logNegError(toBeCopied.logNegError), @@ -186,9 +188,11 @@ DTree::DTree(const DTree& toBeCopied) : //Move constructor template -DTree::DTree(const DTree&& toBeMoved) : +DTree::DTree(DTree&& toBeMoved) : start(toBeMoved.start), end(toBeMoved.end), + maxVals(toBeMoved.maxVals), + minVals(toBeMoved.minVals), splitDim(toBeMoved.splitDim), splitValue(toBeMoved.splitValue), logNegError(toBeMoved.logNegError), @@ -202,20 +206,7 @@ DTree::DTree(const DTree&& toBeMoved) : left(toBeMoved.left), right(toBeMoved.right) { - toBeMoved.start = 0; - toBeMoved.end = 0; - toBeMoved.splitDim = size_t(-1); - toBeMoved.splitValue = std::numeric_limits::max(); - toBeMoved.logNegError = -DBL_MAX; - toBeMoved.subtreeLeavesLogNegError = -DBL_MAX; - toBeMoved.subtreeLeaves = 0; - toBeMoved.root = true; - toBeMoved.ratio = 1.0; - toBeMoved.logVolume = -DBL_MAX; - toBeMoved.bucketTag = -1; - toBeMoved.alphaUpper = 0.0; - toBeMoved.left = NULL; - toBeMoved.right = NULL; + toBeMoved = DTree(); } //Copy Assignment operator @@ -225,24 +216,14 @@ DTree& DTree::operator=(const DTree& toBeCop if(this != &toBeCopied) { //Freeing the existing resource - delete[] start; - delete[] end; - delete[] splitDim; - delete[] splitValue; - delete[] logNegError; - delete[] subtreeLeavesLogNegError; - delete[] subtreeLeaves; - delete[] root; - delete[] ratio; - delete[] logVolume; - delete[] bucketTag; - delete[] alphaUpper; delete[] left; delete[] right; //Copy the data from source to destination start = toBeCopied.start; end = toBeCopied.end; + maxVals = toBeCopied.maxVals; + minVals = toBeCopied.minVals; splitDim = toBeCopied.splitDim; splitValue = toBeCopied.splitValue; logNegError = toBeCopied.logNegError; diff --git a/src/mlpack/tests/det_test.cpp b/src/mlpack/tests/det_test.cpp index 5bba5aa4751..3ff2c508f82 100644 --- a/src/mlpack/tests/det_test.cpp +++ b/src/mlpack/tests/det_test.cpp @@ -447,6 +447,78 @@ BOOST_AUTO_TEST_CASE(TestSparseComputeValue) BOOST_REQUIRE_CLOSE(0.0, testDTree.ComputeValue(q4), 1e-10); } +// Test the copy constructor and the copy operator. +BOOST_AUTO_TEST_CASE(CopyConstructorAndOperatorTest) +{ + arma::Mat testData(3, 5); + + testData << 4 << 5 << 7 << 3 << 5 << arma::endr + << 5 << 0 << 1 << 7 << 1 << arma::endr + << 5 << 6 << 7 << 1 << 8 << arma::endr; + + DTree> tree(testData); + + // Copy the genarated DTree + DTree> tree2(tree); + DTree> tree3 = tree; + + double max0, max1, max2, min0, min1, min2; + + // Computing the max and min vals + max0 = tree.maxVals[0]; + max1 = tree.maxVals[1]; + max2 = tree.maxVals[2]; + min0 = tree.minVals[0]; + min1 = tree.minVals[1]; + min2 = tree.minVals[2]; + + // Comparing with the DTree constructed using copy constructor + BOOST_REQUIRE_EQUAL(tree2.maxVals[0], max0); + BOOST_REQUIRE_EQUAL(tree2.maxVals[1], max1); + BOOST_REQUIRE_EQUAL(tree2.maxVals[2], max2); + BOOST_REQUIRE_EQUAL(tree2.minVals[0], min0); + BOOST_REQUIRE_EQUAL(tree2.minVals[1], min1); + BOOST_REQUIRE_EQUAL(tree2.minVals[2], min2); + + // Comparing with the DTree constructed using copy operator + BOOST_REQUIRE_EQUAL(tree3.maxVals[0], max0); + BOOST_REQUIRE_EQUAL(tree3.maxVals[1], max1); + BOOST_REQUIRE_EQUAL(tree3.maxVals[2], max2); + BOOST_REQUIRE_EQUAL(tree3.minVals[0], min0); + BOOST_REQUIRE_EQUAL(tree3.minVals[1], min1); + BOOST_REQUIRE_EQUAL(tree3.minVals[2], min2); +} + +// Test the move operator. +BOOST_AUTO_TEST_CASE(MoveConstructorTest) +{ + arma::Mat testData(3, 5); + + testData << 4 << 5 << 7 << 3 << 5 << arma::endr + << 5 << 0 << 1 << 7 << 1 << arma::endr + << 5 << 6 << 7 << 1 << 8 << arma::endr; + + DTree> tree(testData); + + double max0, max1, max2, min0, min1, min2, logNegError; + + // Computing the max and min val and the logNegError that has been calculated + max0 = tree.maxVals[0]; + max1 = tree.maxVals[1]; + max2 = tree.maxVals[2]; + min0 = tree.minVals[0]; + min1 = tree.minVals[1]; + min2 = tree.minVals[2]; + logNegError = tree.logNegError; + + DTree> tree2 = std::move(tree); + + BOOST_REQUIRE_EQUAL(tree2.logNegError, logNegError); + + // Checking for the default values in tree after move is performed + BOOST_REQUIRE_EQUAL(tree.logNegError, -DBL_MAX); +} + /** * These are not yet implemented. * From c92b7f3b5a2a18d1cba5f7e6ea15722793c8876d Mon Sep 17 00:00:00 2001 From: piyush-jaiswal Date: Wed, 14 Dec 2016 03:12:31 +0530 Subject: [PATCH 3/4] Constructed DTrees using 'new' operator in the constructor tests --- src/mlpack/tests/det_test.cpp | 52 +++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/src/mlpack/tests/det_test.cpp b/src/mlpack/tests/det_test.cpp index 3ff2c508f82..ece755c17f8 100644 --- a/src/mlpack/tests/det_test.cpp +++ b/src/mlpack/tests/det_test.cpp @@ -456,21 +456,23 @@ BOOST_AUTO_TEST_CASE(CopyConstructorAndOperatorTest) << 5 << 0 << 1 << 7 << 1 << arma::endr << 5 << 6 << 7 << 1 << 8 << arma::endr; - DTree> tree(testData); + DTree>* tree = new DTree>(testData); // Copy the genarated DTree - DTree> tree2(tree); - DTree> tree3 = tree; + DTree> tree2(*tree); + DTree> tree3 = *tree; double max0, max1, max2, min0, min1, min2; // Computing the max and min vals - max0 = tree.maxVals[0]; - max1 = tree.maxVals[1]; - max2 = tree.maxVals[2]; - min0 = tree.minVals[0]; - min1 = tree.minVals[1]; - min2 = tree.minVals[2]; + max0 = tree->maxVals[0]; + max1 = tree->maxVals[1]; + max2 = tree->maxVals[2]; + min0 = tree->minVals[0]; + min1 = tree->minVals[1]; + min2 = tree->minVals[2]; + + delete tree; // Comparing with the DTree constructed using copy constructor BOOST_REQUIRE_EQUAL(tree2.maxVals[0], max0); @@ -498,25 +500,33 @@ BOOST_AUTO_TEST_CASE(MoveConstructorTest) << 5 << 0 << 1 << 7 << 1 << arma::endr << 5 << 6 << 7 << 1 << 8 << arma::endr; - DTree> tree(testData); + DTree>* tree = new DTree>(testData); double max0, max1, max2, min0, min1, min2, logNegError; // Computing the max and min val and the logNegError that has been calculated - max0 = tree.maxVals[0]; - max1 = tree.maxVals[1]; - max2 = tree.maxVals[2]; - min0 = tree.minVals[0]; - min1 = tree.minVals[1]; - min2 = tree.minVals[2]; - logNegError = tree.logNegError; - - DTree> tree2 = std::move(tree); + max0 = tree->maxVals[0]; + max1 = tree->maxVals[1]; + max2 = tree->maxVals[2]; + min0 = tree->minVals[0]; + min1 = tree->minVals[1]; + min2 = tree->minVals[2]; + logNegError = tree->logNegError; - BOOST_REQUIRE_EQUAL(tree2.logNegError, logNegError); + DTree> tree2 = std::move(*tree); // Checking for the default values in tree after move is performed - BOOST_REQUIRE_EQUAL(tree.logNegError, -DBL_MAX); + BOOST_REQUIRE_EQUAL(tree->logNegError, -DBL_MAX); + + delete tree; + + BOOST_REQUIRE_EQUAL(tree2.maxVals[0], max0); + BOOST_REQUIRE_EQUAL(tree2.maxVals[1], max1); + BOOST_REQUIRE_EQUAL(tree2.maxVals[2], max2); + BOOST_REQUIRE_EQUAL(tree2.minVals[0], min0); + BOOST_REQUIRE_EQUAL(tree2.minVals[1], min1); + BOOST_REQUIRE_EQUAL(tree2.minVals[2], min2); + BOOST_REQUIRE_EQUAL(tree2.logNegError, logNegError); } /** From 99b2b02e5092461ddc0d76f4aeb3fd34da61ee07 Mon Sep 17 00:00:00 2001 From: piyush-jaiswal Date: Thu, 29 Dec 2016 02:31:05 +0530 Subject: [PATCH 4/4] fixed bug of copying the left and right of a DTree --- src/mlpack/methods/det/dtree_impl.hpp | 58 +++++++++++++-------------- 1 file changed, 27 insertions(+), 31 deletions(-) diff --git a/src/mlpack/methods/det/dtree_impl.hpp b/src/mlpack/methods/det/dtree_impl.hpp index 461baca80c0..a323cf4e91e 100644 --- a/src/mlpack/methods/det/dtree_impl.hpp +++ b/src/mlpack/methods/det/dtree_impl.hpp @@ -181,10 +181,11 @@ DTree::DTree(const DTree& toBeCopied) : ratio(toBeCopied.ratio), logVolume(toBeCopied.logVolume), bucketTag(toBeCopied.bucketTag), - alphaUpper(toBeCopied.alphaUpper), - left(toBeCopied.left), - right(toBeCopied.right) -{ /* Nothing to do. */ } + alphaUpper(toBeCopied.alphaUpper) +{ + left = new DTree(*toBeCopied.left); + right = new DTree(*toBeCopied.right); +} //Move constructor template @@ -202,10 +203,11 @@ DTree::DTree(DTree&& toBeMoved) : ratio(toBeMoved.ratio), logVolume(toBeMoved.logVolume), bucketTag(toBeMoved.bucketTag), - alphaUpper(toBeMoved.alphaUpper), - left(toBeMoved.left), - right(toBeMoved.right) + alphaUpper(toBeMoved.alphaUpper) { + left = std::move(toBeMoved.left); + right = std::move(toBeMoved.right); + toBeMoved = DTree(); } @@ -213,30 +215,24 @@ DTree::DTree(DTree&& toBeMoved) : template DTree& DTree::operator=(const DTree& toBeCopied) { - if(this != &toBeCopied) - { - //Freeing the existing resource - delete[] left; - delete[] right; - - //Copy the data from source to destination - start = toBeCopied.start; - end = toBeCopied.end; - maxVals = toBeCopied.maxVals; - minVals = toBeCopied.minVals; - splitDim = toBeCopied.splitDim; - splitValue = toBeCopied.splitValue; - logNegError = toBeCopied.logNegError; - subtreeLeavesLogNegError = toBeCopied.subtreeLeavesLogNegError; - subtreeLeaves = toBeCopied.subtreeLeaves; - root = toBeCopied.root; - ratio = toBeCopied.ratio; - logVolume = toBeCopied.logVolume; - bucketTag = toBeCopied.bucketTag; - alphaUpper = toBeCopied.alphaUpper; - left = toBeCopied.left; - right = toBeCopied.right; - } + left = new DTree(*toBeCopied.left); + right = new DTree(*toBeCopied.right); + + //Copy the data from source to destination + start = toBeCopied.start; + end = toBeCopied.end; + maxVals = toBeCopied.maxVals; + minVals = toBeCopied.minVals; + splitDim = toBeCopied.splitDim; + splitValue = toBeCopied.splitValue; + logNegError = toBeCopied.logNegError; + subtreeLeavesLogNegError = toBeCopied.subtreeLeavesLogNegError; + subtreeLeaves = toBeCopied.subtreeLeaves; + root = toBeCopied.root; + ratio = toBeCopied.ratio; + logVolume = toBeCopied.logVolume; + bucketTag = toBeCopied.bucketTag; + alphaUpper = toBeCopied.alphaUpper; return *this; }