Improve BDT in L1Trigger/L1TMuonEndCap

- fixed memory corruption problems which could lead to double
 deletes or deletion of random memory locations
- memory handling now done via std::unique_ptr
- improved const correctness

Author: Dr15Jones

L1Trigger/L1TMuonEndCap/interface/bdt/Forest.h

@@ -3,6 +3,7 @@
 #ifndef L1Trigger_L1TMuonEndCap_emtf_Forest
 #define L1Trigger_L1TMuonEndCap_emtf_Forest
 
+#include <memory>
 #include "Tree.h"
 #include "LossFunctions.h"
 #include "CondFormats/L1TObjects/interface/L1TMuonEndCapForest.h"
@@ -14,28 +15,29 @@ namespace emtf {
     // Constructor(s)/Destructor
     Forest();
     Forest(std::vector<Event*>& trainingEvents);
-    ~Forest();
+    ~Forest() = default;
 
     Forest(const Forest& forest);
     Forest& operator=(const Forest& forest);
     Forest(Forest&& forest) = default;
+    Forest& operator=(Forest&& forest) = default;
 
     // Get/Set
     void setTrainingEvents(std::vector<Event*>& trainingEvents);
     std::vector<Event*> getTrainingEvents();
 
     // Returns the number of trees in the forest.
-    unsigned int size();
+    unsigned int size() const;
 
     // Get info on variable importance.
-    void rankVariables(std::vector<int>& rank);
+    void rankVariables(std::vector<int>& rank) const;
 
     // Output the list of split values used for each variable.
-    void saveSplitValues(const char* savefilename);
+    void saveSplitValues(const char* savefilename) const;
 
     // Helpful operations
-    void listEvents(std::vector<std::vector<Event*> >& e);
-    void sortEventVectors(std::vector<std::vector<Event*> >& e);
+    void listEvents(std::vector<std::vector<Event*>>& e) const;
+    void sortEventVectors(std::vector<std::vector<Event*>>& e) const;
     void generate(int numTrainEvents, int numTestEvents, double sigma);
     void loadForestFromXML(const char* directory, unsigned int numTrees);
     void loadFromCondPayload(const L1TMuonEndCapForest::DForest& payload);
@@ -63,9 +65,9 @@ namespace emtf {
     Tree* getTree(unsigned int i);
 
   private:
-    std::vector<std::vector<Event*> > events;
-    std::vector<std::vector<Event*> > subSample;
-    std::vector<Tree*> trees;
+    std::vector<std::vector<Event*>> events;
+    std::vector<std::vector<Event*>> subSample;
+    std::vector<std::unique_ptr<Tree>> trees;
   };
 
 }  // namespace emtf

L1Trigger/L1TMuonEndCap/interface/bdt/Node.h

@@ -5,6 +5,7 @@
 
 #include <string>
 #include <vector>
+#include <memory>
 #include "Event.h"
 
 namespace emtf {
@@ -13,43 +14,46 @@ namespace emtf {
   public:
     Node();
     Node(std::string cName);
-    ~Node();
+    ~Node() = default;
 
     Node(Node &&) = default;
     Node(const Node &) = delete;
     Node &operator=(const Node &) = delete;
 
-    std::string getName();
+    std::string getName() const;
     void setName(std::string sName);
 
-    double getErrorReduction();
+    double getErrorReduction() const;
     void setErrorReduction(double sErrorReduction);
 
     Node *getLeftDaughter();
-    void setLeftDaughter(Node *sLeftDaughter);
+    const Node *getLeftDaughter() const;
+    void setLeftDaughter(std::unique_ptr<Node> sLeftDaughter);
 
+    const Node *getRightDaughter() const;
     Node *getRightDaughter();
-    void setRightDaughter(Node *sLeftDaughter);
+    void setRightDaughter(std::unique_ptr<Node> sLeftDaughter);
 
     Node *getParent();
+    const Node *getParent() const;
     void setParent(Node *sParent);
 
-    double getSplitValue();
+    double getSplitValue() const;
     void setSplitValue(double sSplitValue);
 
-    int getSplitVariable();
+    int getSplitVariable() const;
     void setSplitVariable(int sSplitVar);
 
-    double getFitValue();
+    double getFitValue() const;
     void setFitValue(double sFitValue);
 
-    double getTotalError();
+    double getTotalError() const;
     void setTotalError(double sTotalError);
 
-    double getAvgError();
+    double getAvgError() const;
     void setAvgError(double sAvgError);
 
-    int getNumEvents();
+    int getNumEvents() const;
     void setNumEvents(int sNumEvents);
 
     std::vector<std::vector<Event *> > &getEvents();
@@ -64,8 +68,8 @@ namespace emtf {
   private:
     std::string name;
 
-    Node *leftDaughter;
-    Node *rightDaughter;
+    std::unique_ptr<Node> leftDaughter;
+    std::unique_ptr<Node> rightDaughter;
     Node *parent;
 
     double splitValue;

L1Trigger/L1TMuonEndCap/interface/bdt/Tree.h

@@ -4,6 +4,7 @@
 #define L1Trigger_L1TMuonEndCap_emtf_Tree
 
 #include <list>
+#include <memory>
 #include "Node.h"
 #include "TXMLEngine.h"
 #include "CondFormats/L1TObjects/interface/L1TMuonEndCapForest.h"
@@ -16,14 +17,15 @@ namespace emtf {
   public:
     Tree();
     Tree(std::vector<std::vector<Event*>>& cEvents);
-    ~Tree();
+    ~Tree() = default;
 
     Tree(const Tree& tree);
     Tree& operator=(const Tree& tree);
-    Tree(Tree&& tree);
+    Tree(Tree&& tree) = default;
 
     void setRootNode(Node* sRootNode);
     Node* getRootNode();
+    const Node* getRootNode() const;
 
     void setTerminalNodes(std::list<Node*>& sTNodes);
     std::list<Node*>& getTerminalNodes();
@@ -48,25 +50,25 @@ namespace emtf {
                                       const L1TMuonEndCapForest::DTreeNode& node,
                                       Node* tnode);
 
-    void rankVariables(std::vector<double>& v);
-    void rankVariablesRecursive(Node* node, std::vector<double>& v);
+    void rankVariables(std::vector<double>& v) const;
+    void rankVariablesRecursive(Node* node, std::vector<double>& v) const;
 
-    void getSplitValues(std::vector<std::vector<double>>& v);
-    void getSplitValuesRecursive(Node* node, std::vector<std::vector<double>>& v);
+    void getSplitValues(std::vector<std::vector<double>>& v) const;
+    void getSplitValuesRecursive(Node* node, std::vector<std::vector<double>>& v) const;
 
     double getBoostWeight(void) const { return boostWeight; }
     void setBoostWeight(double wgt) { boostWeight = wgt; }
 
   private:
-    Node* rootNode;
+    std::unique_ptr<Node> rootNode;
     std::list<Node*> terminalNodes;
     int numTerminalNodes;
     double rmsError;
     double boostWeight;
     unsigned xmlVersion;  // affects only XML loading part, save uses an old format and looses the boostWeight
 
     // this is the main recursive workhorse function that compensates for Nodes being non-copyable
-    Node* copyFrom(const Node* local_root);  // no garantees if throws in the process
+    std::unique_ptr<Node> copyFrom(const Node* local_root);  // no garantees if throws in the process
     // a dumb DFS tree traversal
     void findLeafs(Node* local_root, std::list<Node*>& tn);
   };

L1Trigger/L1TMuonEndCap/src/bdt/Forest.cc

@@ -45,37 +45,17 @@ Forest::Forest() { events = std::vector<std::vector<Event*>>(1); }
 
 Forest::Forest(std::vector<Event*>& trainingEvents) { setTrainingEvents(trainingEvents); }
 
-/////////////////////////////////////////////////////////////////////////
-// _______________________Destructor____________________________________//
-//////////////////////////////////////////////////////////////////////////
-
-Forest::~Forest() {
-  // When the forest is destroyed it will delete the trees as well as the
-  // events from the training and testing sets.
-  // The user may want the events to remain after they destroy the forest
-  // this should be changed in future upgrades.
-
-  for (unsigned int i = 0; i < trees.size(); i++) {
-    if (trees[i])
-      delete trees[i];
-  }
-}
-
 Forest::Forest(const Forest& forest) {
-  transform(forest.trees.cbegin(), forest.trees.cend(), back_inserter(trees), [](const Tree* tree) {
-    return new Tree(*tree);
+  transform(forest.trees.cbegin(), forest.trees.cend(), back_inserter(trees), [](const std::unique_ptr<Tree>& tree) {
+    return std::make_unique<Tree>(*tree);
   });
 }
 
 Forest& Forest::operator=(const Forest& forest) {
-  for (unsigned int i = 0; i < trees.size(); i++) {
-    if (trees[i])
-      delete trees[i];
-  }
   trees.resize(0);
-
-  transform(forest.trees.cbegin(), forest.trees.cend(), back_inserter(trees), [](const Tree* tree) {
-    return new Tree(*tree);
+  trees.reserve(forest.trees.size());
+  transform(forest.trees.cbegin(), forest.trees.cend(), back_inserter(trees), [](const std::unique_ptr<Tree>& tree) {
+    return std::make_unique<Tree>(*tree);
   });
   return *this;
 }
@@ -113,7 +93,7 @@ std::vector<Event*> Forest::getTrainingEvents() { return events[0]; }
 // return the ith tree
 Tree* Forest::getTree(unsigned int i) {
   if (/*i>=0 && */ i < trees.size())
-    return trees[i];
+    return trees[i].get();
   else {
     //std::cout << i << "is an invalid input for getTree. Out of range." << std::endl;
     return nullptr;
@@ -124,7 +104,7 @@ Tree* Forest::getTree(unsigned int i) {
 // ______________________Various_Helpful_Functions______________________//
 //////////////////////////////////////////////////////////////////////////
 
-unsigned int Forest::size() {
+unsigned int Forest::size() const {
   // Return the number of trees in the forest.
   return trees.size();
 }
@@ -139,7 +119,7 @@ unsigned int Forest::size() {
 // ----------------------------------------------------------------------
 //////////////////////////////////////////////////////////////////////////
 
-void Forest::listEvents(std::vector<std::vector<Event*>>& e) {
+void Forest::listEvents(std::vector<std::vector<Event*>>& e) const {
   // Simply list the events in each event vector. We have multiple copies
   // of the events vector. Each copy is sorted according to a different
   // determining variable.
@@ -178,7 +158,7 @@ bool compareEventsById(Event* e1, Event* e2) {
 // ----------------------------------------------------------------------
 //////////////////////////////////////////////////////////////////////////
 
-void Forest::sortEventVectors(std::vector<std::vector<Event*>>& e) {
+void Forest::sortEventVectors(std::vector<std::vector<Event*>>& e) const {
   // When a node chooses the optimum split point and split variable it needs
   // the events to be sorted according to the variable it is considering.
 
@@ -192,7 +172,7 @@ void Forest::sortEventVectors(std::vector<std::vector<Event*>>& e) {
 // ----------------------------------------------------------------------
 //////////////////////////////////////////////////////////////////////////
 
-void Forest::rankVariables(std::vector<int>& rank) {
+void Forest::rankVariables(std::vector<int>& rank) const {
   // This function ranks the determining variables according to their importance
   // in determining the fit. Use a low learning rate for better results.
   // Separates completely useless variables from useful ones well,
@@ -242,7 +222,7 @@ void Forest::rankVariables(std::vector<int>& rank) {
 // ----------------------------------------------------------------------
 //////////////////////////////////////////////////////////////////////////
 
-void Forest::saveSplitValues(const char* savefilename) {
+void Forest::saveSplitValues(const char* savefilename) const {
   // This function gathers all of the split values from the forest and puts them into lists.
 
   std::ofstream splitvaluefile;
@@ -377,8 +357,8 @@ void Forest::doRegression(int nodeLimit,
 
   for (unsigned int i = 0; i < (unsigned)treeLimit; i++) {
     // std::cout << "++Building Tree " << i << "... " << std::endl;
-    Tree* tree = new Tree(events);
-    trees.push_back(tree);
+    trees.emplace_back(std::make_unique<Tree>(events));
+    Tree* tree = trees.back().get();
     tree->buildTree(nodeLimit);
 
     // Update the targets for the next tree to fit.
@@ -426,7 +406,7 @@ void Forest::predictEvents(std::vector<Event*>& eventsp, unsigned int numtrees)
 void Forest::appendCorrection(std::vector<Event*>& eventsp, int treenum) {
   // Update the prediction by appending the next correction.
 
-  Tree* tree = trees[treenum];
+  Tree* tree = trees[treenum].get();
   tree->filterEvents(eventsp);
 
   // Update the events with their new prediction.
@@ -463,7 +443,7 @@ void Forest::predictEvent(Event* e, unsigned int numtrees) {
 void Forest::appendCorrection(Event* e, int treenum) {
   // Update the prediction by appending the next correction.
 
-  Tree* tree = trees[treenum];
+  Tree* tree = trees[treenum].get();
   Node* terminalNode = tree->filterEvent(e);
 
   // Update the event with its new prediction.
@@ -478,12 +458,13 @@ void Forest::loadForestFromXML(const char* directory, unsigned int numTrees) {
   // Load a forest that has already been created and stored into XML somewhere.
 
   // Initialize the vector of trees.
-  trees = std::vector<Tree*>(numTrees);
+  trees.resize(numTrees);
+  trees.shrink_to_fit();
 
   // Load the Forest.
   // std::cout << std::endl << "Loading Forest from XML ... " << std::endl;
   for (unsigned int i = 0; i < numTrees; i++) {
-    trees[i] = new Tree();
+    trees[i] = std::make_unique<Tree>();
 
     std::stringstream ss;
     ss << directory << "/" << i << ".xml";
@@ -499,17 +480,12 @@ void Forest::loadFromCondPayload(const L1TMuonEndCapForest::DForest& forest) {
   // Initialize the vector of trees.
   unsigned int numTrees = forest.size();
 
-  // clean-up leftovers from previous initialization (if any)
-  for (unsigned int i = 0; i < trees.size(); i++) {
-    if (trees[i])
-      delete trees[i];
-  }
-
-  trees = std::vector<Tree*>(numTrees);
+  trees.resize(numTrees);
+  trees.shrink_to_fit();
 
   // Load the Forest.
   for (unsigned int i = 0; i < numTrees; i++) {
-    trees[i] = new Tree();
+    trees[i] = std::make_unique<Tree>();
     trees[i]->loadFromCondPayload(forest[i]);
   }
 }
@@ -556,7 +532,8 @@ void Forest::doStochasticRegression(
 
   // Prepare some things.
   sortEventVectors(events);
-  trees = std::vector<Tree*>(treeLimit);
+  trees.resize(treeLimit);
+  trees.shrink_to_fit();
 
   // See how long the regression takes.
   TStopwatch timer;
@@ -572,15 +549,15 @@ void Forest::doStochasticRegression(
   for (unsigned int i = 0; i < (unsigned)treeLimit; i++) {
     // Build the tree using a random subsample.
     prepareRandomSubsample(fraction);
-    trees[i] = new Tree(subSample);
+    trees[i] = std::make_unique<Tree>(subSample);
     trees[i]->buildTree(nodeLimit);
 
     // Fit all of the events based upon the tree we built using
     // the subsample of events.
     trees[i]->filterEvents(events[0]);
 
     // Update the targets for the next tree to fit.
-    updateRegTargets(trees[i], learningRate, l);
+    updateRegTargets(trees[i].get(), learningRate, l);
 
     // Save trees to xml in some directory.
     std::ostringstream ss;