From f0f947dfff21047c48b6064f1f08b429b10855fa Mon Sep 17 00:00:00 2001 From: himanshupathak21061998 Date: Tue, 25 Aug 2020 23:43:40 +0530 Subject: [PATCH 1/7] Adding copy constructor in linear layer --- src/mlpack/methods/ann/ffn_impl.hpp | 5 ++ src/mlpack/methods/ann/layer/linear.hpp | 6 ++ src/mlpack/methods/ann/layer/linear_impl.hpp | 28 ++++++ src/mlpack/tests/feedforward_network_test.cpp | 88 +++++++++++++++++++ 4 files changed, 127 insertions(+) diff --git a/src/mlpack/methods/ann/ffn_impl.hpp b/src/mlpack/methods/ann/ffn_impl.hpp index e11b624cc8..2e75f8a29a 100644 --- a/src/mlpack/methods/ann/ffn_impl.hpp +++ b/src/mlpack/methods/ann/ffn_impl.hpp @@ -642,6 +642,11 @@ FFN::FFN( this->network.push_back(boost::apply_visitor(copyVisitor, network.network[i])); } + + for (size_t i = 0; i < this->network.size(); ++i) + { + boost::apply_visitor(resetVisitor, this->network[i]); + } }; template::Linear( weights.set_size(outSize * inSize + outSize, 1); } +template +Linear::Linear( + const Linear& layer) : + inSize(layer.inSize), + outSize(layer.outSize), + weights(layer.weights), + regularizer(layer.regularizer) +{ + // Nothing to do here. +} + +template +Linear& +Linear:: +operator = (const Linear& layer) +{ + if (this != &layer) + { + inSize = layer.inSize; + outSize = layer.outSize; + weights = layer.weights; + regularizer = layer.regularizer; + } + return *this; +} + template void Linear::Reset() diff --git a/src/mlpack/tests/feedforward_network_test.cpp b/src/mlpack/tests/feedforward_network_test.cpp index f9a338e0b3..be3ed051e6 100644 --- a/src/mlpack/tests/feedforward_network_test.cpp +++ b/src/mlpack/tests/feedforward_network_test.cpp @@ -57,6 +57,93 @@ void TestNetwork(ModelType& model, REQUIRE(classificationError <= classificationErrorThreshold); } +// network1 should be allocated with `new`, and trained on some data. +template +void CheckCopyFunction(ModelType* network1, + MatType& trainData, + MatType& trainLabels, + MatType& testData, + MatType& testLabels, + const size_t maxEpochs, + const double classificationErrorThreshold) +{ + ens::RMSProp opt(0.01, 32, 0.88, 1e-8, maxEpochs * trainData.n_cols, -1); + network1->Train(trainData, trainLabels, opt); + + MatType predictionTemp; + network1->Predict(testData, predictionTemp); + MatType prediction = arma::zeros(1, predictionTemp.n_cols); + + for (size_t i = 0; i < predictionTemp.n_cols; ++i) + { + prediction(i) = arma::as_scalar(arma::find( + arma::max(predictionTemp.col(i)) == predictionTemp.col(i), 1)) + 1; + } + + FFN<> network2(*network1); + arma::mat predictions1; + network1->Predict(trainData, predictions1); + delete network1; + + // Deallocating all of network1's memory, so that + // if network2 is trying to use any of that memory. + arma::mat predictions2; + network2.Predict(trainData, predictions2); + CheckMatrices(predictions1, predictions2); +} + + + +/** + * Check whether copying Vanila network is working or not. + */ +BOOST_AUTO_TEST_CASE(CheckCopyVanillaNetworkTest) +{ + // Load the dataset. + arma::mat trainData; + data::Load("thyroid_train.csv", trainData, true); + + arma::mat trainLabels = trainData.row(trainData.n_rows - 1); + trainData.shed_row(trainData.n_rows - 1); + + arma::mat testData; + data::Load("thyroid_test.csv", testData, true); + + arma::mat testLabels = testData.row(testData.n_rows - 1); + testData.shed_row(testData.n_rows - 1); + + /* + * Construct a feed forward network with trainData.n_rows input nodes, + * hiddenLayerSize hidden nodes and trainLabels.n_rows output nodes. The + * network structure looks like: + * + * Input Hidden Output + * Layer Layer Layer + * +-----+ +-----+ +-----+ + * | | | | | | + * | +------>| +------>| | + * | | +>| | +>| | + * +-----+ | +--+--+ | +-----+ + * | | + * Bias | Bias | + * Layer | Layer | + * +-----+ | +-----+ | + * | | | | | | + * | +-----+ | +-----+ + * | | | | + * +-----+ +-----+ + */ + + FFN > *model = new FFN >; + model->Add >(trainData.n_rows, 8); + model->Add >(); + model->Add >(8, 3); + model->Add >(); + + // Check whether copy is working or not. + CheckCopyFunction<>(model, trainData, trainLabels, testData, testLabels, 10, 0.1); +} + /** * Train the vanilla network on a larger dataset. */ @@ -103,6 +190,7 @@ TEST_CASE("FFVanillaNetworkTest", "[FeedForwardNetworkTest]") model.Add >(8, 3); model.Add >(); + // Vanilla neural net with logistic activation function. // Because 92% of the patients are not hyperthyroid the neural // network must be significant better than 92%. From caff0fcf81ed2f993f3d5cbafbe84b2089f4a0ce Mon Sep 17 00:00:00 2001 From: himanshupathak21061998 Date: Sun, 30 Aug 2020 21:47:36 +0530 Subject: [PATCH 2/7] Serializing weights --- src/mlpack/methods/ann/layer/linear_impl.hpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/mlpack/methods/ann/layer/linear_impl.hpp b/src/mlpack/methods/ann/layer/linear_impl.hpp index 51b54cfec2..089b66df58 100644 --- a/src/mlpack/methods/ann/layer/linear_impl.hpp +++ b/src/mlpack/methods/ann/layer/linear_impl.hpp @@ -120,11 +120,7 @@ void Linear::serialize( { ar & BOOST_SERIALIZATION_NVP(inSize); ar & BOOST_SERIALIZATION_NVP(outSize); - - // This is inefficient, but we have to allocate this memory so that - // WeightSetVisitor gets the right size. - if (Archive::is_loading::value) - weights.set_size(outSize * inSize + outSize, 1); + ar & BOOST_SERIALIZATION_NVP(weights); } } // namespace ann From 7e71420b48d6f724c377e00a934a7a0bf8e9dc07 Mon Sep 17 00:00:00 2001 From: himanshupathak21061998 Date: Wed, 2 Sep 2020 12:19:27 +0530 Subject: [PATCH 3/7] Adding some suggestions --- src/mlpack/methods/ann/ffn_impl.hpp | 4 +- src/mlpack/methods/ann/layer/linear.hpp | 8 +++- src/mlpack/methods/ann/layer/linear_impl.hpp | 38 ++++++++++++++++--- src/mlpack/tests/feedforward_network_test.cpp | 3 +- 4 files changed, 43 insertions(+), 10 deletions(-) diff --git a/src/mlpack/methods/ann/ffn_impl.hpp b/src/mlpack/methods/ann/ffn_impl.hpp index 2e75f8a29a..468a1e56d1 100644 --- a/src/mlpack/methods/ann/ffn_impl.hpp +++ b/src/mlpack/methods/ann/ffn_impl.hpp @@ -643,9 +643,9 @@ FFN::FFN( network.network[i])); } - for (size_t i = 0; i < this->network.size(); ++i) + for (const auto& net:this->network) { - boost::apply_visitor(resetVisitor, this->network[i]); + boost::apply_visitor(resetVisitor, net); } }; diff --git a/src/mlpack/methods/ann/layer/linear.hpp b/src/mlpack/methods/ann/layer/linear.hpp index 3752d85ebb..1930181654 100644 --- a/src/mlpack/methods/ann/layer/linear.hpp +++ b/src/mlpack/methods/ann/layer/linear.hpp @@ -55,8 +55,14 @@ class Linear //! Copy constructor. Linear(const Linear& layer); + //! Move constructor. + Linear(Linear&&); + //! Copy assignment operator. - Linear& operator = (const Linear& layer); + Linear& operator=(const Linear& layer); + + //! Move assignment operator. + Linear& operator=(Linear&& layer); /* * Reset the layer parameter. diff --git a/src/mlpack/methods/ann/layer/linear_impl.hpp b/src/mlpack/methods/ann/layer/linear_impl.hpp index 089b66df58..1abf159cde 100644 --- a/src/mlpack/methods/ann/layer/linear_impl.hpp +++ b/src/mlpack/methods/ann/layer/linear_impl.hpp @@ -53,18 +53,46 @@ Linear::Linear( // Nothing to do here. } +template +Linear::Linear( + Linear&& layer) : + inSize(layer.inSize), + outSize(layer.outSize), + weights(std::move(layer.weights)), + regularizer(std::move(layer.regularizer)) +{ + // Nothing to do here. +} + template Linear& Linear:: -operator = (const Linear& layer) +operator=(const Linear& layer) { if (this != &layer) { - inSize = layer.inSize; - outSize = layer.outSize; - weights = layer.weights; - regularizer = layer.regularizer; + inSize = layer.inSize; + outSize = layer.outSize; + weights = layer.weights; + regularizer = layer.regularizer; + } + return *this; +} + +template +Linear& +Linear:: +operator=(Linear&& layer) +{ + if (this != &layer) + { + inSize = layer.inSize; + outSize = layer.outSize; + weights = std::move(layer.weights); + regularizer = std::move(layer.regularizer); } return *this; } diff --git a/src/mlpack/tests/feedforward_network_test.cpp b/src/mlpack/tests/feedforward_network_test.cpp index be3ed051e6..729267dec7 100644 --- a/src/mlpack/tests/feedforward_network_test.cpp +++ b/src/mlpack/tests/feedforward_network_test.cpp @@ -145,7 +145,7 @@ BOOST_AUTO_TEST_CASE(CheckCopyVanillaNetworkTest) } /** - * Train the vanilla network on a larger dataset. + * Train the vanila network on a larger dataset. */ TEST_CASE("FFVanillaNetworkTest", "[FeedForwardNetworkTest]") { @@ -190,7 +190,6 @@ TEST_CASE("FFVanillaNetworkTest", "[FeedForwardNetworkTest]") model.Add >(8, 3); model.Add >(); - // Vanilla neural net with logistic activation function. // Because 92% of the patients are not hyperthyroid the neural // network must be significant better than 92%. From 5c72c057fc613e85c5178bd8772d38e16e56ccd4 Mon Sep 17 00:00:00 2001 From: himanshupathak21061998 Date: Thu, 3 Sep 2020 22:14:02 +0530 Subject: [PATCH 4/7] Changing test to catch --- src/mlpack/tests/feedforward_network_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mlpack/tests/feedforward_network_test.cpp b/src/mlpack/tests/feedforward_network_test.cpp index 729267dec7..240f87b339 100644 --- a/src/mlpack/tests/feedforward_network_test.cpp +++ b/src/mlpack/tests/feedforward_network_test.cpp @@ -97,7 +97,7 @@ void CheckCopyFunction(ModelType* network1, /** * Check whether copying Vanila network is working or not. */ -BOOST_AUTO_TEST_CASE(CheckCopyVanillaNetworkTest) +TEST_CASE("CheckCopyVanillaNetworkTest", "[FeedForwardNetworkTest]") { // Load the dataset. arma::mat trainData; @@ -145,7 +145,7 @@ BOOST_AUTO_TEST_CASE(CheckCopyVanillaNetworkTest) } /** - * Train the vanila network on a larger dataset. + * Train the vanilla network on a larger dataset. */ TEST_CASE("FFVanillaNetworkTest", "[FeedForwardNetworkTest]") { From fedad5ee706aa4fd069bf1b16315b9d0859600ac Mon Sep 17 00:00:00 2001 From: himanshupathak21061998 Date: Sun, 13 Sep 2020 20:27:34 +0530 Subject: [PATCH 5/7] Adding some suggestions [skip ci] --- src/mlpack/methods/ann/ffn_impl.hpp | 6 +----- src/mlpack/methods/ann/layer/linear_impl.hpp | 4 ++-- src/mlpack/tests/feedforward_network_test.cpp | 14 +++++++++----- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/mlpack/methods/ann/ffn_impl.hpp b/src/mlpack/methods/ann/ffn_impl.hpp index 468a1e56d1..703e67d459 100644 --- a/src/mlpack/methods/ann/ffn_impl.hpp +++ b/src/mlpack/methods/ann/ffn_impl.hpp @@ -641,11 +641,7 @@ FFN::FFN( { this->network.push_back(boost::apply_visitor(copyVisitor, network.network[i])); - } - - for (const auto& net:this->network) - { - boost::apply_visitor(resetVisitor, net); + boost::apply_visitor(resetVisitor, this->network.back()); } }; diff --git a/src/mlpack/methods/ann/layer/linear_impl.hpp b/src/mlpack/methods/ann/layer/linear_impl.hpp index 1abf159cde..10ae0e5988 100644 --- a/src/mlpack/methods/ann/layer/linear_impl.hpp +++ b/src/mlpack/methods/ann/layer/linear_impl.hpp @@ -57,8 +57,8 @@ template Linear::Linear( Linear&& layer) : - inSize(layer.inSize), - outSize(layer.outSize), + inSize(0), + outSize(0), weights(std::move(layer.weights)), regularizer(std::move(layer.regularizer)) { diff --git a/src/mlpack/tests/feedforward_network_test.cpp b/src/mlpack/tests/feedforward_network_test.cpp index 240f87b339..477cd95e5b 100644 --- a/src/mlpack/tests/feedforward_network_test.cpp +++ b/src/mlpack/tests/feedforward_network_test.cpp @@ -59,7 +59,7 @@ void TestNetwork(ModelType& model, // network1 should be allocated with `new`, and trained on some data. template -void CheckCopyFunction(ModelType* network1, +void CheckCopyMoveFunction(ModelType* network1, MatType& trainData, MatType& trainLabels, MatType& testData, @@ -80,20 +80,23 @@ void CheckCopyFunction(ModelType* network1, arma::max(predictionTemp.col(i)) == predictionTemp.col(i), 1)) + 1; } - FFN<> network2(*network1); arma::mat predictions1; network1->Predict(trainData, predictions1); + FFN<> network3; + network3 = *network1; + FFN<> network2(std::move(*network1)); delete network1; // Deallocating all of network1's memory, so that // if network2 is trying to use any of that memory. arma::mat predictions2; network2.Predict(trainData, predictions2); + arma::mat predictions3; + network3.Predict(trainData, predictions3); CheckMatrices(predictions1, predictions2); + CheckMatrices(predictions1, predictions3); } - - /** * Check whether copying Vanila network is working or not. */ @@ -141,7 +144,8 @@ TEST_CASE("CheckCopyVanillaNetworkTest", "[FeedForwardNetworkTest]") model->Add >(); // Check whether copy is working or not. - CheckCopyFunction<>(model, trainData, trainLabels, testData, testLabels, 10, 0.1); + CheckCopyMoveFunction<>(model, + trainData, trainLabels, testData, testLabels, 1, 0.1); } /** From e744625ef0110d57823ab3a1448bfbab4f7725b5 Mon Sep 17 00:00:00 2001 From: Himanshu Pathak Date: Sun, 13 Sep 2020 20:44:40 +0530 Subject: [PATCH 6/7] Apply suggestions from code review Co-authored-by: Ryan Curtin --- src/mlpack/methods/ann/layer/linear_impl.hpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/mlpack/methods/ann/layer/linear_impl.hpp b/src/mlpack/methods/ann/layer/linear_impl.hpp index 10ae0e5988..79799fea98 100644 --- a/src/mlpack/methods/ann/layer/linear_impl.hpp +++ b/src/mlpack/methods/ann/layer/linear_impl.hpp @@ -44,7 +44,7 @@ Linear::Linear( template Linear::Linear( - const Linear& layer) : + const Linear& layer) : inSize(layer.inSize), outSize(layer.outSize), weights(layer.weights), @@ -56,7 +56,7 @@ Linear::Linear( template Linear::Linear( - Linear&& layer) : + Linear&& layer) : inSize(0), outSize(0), weights(std::move(layer.weights)), @@ -73,10 +73,10 @@ operator=(const Linear& layer) { if (this != &layer) { - inSize = layer.inSize; - outSize = layer.outSize; - weights = layer.weights; - regularizer = layer.regularizer; + inSize = layer.inSize; + outSize = layer.outSize; + weights = layer.weights; + regularizer = layer.regularizer; } return *this; } @@ -89,10 +89,10 @@ operator=(Linear&& layer) { if (this != &layer) { - inSize = layer.inSize; - outSize = layer.outSize; - weights = std::move(layer.weights); - regularizer = std::move(layer.regularizer); + inSize = layer.inSize; + outSize = layer.outSize; + weights = std::move(layer.weights); + regularizer = std::move(layer.regularizer); } return *this; } From 903bcc0e0a37d090bfcda0e7736a16635cc48ed8 Mon Sep 17 00:00:00 2001 From: himanshupathak21061998 Date: Tue, 15 Sep 2020 00:42:31 +0530 Subject: [PATCH 7/7] Adding move function --- src/mlpack/tests/feedforward_network_test.cpp | 65 ++++++++++--------- 1 file changed, 36 insertions(+), 29 deletions(-) diff --git a/src/mlpack/tests/feedforward_network_test.cpp b/src/mlpack/tests/feedforward_network_test.cpp index 477cd95e5b..ba45afd29d 100644 --- a/src/mlpack/tests/feedforward_network_test.cpp +++ b/src/mlpack/tests/feedforward_network_test.cpp @@ -59,31 +59,39 @@ void TestNetwork(ModelType& model, // network1 should be allocated with `new`, and trained on some data. template -void CheckCopyMoveFunction(ModelType* network1, +void CheckCopyFunction(ModelType* network1, MatType& trainData, MatType& trainLabels, - MatType& testData, - MatType& testLabels, - const size_t maxEpochs, - const double classificationErrorThreshold) + const size_t maxEpochs) { ens::RMSProp opt(0.01, 32, 0.88, 1e-8, maxEpochs * trainData.n_cols, -1); network1->Train(trainData, trainLabels, opt); - MatType predictionTemp; - network1->Predict(testData, predictionTemp); - MatType prediction = arma::zeros(1, predictionTemp.n_cols); + arma::mat predictions1; + network1->Predict(trainData, predictions1); + FFN<> network2; + network2 = *network1; + delete network1; - for (size_t i = 0; i < predictionTemp.n_cols; ++i) - { - prediction(i) = arma::as_scalar(arma::find( - arma::max(predictionTemp.col(i)) == predictionTemp.col(i), 1)) + 1; - } + // Deallocating all of network1's memory, so that + // if network2 is trying to use any of that memory. + arma::mat predictions2; + network2.Predict(trainData, predictions2); + CheckMatrices(predictions1, predictions2); +} + +// network1 should be allocated with `new`, and trained on some data. +template +void CheckMoveFunction(ModelType* network1, + MatType& trainData, + MatType& trainLabels, + const size_t maxEpochs) +{ + ens::RMSProp opt(0.01, 32, 0.88, 1e-8, maxEpochs * trainData.n_cols, -1); + network1->Train(trainData, trainLabels, opt); arma::mat predictions1; network1->Predict(trainData, predictions1); - FFN<> network3; - network3 = *network1; FFN<> network2(std::move(*network1)); delete network1; @@ -91,16 +99,13 @@ void CheckCopyMoveFunction(ModelType* network1, // if network2 is trying to use any of that memory. arma::mat predictions2; network2.Predict(trainData, predictions2); - arma::mat predictions3; - network3.Predict(trainData, predictions3); CheckMatrices(predictions1, predictions2); - CheckMatrices(predictions1, predictions3); } /** - * Check whether copying Vanila network is working or not. + * Check whether copying and moving Vanila network is working or not. */ -TEST_CASE("CheckCopyVanillaNetworkTest", "[FeedForwardNetworkTest]") +TEST_CASE("CheckCopyMovingVanillaNetworkTest", "[FeedForwardNetworkTest]") { // Load the dataset. arma::mat trainData; @@ -109,12 +114,6 @@ TEST_CASE("CheckCopyVanillaNetworkTest", "[FeedForwardNetworkTest]") arma::mat trainLabels = trainData.row(trainData.n_rows - 1); trainData.shed_row(trainData.n_rows - 1); - arma::mat testData; - data::Load("thyroid_test.csv", testData, true); - - arma::mat testLabels = testData.row(testData.n_rows - 1); - testData.shed_row(testData.n_rows - 1); - /* * Construct a feed forward network with trainData.n_rows input nodes, * hiddenLayerSize hidden nodes and trainLabels.n_rows output nodes. The @@ -143,9 +142,17 @@ TEST_CASE("CheckCopyVanillaNetworkTest", "[FeedForwardNetworkTest]") model->Add >(8, 3); model->Add >(); - // Check whether copy is working or not. - CheckCopyMoveFunction<>(model, - trainData, trainLabels, testData, testLabels, 1, 0.1); + FFN > *model1 = new FFN >; + model1->Add >(trainData.n_rows, 8); + model1->Add >(); + model1->Add >(8, 3); + model1->Add >(); + + // Check whether copy cpnstructor is working or not. + CheckCopyFunction<>(model, trainData, trainLabels, 1); + + // Check whether move cpnstructor is working or not. + CheckMoveFunction<>(model1, trainData, trainLabels, 1); } /**