Move Online_calculator node de-registration to Deleter

CMakeLists.txt

@@ -38,6 +38,7 @@ set(STS_CPP_FILES
   ${CMAKE_CURRENT_SOURCE_DIR}/src/log/sampler.cc
   ${CMAKE_CURRENT_SOURCE_DIR}/src/metropolis_hastings_move.cc
   ${CMAKE_CURRENT_SOURCE_DIR}/src/node.cc
+  ${CMAKE_CURRENT_SOURCE_DIR}/src/node_deleter.cc
   ${CMAKE_CURRENT_SOURCE_DIR}/src/online_calculator.cc
   ${CMAKE_CURRENT_SOURCE_DIR}/src/rooted_merge.cc
   ${CMAKE_CURRENT_SOURCE_DIR}/src/smc_init.cc

src/edge.cc

@@ -36,10 +36,10 @@ Edge::Edge(std::shared_ptr<Node> node) :
     prior_log_likelihood(0),
     node(node) {}
 
-std::shared_ptr<Edge> Edge::of_tree(std::shared_ptr<sts::likelihood::Online_calculator> calc, bpp::TreeTemplate<bpp::Node> &tree, int node_number, std::unordered_map<std::shared_ptr<Node>, std::string>& names)
+std::shared_ptr<Edge> Edge::of_tree(bpp::TreeTemplate<bpp::Node> &tree, int node_number, std::unordered_map<std::shared_ptr<Node>, std::string>& names)
 {
     return std::make_shared<Edge>(
-               Node::of_tree(calc, tree, node_number, names),
+               Node::of_tree(tree, node_number, names),
                tree.getDistanceToFather(node_number));
 }
 

src/edge.h

@@ -35,8 +35,7 @@ class Edge
     std::shared_ptr<Node> node;
 
     /// Make an edge from a bpp Tree and node number
-    static std::shared_ptr<Edge> of_tree(std::shared_ptr<sts::likelihood::Online_calculator>,
-                                         bpp::TreeTemplate<bpp::Node> &,
+    static std::shared_ptr<Edge> of_tree(bpp::TreeTemplate<bpp::Node> &,
                                          int,
                                          std::unordered_map<std::shared_ptr<Node>, std::string>&);
 };

src/metropolis_hastings_move.cc

@@ -1,6 +1,7 @@
 #include "metropolis_hastings_move.h"
 
 #include "edge.h"
+#include "node_deleter.h"
 
 using namespace sts::particle;
 
@@ -25,7 +26,7 @@ int Metropolis_hastings_move::operator()(long time, smc::particle<particle::Part
 
     // Under a uniform topological prior, the prior for the current node is proportional to the branch length prior.
     const double cur_prior = cur_part->node->edge_prior_log_likelihood();
-    particle::Node_ptr new_node = std::make_shared<particle::Node>(*cur_part->node);
+    particle::Node_ptr new_node(new particle::Node(*cur_part->node), likelihood::Node_deleter(calc));
 
     new_part->node = new_node;
 

src/node.cc

@@ -3,33 +3,24 @@
 
 #include "node.h"
 #include "edge.h"
-#include "online_calculator.h"
 
 namespace sts
 {
 namespace particle
 {
 
-// Implementation
-Node::Node(std::shared_ptr<likelihood::Online_calculator> calc) : calc(calc) {}
-Node::Node(const Node & other) : calc(other.calc)
+Node::Node() {};
+
+Node::Node(const Node & other)
 {
     if(!other.is_leaf()) {
         child1 = std::make_shared<Edge>(*other.child1);
         child2 = std::make_shared<Edge>(*other.child2);
     }
 }
 
-Node::~Node()
-{
-    auto p = calc.lock();
-    if(p)
-        p->unregister_node(this);
-}
-
 Node & Node::operator=(const Node & other)
 {
-    calc = other.calc;
     if(!other.is_leaf()) {
         child1 = std::make_shared<Edge>(*other.child1);
         child2 = std::make_shared<Edge>(*other.child2);
@@ -43,23 +34,23 @@ bool Node::is_leaf() const
     return this->child1 == nullptr && this->child2 == nullptr;
 }
 
-Node_ptr Node::of_tree(std::shared_ptr<likelihood::Online_calculator> calc, bpp::TreeTemplate<bpp::Node> &tree, int node_number, std::unordered_map<Node_ptr, std::string>& names)
+Node_ptr Node::of_tree(bpp::TreeTemplate<bpp::Node>& tree, int node_number, std::unordered_map<Node_ptr, std::string>& names)
 {
-    Node_ptr n = std::make_shared<Node>(calc);
+    Node_ptr n = std::make_shared<Node>();
     if(tree.isLeaf(node_number)) {
         names[n] = tree.getNodeName(node_number);
         return n;
     }
     std::vector<int> children = tree.getSonsId(node_number);
     assert(children.size() == 2);
-    n->child1 = Edge::of_tree(calc, tree, children[0], names);
-    n->child2 = Edge::of_tree(calc, tree, children[1], names);
+    n->child1 = Edge::of_tree(tree, children[0], names);
+    n->child2 = Edge::of_tree(tree, children[1], names);
     return n;
 }
 
-Node_ptr Node::of_tree(std::shared_ptr<likelihood::Online_calculator> calc, bpp::TreeTemplate<bpp::Node> &tree, std::unordered_map<Node_ptr, std::string>& names)
+Node_ptr Node::of_tree(bpp::TreeTemplate<bpp::Node> &tree, std::unordered_map<Node_ptr, std::string>& names)
 {
-    return Node::of_tree(calc, tree, tree.getRootId(), names);
+    return Node::of_tree(tree, tree.getRootId(), names);
 }
 
 

src/node.h

@@ -10,25 +10,18 @@
 namespace sts
 {
 
-// Circular dependencies
-namespace likelihood
-{
-class Online_calculator;
-}
-
 namespace particle
 {
 
 class Edge;
 
 /// \class Node
-/// Represents the merge of two trees in a forest.
+/// \brief Represents the merge of two trees in a forest.
 class Node
 {
 public:
-    explicit Node(std::shared_ptr<likelihood::Online_calculator> calc);
+    Node();
     Node(const Node & other);
-    ~Node();
 
     Node & operator=(const Node & other);
 
@@ -39,18 +32,15 @@ class Node
 
     /// Make a Node from a bpp Tree
     static std::shared_ptr<Node>
-    of_tree(std::shared_ptr<likelihood::Online_calculator>,
-            bpp::TreeTemplate<bpp::Node>&,
+    of_tree(bpp::TreeTemplate<bpp::Node>&,
             std::unordered_map<std::shared_ptr<Node>, std::string>&);
 
     /// Make a Node from a bpp tree and node number
     static std::shared_ptr<Node>
-    of_tree(std::shared_ptr<likelihood::Online_calculator>, bpp::TreeTemplate<bpp::Node> &, int,
+    of_tree(bpp::TreeTemplate<bpp::Node> &, int,
             std::unordered_map<std::shared_ptr<Node>, std::string>&);
 
     double edge_prior_log_likelihood() const;
-private:
-    std::weak_ptr<likelihood::Online_calculator> calc;
 };
 
 /// A node in a phylogenetic tree

src/node_deleter.cc

@@ -0,0 +1,23 @@
+#include "node_deleter.h"
+#include "node.h"
+#include "online_calculator.h"
+
+using sts::particle::Node;
+
+namespace sts
+{
+namespace likelihood
+{
+
+void Node_deleter::operator()(Node* node)
+{
+    // Unregister from the calculator
+    if(auto p = calc.lock())
+        p->unregister_node(node);
+    // ... before deleting the node
+    d(node);
+}
+
+
+} // namespace likelihood
+} // namespace sts

src/node_deleter.h

@@ -0,0 +1,41 @@
+/// \file node_deleter.h
+#ifndef STS_LIKELIHOOD_NODE_DELETER_H
+#define STS_LIKELIHOOD_NODE_DELETER_H
+
+#include <memory>
+
+namespace sts
+{
+// Forwards
+namespace particle { class Node; }
+namespace likelihood
+{
+class Online_calculator;
+
+/// \brief Deleter for sts::particle::Node which unregisters nodes from an Online_calculator
+/// during deletion
+/// 
+/// Sample usage:
+/// \code
+/// std::shared_ptr<sts::likelihood::Online_calculator> c;
+/// sts::particle::Node_ptr p = sts::particle::Node_ptr(new sts::particle::Node(), Node_deleter(c));
+/// \endcode
+///
+/// \related Node
+/// \related Node_ptr
+struct Node_deleter
+{
+    Node_deleter() {};
+    Node_deleter(const std::shared_ptr<Online_calculator>& c) : calc(c) {};
+
+    /// The online calculator to unregister the node with on deletion
+    std::weak_ptr<Online_calculator> calc;
+    std::default_delete<sts::particle::Node> d;
+
+    void operator()(sts::particle::Node* node);
+};
+
+}
+}
+
+#endif // STS_LIKELIHOOD_NODE_DELETER_H

src/node_ptr.h

@@ -1,3 +1,6 @@
+/// \file node_ptr.h
+/// \brief Typedef for shared_ptr<Node>
+
 #ifndef STS_NODE_PTR_H
 #define STS_NODE_PTR_H
 
@@ -8,6 +11,17 @@ namespace sts
 namespace particle
 {
 class Node;
+
+/// \brief A shared_ptr to a Node.
+
+/// NB: if using Node_ptr in conjunction with an Online_calculator instance, use
+/// a Node_deleter, e.g.:
+/// \code
+/// std::shared_ptr<Online_calculator> calc = ...;
+/// Node_ptr n = Node_ptr(new Node(), Node_deleter(calc));
+/// \endcode
+/// \related sts::particle::Node
+/// \related sts::likelihood::Node_deleter
 typedef std::shared_ptr<Node> Node_ptr;
 } // sts
 } // particle

src/rooted_merge.cc

@@ -1,6 +1,7 @@
-#include "rooted_merge.h"
 #include "edge.h"
+#include "node_deleter.h"
 #include "node.h"
+#include "rooted_merge.h"
 #include "util.h"
 
 #include <cassert>
@@ -38,7 +39,7 @@ int Rooted_merge::do_move(long time, smc::particle<particle::Particle>& p_from,
     // The following gives the uniform distribution on legal choices that are not n1. Think of taking the uniform
     // distribution on [0,n-2], breaking it at n1 and moving the right hand bit one to the right.
     if(n2 >= n1) n2++;
-    pp->node = std::make_shared<particle::Node>(calc);
+    pp->node = particle::Node_ptr(new particle::Node(), likelihood::Node_deleter(calc));
 
     // Draw branch lengths.
     pp->node->child1 = std::make_shared<particle::Edge>(prop_vector[n1]);

src/state.cc

@@ -7,11 +7,11 @@ namespace sts
 namespace particle
 {
 
-Particle State::of_tree(std::shared_ptr<likelihood::Online_calculator> calc, bpp::TreeTemplate<bpp::Node> &tree,
+Particle State::of_tree(bpp::TreeTemplate<bpp::Node>& tree,
                         std::unordered_map<sts::particle::Node_ptr, std::string>& names)
 {
     Particle p = std::make_shared<State>();
-    p->node = Node::of_tree(calc, tree, names);
+    p->node = Node::of_tree(tree, names);
     if(p->node->is_leaf())
         return p;
 
@@ -34,13 +34,11 @@ Particle State::of_tree(std::shared_ptr<likelihood::Online_calculator> calc, bpp
     return p;
 }
 
-
-Particle State::of_newick_string(std::shared_ptr<likelihood::Online_calculator> calc, std::string &tree_string,
+Particle State::of_newick_string(std::string &tree_string,
                                  std::unordered_map<sts::particle::Node_ptr, std::string>& names)
 {
-    bpp::TreeTemplate<bpp::Node> *tree = bpp::TreeTemplateTools::parenthesisToTree(tree_string);
-    Particle node = State::of_tree(calc, *tree, names);
-    delete tree;
+    std::unique_ptr<bpp::TreeTemplate<bpp::Node>> tree(bpp::TreeTemplateTools::parenthesisToTree(tree_string));
+    Particle node = State::of_tree(*tree, names);
     return node;
 }
 

src/state.h

@@ -32,7 +32,7 @@ class State
         log_likelihood(0.0) {};
 
     /// The merge novel to this particle. If \c nullptr then the particle is \f$\perp\f$.
-    std::shared_ptr<Node> node;
+    Node_ptr node;
     /// The predecessor particles, which specify the rest of the merges for this particle.
     std::shared_ptr<State> predecessor;
 
@@ -45,14 +45,14 @@ class State
 
     /// Make a State from a bpp Tree
     static std::shared_ptr<State>
-    of_tree(std::shared_ptr<likelihood::Online_calculator>, bpp::TreeTemplate<bpp::Node> &, std::unordered_map<sts::particle::Node_ptr, std::string>&);
+    of_tree(bpp::TreeTemplate<bpp::Node> &, std::unordered_map<sts::particle::Node_ptr, std::string>&);
 
     /// Make a State from a Newick tree string
     static std::shared_ptr<State>
-    of_newick_string(std::shared_ptr<likelihood::Online_calculator>, std::string &, std::unordered_map<sts::particle::Node_ptr, std::string>&);
+    of_newick_string(std::string &, std::unordered_map<sts::particle::Node_ptr, std::string>&);
 };
 
 } // namespace particle
 } // namespace sts
 
-#endif // STS_PARTICLE_PHYLO_PARTICLE_H
+#endif // STS_PARTICLE_PHYLO_PARTICLE_H

src/sts.cc

@@ -5,6 +5,7 @@
 #include "forest_likelihood.h"
 #include "online_calculator.h"
 #include "node.h"
+#include "node_deleter.h"
 #include "state.h"
 #include "util.h"
 
@@ -21,7 +22,6 @@
 #include "gamma_branch_length_proposer.h"
 #include "uniform_branch_length_proposer.h"
 
-
 #include <Bpp/Numeric/Prob/ConstantDistribution.h>
 #include <Bpp/Phyl/Likelihood/RHomogeneousTreeLikelihood.h>
 #include <Bpp/Phyl/Model/GTR.h>
@@ -246,7 +246,7 @@ model->getAlphabet()));
     leaf_nodes.resize(num_iters);
     unordered_map<Node_ptr, string> node_name_map;
     for(int i = 0; i < num_iters; i++) {
-        leaf_nodes[i] = make_shared<Node>(calc);
+        leaf_nodes[i] = shared_ptr<Node>(new Node(), Node_deleter(calc));
         calc->register_leaf(leaf_nodes[i], aln->getSequencesNames()[i]);
         node_name_map[leaf_nodes[i]] = aln->getSequencesNames()[i];
     }

src/tests/test_sts_likelihood.hpp

@@ -53,7 +53,7 @@ void test_known_tree_jc69(std::string fasta_path, std::string newick_path, doubl
     if(compress)
         calc->set_weights(weights);
     std::unordered_map<sts::particle::Node_ptr, std::string> names;
-    auto root = sts::particle::State::of_newick_string(calc, nwk_string, names);
+    auto root = sts::particle::State::of_newick_string(nwk_string, names);
     // Register
     sts::util::register_nodes(*calc, root->node, names);
     std::unordered_set<sts::particle::Node_ptr> visited;