From 6ddc06f64235f5f7e2f87704b57ebd97eb58b9c0 Mon Sep 17 00:00:00 2001 From: Jun An Date: Wed, 14 Mar 2018 19:14:13 +0800 Subject: [PATCH 1/8] Remove extra reference dataset constructor --- .../methods/range_search/range_search.hpp | 32 +++---------------- .../range_search/range_search_impl.hpp | 27 +--------------- 2 files changed, 5 insertions(+), 54 deletions(-) diff --git a/src/mlpack/methods/range_search/range_search.hpp b/src/mlpack/methods/range_search/range_search.hpp index d35d09b9c2..7709644c0c 100644 --- a/src/mlpack/methods/range_search/range_search.hpp +++ b/src/mlpack/methods/range_search/range_search.hpp @@ -51,9 +51,9 @@ class RangeSearch * naive mode or single-tree mode. Additionally, an instantiated metric can be * given, for cases where the distance metric holds data. * - * This method will copy the matrices to internal copies, which are rearranged - * during tree-building. You can avoid this extra copy by pre-constructing - * the trees and passing them using a different constructor. + * This method will move the matrices to internal copies, which are rearranged + * during tree-building. You can avoid creating an extra copy by + * pre-constructing the trees and passing them in using std::move. * * @param referenceSet Reference dataset. * @param naive Whether the computation should be done in O(n^2) naive mode. @@ -61,31 +61,7 @@ class RangeSearch * opposed to dual-tree computation). * @param metric Instantiated distance metric. */ - RangeSearch(const MatType& referenceSet, - const bool naive = false, - const bool singleMode = false, - const MetricType metric = MetricType()); - - /** - * Initialize the RangeSearch object with the given reference dataset (this is - * the dataset which is searched), taking ownership of the matrix. - * Optionally, perform the computation in naive mode or single-tree mode. - * Additionally, an instantiated metric can be given, for cases where the - * distance metric holds data. - * - * This method will not copy the data matrix, but will take ownership of it, - * and depending on the type of tree used, may rearrange the points. If you - * would rather a copy be made, consider using the constructor that takes a - * const reference to the data instead. - * - * @param referenceSet Set of reference points. - * @param naive If true, brute force naive search will be used (as opposed to - * dual-tree search). This overrides singleMode (if it is set to true). - * @param singleMode If true, single-tree search will be used (as opposed to - * dual-tree search). - * @param metric An optional instance of the MetricType class. - */ - RangeSearch(MatType&& referenceSet, + RangeSearch(MatType referenceSet, const bool naive = false, const bool singleMode = false, const MetricType metric = MetricType()); diff --git a/src/mlpack/methods/range_search/range_search_impl.hpp b/src/mlpack/methods/range_search/range_search_impl.hpp index 141ca217b6..3f1b672e2e 100644 --- a/src/mlpack/methods/range_search/range_search_impl.hpp +++ b/src/mlpack/methods/range_search/range_search_impl.hpp @@ -48,32 +48,7 @@ template class TreeType> RangeSearch::RangeSearch( - const MatType& referenceSetIn, - const bool naive, - const bool singleMode, - const MetricType metric) : - referenceTree(naive ? NULL : BuildTree(referenceSetIn, - oldFromNewReferences)), - referenceSet(naive ? &referenceSetIn : &referenceTree->Dataset()), - treeOwner(!naive), // If in naive mode, we are not building any trees. - setOwner(false), - naive(naive), - singleMode(!naive && singleMode), // Naive overrides single mode. - metric(metric), - baseCases(0), - scores(0) -{ - // Nothing to do. -} - -// Move constructor. -template class TreeType> -RangeSearch::RangeSearch( - MatType&& referenceSet, + MatType referenceSet, const bool naive, const bool singleMode, const MetricType metric) : From ce86b479659dfdde730f32262e595790efe481a8 Mon Sep 17 00:00:00 2001 From: Jun An Date: Wed, 14 Mar 2018 21:35:11 +0800 Subject: [PATCH 2/8] Remove and update =operator in range_search --- .../methods/range_search/range_search.hpp | 10 +---- .../range_search/range_search_impl.hpp | 44 +------------------ 2 files changed, 3 insertions(+), 51 deletions(-) diff --git a/src/mlpack/methods/range_search/range_search.hpp b/src/mlpack/methods/range_search/range_search.hpp index 7709644c0c..11b6e5b759 100644 --- a/src/mlpack/methods/range_search/range_search.hpp +++ b/src/mlpack/methods/range_search/range_search.hpp @@ -125,17 +125,11 @@ class RangeSearch /** * Copy the given RangeSearch model. + * Use std::move to pass in the model if an additional copy is not needed. * * @param other RangeSearch model to copy. */ - RangeSearch& operator=(const RangeSearch& other); - - /** - * Take ownership of the given RangeSearch model. - * - * @param other RangeSearch model to take ownership of. - */ - RangeSearch& operator=(RangeSearch&& other); + RangeSearch& operator=(RangeSearch other); /** * Destroy the RangeSearch object. If trees were created, they will be diff --git a/src/mlpack/methods/range_search/range_search_impl.hpp b/src/mlpack/methods/range_search/range_search_impl.hpp index 3f1b672e2e..662086aafa 100644 --- a/src/mlpack/methods/range_search/range_search_impl.hpp +++ b/src/mlpack/methods/range_search/range_search_impl.hpp @@ -175,37 +175,7 @@ template class TreeType> RangeSearch& -RangeSearch::operator=(const RangeSearch& other) -{ - // Clean memory first. - if (treeOwner) - delete referenceTree; - if (setOwner) - delete referenceSet; - - // Copy the other model. - oldFromNewReferences = other.oldFromNewReferences; - referenceTree = other.referenceTree ? new Tree(*other.referenceTree) : NULL; - referenceSet = other.referenceTree ? &referenceTree->Dataset() : - new MatType(*other.referenceSet); - treeOwner = other.referenceTree; - setOwner = !other.referenceTree; - naive = other.naive; - singleMode = other.singleMode; - metric = other.metric; - baseCases = other.baseCases; - scores = other.scores; - - return *this; -} - -template class TreeType> -RangeSearch& -RangeSearch::operator=(RangeSearch&& other) +RangeSearch::operator=(RangeSearch other) { // Clean memory first. if (treeOwner) @@ -225,18 +195,6 @@ RangeSearch::operator=(RangeSearch&& other) baseCases = other.baseCases; scores = other.scores; - // Clean other model. - other.referenceSet = new MatType(); - other.referenceTree = - BuildTree(const_cast(*other.referenceSet), - other.oldFromNewReferences); - other.treeOwner = true; - other.setOwner = true; - other.naive = false; - other.singleMode = false; - other.baseCases = 0; - other.scores = 0; - return *this; } From 5eb9d143015f55d571f5c0e2be47a96563fbe259 Mon Sep 17 00:00:00 2001 From: Jun An Date: Wed, 14 Mar 2018 22:03:23 +0800 Subject: [PATCH 3/8] Remove and update Train method with MatType --- .../methods/range_search/range_search.hpp | 12 +----- .../range_search/range_search_impl.hpp | 37 +------------------ 2 files changed, 3 insertions(+), 46 deletions(-) diff --git a/src/mlpack/methods/range_search/range_search.hpp b/src/mlpack/methods/range_search/range_search.hpp index 11b6e5b759..7c4ffeb139 100644 --- a/src/mlpack/methods/range_search/range_search.hpp +++ b/src/mlpack/methods/range_search/range_search.hpp @@ -143,19 +143,11 @@ class RangeSearch * the mlpack abstractions, even though calling this "training" is maybe a bit * of a stretch. * - * @param referenceSet New set of reference data. - */ - void Train(const MatType& referenceSet); - - /** - * Set the reference set to a new reference set, taking ownership of the set. - * A tree is built if necessary. This method is called 'Train()' in order to - * match the rest of the mlpack abstractions, even though calling this - * "training" is maybe a bit of a stretch. + * Use std::move to pass in the reference set if an additional copy is not needed. * * @param referenceSet New set of reference data. */ - void Train(MatType&& referenceSet); + void Train(MatType referenceSet); /** * Set the reference tree to a new reference tree. diff --git a/src/mlpack/methods/range_search/range_search_impl.hpp b/src/mlpack/methods/range_search/range_search_impl.hpp index 662086aafa..3f023ac2da 100644 --- a/src/mlpack/methods/range_search/range_search_impl.hpp +++ b/src/mlpack/methods/range_search/range_search_impl.hpp @@ -217,42 +217,7 @@ template class TreeType> void RangeSearch::Train( - const MatType& referenceSet) -{ - // Clean up the old tree, if we built one. - if (treeOwner && referenceTree) - delete referenceTree; - - // Rebuild the tree, if necessary. - if (!naive) - { - referenceTree = BuildTree(const_cast(referenceSet), - oldFromNewReferences); - treeOwner = true; - } - else - { - treeOwner = false; - } - - // Delete the old reference set, if we owned it. - if (setOwner && this->referenceSet) - delete this->referenceSet; - - if (!naive) - this->referenceSet = &referenceTree->Dataset(); - else - this->referenceSet = &referenceSet; - setOwner = false; -} - -template class TreeType> -void RangeSearch::Train( - MatType&& referenceSet) + MatType referenceSet) { // Clean up the old tree, if we built one. if (treeOwner && referenceTree) From 3c34bac4df709da05f66f40054509e6ca13e7f85 Mon Sep 17 00:00:00 2001 From: Jun An Date: Thu, 15 Mar 2018 00:21:07 +0800 Subject: [PATCH 4/8] Remove and update =operator in rs_model --- .../methods/range_search/range_search.hpp | 5 +++-- src/mlpack/methods/range_search/rs_model.hpp | 11 +++-------- .../methods/range_search/rs_model_impl.hpp | 19 ++----------------- 3 files changed, 8 insertions(+), 27 deletions(-) diff --git a/src/mlpack/methods/range_search/range_search.hpp b/src/mlpack/methods/range_search/range_search.hpp index 7c4ffeb139..cfdc146270 100644 --- a/src/mlpack/methods/range_search/range_search.hpp +++ b/src/mlpack/methods/range_search/range_search.hpp @@ -125,7 +125,7 @@ class RangeSearch /** * Copy the given RangeSearch model. - * Use std::move to pass in the model if an additional copy is not needed. + * Use std::move to pass in the model if the old copy is no longer needed. * * @param other RangeSearch model to copy. */ @@ -143,7 +143,8 @@ class RangeSearch * the mlpack abstractions, even though calling this "training" is maybe a bit * of a stretch. * - * Use std::move to pass in the reference set if an additional copy is not needed. + * Use std::move to pass in the reference set if an the old copy is no longer + * needed. * * @param referenceSet New set of reference data. */ diff --git a/src/mlpack/methods/range_search/rs_model.hpp b/src/mlpack/methods/range_search/rs_model.hpp index 6abd6cd07b..9e429a14be 100644 --- a/src/mlpack/methods/range_search/rs_model.hpp +++ b/src/mlpack/methods/range_search/rs_model.hpp @@ -287,16 +287,11 @@ class RSModel /** * Copy the given RSModel. * + * Use std::move to pass in the model if the old copy is no longer needed. + * * @param other RSModel to copy. */ - RSModel& operator=(const RSModel& other); - - /** - * Take ownership of the given RSModel. - * - * @param other RSModel to take ownership of. - */ - RSModel& operator=(RSModel&& other); + RSModel& operator=(RSModel other); /** * Clean memory, if necessary. diff --git a/src/mlpack/methods/range_search/rs_model_impl.hpp b/src/mlpack/methods/range_search/rs_model_impl.hpp index 8045aa168a..5c55cac3ee 100644 --- a/src/mlpack/methods/range_search/rs_model_impl.hpp +++ b/src/mlpack/methods/range_search/rs_model_impl.hpp @@ -33,30 +33,15 @@ inline RSModel::RSModel(TreeTypes treeType, bool randomBasis) : // Nothing to do. } -// Copy constructor. -inline RSModel::RSModel(const RSModel& other) : - treeType(other.treeType), - leafSize(other.leafSize), - randomBasis(other.randomBasis), - q(other.q), - rSearch(other.rSearch) -{ - // Nothing to do. -} - // Move constructor. -inline RSModel::RSModel(RSModel&& other) : +inline RSModel::RSModel(RSModel other) : treeType(other.treeType), leafSize(other.leafSize), randomBasis(other.randomBasis), q(std::move(other.q)), rSearch(std::move(other.rSearch)) { - // Reset other model. - other.treeType = TreeTypes::KD_TREE; - other.leafSize = 0; - other.randomBasis = false; - other.rSearch = decltype(other.rSearch)(); + // Nothing to do. } // Copy operator. From 44925a1595013737be2158942d65c536b25548c4 Mon Sep 17 00:00:00 2001 From: Jun An Date: Thu, 15 Mar 2018 23:34:07 +0800 Subject: [PATCH 5/8] Some changes in comments. --- src/mlpack/methods/range_search/range_search.hpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/mlpack/methods/range_search/range_search.hpp b/src/mlpack/methods/range_search/range_search.hpp index cfdc146270..d328daa7e1 100644 --- a/src/mlpack/methods/range_search/range_search.hpp +++ b/src/mlpack/methods/range_search/range_search.hpp @@ -51,8 +51,8 @@ class RangeSearch * naive mode or single-tree mode. Additionally, an instantiated metric can be * given, for cases where the distance metric holds data. * - * This method will move the matrices to internal copies, which are rearranged - * during tree-building. You can avoid creating an extra copy by + * This method will move the matrices to internal copies, which are + * rearranged during tree-building. You can avoid creating an extra copy by * pre-constructing the trees and passing them in using std::move. * * @param referenceSet Reference dataset. @@ -140,10 +140,10 @@ class RangeSearch /** * Set the reference set to a new reference set, and build a tree if * necessary. This method is called 'Train()' in order to match the rest of - * the mlpack abstractions, even though calling this "training" is maybe a bit - * of a stretch. + * the mlpack abstractions, even though calling this "training" is maybe a + * bit of a stretch. * - * Use std::move to pass in the reference set if an the old copy is no longer + * Use std::move to pass in the reference set if the old copy is no longer * needed. * * @param referenceSet New set of reference data. From ba4511b03c7e3152e86b0c019ef00a1a303ac8d2 Mon Sep 17 00:00:00 2001 From: Jun An Date: Fri, 16 Mar 2018 07:38:19 +0800 Subject: [PATCH 6/8] Resolve wrong method updated error in RSModel. --- .../methods/range_search/rs_model_impl.hpp | 42 ++++++++----------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/src/mlpack/methods/range_search/rs_model_impl.hpp b/src/mlpack/methods/range_search/rs_model_impl.hpp index 5c55cac3ee..23879f278d 100644 --- a/src/mlpack/methods/range_search/rs_model_impl.hpp +++ b/src/mlpack/methods/range_search/rs_model_impl.hpp @@ -33,33 +33,33 @@ inline RSModel::RSModel(TreeTypes treeType, bool randomBasis) : // Nothing to do. } +// Copy constructor. +inline RSModel::RSModel(const RSModel& other) : + treeType(other.treeType), + leafSize(other.leafSize), + randomBasis(other.randomBasis), + q(other.q), + rSearch(other.rSearch) +{ + // Nothing to do. +} + // Move constructor. -inline RSModel::RSModel(RSModel other) : +inline RSModel::RSModel(RSModel&& other) : treeType(other.treeType), leafSize(other.leafSize), randomBasis(other.randomBasis), q(std::move(other.q)), rSearch(std::move(other.rSearch)) { - // Nothing to do. + // Reset other model. + other.treeType = TreeTypes::KD_TREE; + other.leafSize = 0; + other.randomBasis = false; + other.rSearch = decltype(other.rSearch)(); } -// Copy operator. -inline RSModel& RSModel::operator=(const RSModel& other) -{ - boost::apply_visitor(DeleteVisitor(), rSearch); - - treeType = other.treeType; - leafSize = other.leafSize; - randomBasis = other.randomBasis; - q = other.q; - rSearch = other.rSearch; - - return *this; -} - -// Move operator. -inline RSModel& RSModel::operator=(RSModel&& other) +inline RSModel& RSModel::operator=(RSModel other) { boost::apply_visitor(DeleteVisitor(), rSearch); @@ -69,12 +69,6 @@ inline RSModel& RSModel::operator=(RSModel&& other) q = std::move(other.q); rSearch = std::move(other.rSearch); - // Reset other model. - other.treeType = TreeTypes::KD_TREE; - other.leafSize = 0; - other.randomBasis = false; - other.rSearch = decltype(other.rSearch)(); - return *this; } From fa23d377cee71d66f368cff72f21c4518df22c8f Mon Sep 17 00:00:00 2001 From: Jun An Date: Mon, 26 Mar 2018 01:34:50 +0800 Subject: [PATCH 7/8] Remove setOwner from range_search. --- .../methods/range_search/range_search.hpp | 2 -- .../range_search/range_search_impl.hpp | 25 +++++-------------- 2 files changed, 6 insertions(+), 21 deletions(-) diff --git a/src/mlpack/methods/range_search/range_search.hpp b/src/mlpack/methods/range_search/range_search.hpp index d328daa7e1..4b0c08f0b4 100644 --- a/src/mlpack/methods/range_search/range_search.hpp +++ b/src/mlpack/methods/range_search/range_search.hpp @@ -298,8 +298,6 @@ class RangeSearch //! If true, this object is responsible for deleting the trees. bool treeOwner; - //! If true, we own the reference set. - bool setOwner; //! If true, O(n^2) naive computation is used. bool naive; diff --git a/src/mlpack/methods/range_search/range_search_impl.hpp b/src/mlpack/methods/range_search/range_search_impl.hpp index 3f023ac2da..b7a441232a 100644 --- a/src/mlpack/methods/range_search/range_search_impl.hpp +++ b/src/mlpack/methods/range_search/range_search_impl.hpp @@ -57,7 +57,6 @@ RangeSearch::RangeSearch( referenceSet(naive ? new MatType(std::move(referenceSet)) : &referenceTree->Dataset()), treeOwner(!naive), - setOwner(naive), naive(naive), singleMode(!naive && singleMode), metric(metric), @@ -79,7 +78,6 @@ RangeSearch::RangeSearch( referenceTree(referenceTree), referenceSet(&referenceTree->Dataset()), treeOwner(false), - setOwner(false), naive(false), singleMode(singleMode), metric(metric), @@ -101,7 +99,6 @@ RangeSearch::RangeSearch( referenceTree(NULL), referenceSet(new MatType()), // Empty matrix. treeOwner(false), - setOwner(true), naive(naive), singleMode(singleMode), metric(metric), @@ -129,7 +126,6 @@ RangeSearch::RangeSearch( referenceSet(other.referenceTree ? &referenceTree->Dataset() : new MatType(*other.referenceSet)), treeOwner(other.referenceTree), - setOwner(!other.referenceTree), naive(other.naive), singleMode(other.singleMode), metric(other.metric), @@ -149,7 +145,6 @@ RangeSearch::RangeSearch(RangeSearch&& other) : referenceTree(other.referenceTree), referenceSet(other.referenceSet), treeOwner(other.treeOwner), - setOwner(other.setOwner), naive(other.naive), singleMode(other.singleMode), metric(std::move(other.metric)), @@ -162,7 +157,6 @@ RangeSearch::RangeSearch(RangeSearch&& other) : BuildTree(const_cast(*other.referenceSet), other.oldFromNewReferences); other.treeOwner = true; - other.setOwner = true; other.naive = false; other.singleMode = false; other.baseCases = 0; @@ -180,7 +174,7 @@ RangeSearch::operator=(RangeSearch other) // Clean memory first. if (treeOwner) delete referenceTree; - if (setOwner) + if (naive) delete referenceSet; // Move the other model. @@ -188,7 +182,6 @@ RangeSearch::operator=(RangeSearch other) referenceTree = other.referenceTree; referenceSet = other.referenceSet; treeOwner = other.treeOwner; - setOwner = other.setOwner; naive = other.naive; singleMode = other.singleMode; metric = std::move(other.metric); @@ -207,7 +200,7 @@ RangeSearch::~RangeSearch() { if (treeOwner && referenceTree) delete referenceTree; - if (setOwner && referenceSet) + if (naive && referenceSet) delete referenceSet; } @@ -236,18 +229,16 @@ void RangeSearch::Train( } // Delete the old reference set, if we owned it. - if (setOwner && this->referenceSet) + if (naive && this->referenceSet) delete this->referenceSet; if (!naive) { this->referenceSet = &referenceTree->Dataset(); - setOwner = false; } else { this->referenceSet = new MatType(std::move(referenceSet)); - setOwner = true; } } @@ -265,13 +256,12 @@ void RangeSearch::Train( if (treeOwner && referenceTree) delete this->referenceTree; - if (setOwner && referenceSet) + if (naive && referenceSet) delete this->referenceSet; this->referenceTree = referenceTree; this->referenceSet = &referenceTree->Dataset(); treeOwner = false; - setOwner = false; } template::serialize( { if (Archive::is_loading::value) { - if (setOwner && referenceSet) + if (naive && referenceSet) delete referenceSet; - - setOwner = true; } ar & BOOST_SERIALIZATION_NVP(referenceSet); @@ -696,12 +684,11 @@ void RangeSearch::serialize( // necessary. if (Archive::is_loading::value) { - if (setOwner && referenceSet) + if (naive && referenceSet) delete referenceSet; referenceSet = &referenceTree->Dataset(); metric = referenceTree->Metric(); // Get the metric from the tree. - setOwner = false; } } } From e67305ffbee3b8f192bcb8bcd29f2b32444da20c Mon Sep 17 00:00:00 2001 From: Ryan Curtin Date: Fri, 30 Mar 2018 10:28:11 -0400 Subject: [PATCH 8/8] Fix static code analysis issues. --- src/mlpack/methods/range_search/range_search_impl.hpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/mlpack/methods/range_search/range_search_impl.hpp b/src/mlpack/methods/range_search/range_search_impl.hpp index b7a441232a..d4cbcc5f21 100644 --- a/src/mlpack/methods/range_search/range_search_impl.hpp +++ b/src/mlpack/methods/range_search/range_search_impl.hpp @@ -256,8 +256,6 @@ void RangeSearch::Train( if (treeOwner && referenceTree) delete this->referenceTree; - if (naive && referenceSet) - delete this->referenceSet; this->referenceTree = referenceTree; this->referenceSet = &referenceTree->Dataset(); @@ -647,7 +645,7 @@ void RangeSearch::serialize( { if (Archive::is_loading::value) { - if (naive && referenceSet) + if (referenceSet) delete referenceSet; } @@ -684,9 +682,6 @@ void RangeSearch::serialize( // necessary. if (Archive::is_loading::value) { - if (naive && referenceSet) - delete referenceSet; - referenceSet = &referenceTree->Dataset(); metric = referenceTree->Metric(); // Get the metric from the tree. }