From f7da1a1ce0613c69ec0d04694105f904de2cda3a Mon Sep 17 00:00:00 2001
From: Neal Gafter <nmgafter@meta.com>
Date: Sun, 30 Oct 2022 14:50:13 -0700
Subject: [PATCH] minibmg: change std::list to std::vector (#1792)

Summary:
Pull Request resolved: https://github.com/facebookresearch/beanmachine/pull/1792

We change all uses of `std::list` to `std::vector` throughout minibmg.  The books I've read recommond not using `list` unless you really need some of its APIs, which we do not.  `vector` is faster and uses less memory.

I'm doing this now because I need it in another diff, and I want to keep each diff as simple as possible.

Reviewed By: AishwaryaSivaraman

Differential Revision: D40812419

fbshipit-source-id: 0ce97a5f3570aa2170e1ec48c7e285f15eb35fdb
---
 minibmg/ad/reverse.h                          | 78 ++++++++++---------
 minibmg/dedup.h                               |  1 -
 minibmg/fluid_factory.h                       |  2 +-
 minibmg/graph.cpp                             | 15 ++--
 minibmg/graph.h                               | 10 +--
 minibmg/graph_factory.cpp                     |  1 -
 minibmg/graph_factory.h                       |  3 +-
 .../graph_properties/observations_by_node.h   |  1 -
 minibmg/graph_properties/out_nodes.cpp        | 15 ++--
 minibmg/graph_properties/out_nodes.h          |  4 +-
 minibmg/json.cpp                              |  2 +-
 minibmg/rewrite_adapter.h                     | 28 -------
 minibmg/tests/ad/num3_test.cpp                |  1 -
 .../tests/graph_properties/out_nodes_test.cpp | 16 ++--
 minibmg/tests/localopt_test.cpp               | 12 +--
 minibmg/tests/minibmg_test.cpp                |  8 +-
 minibmg/tests/topological_test.cpp            |  4 +-
 minibmg/topological.h                         | 29 ++++---
 18 files changed, 103 insertions(+), 127 deletions(-)

diff --git a/minibmg/ad/reverse.h b/minibmg/ad/reverse.h
index 1536460bac..a7d03234f5 100644
--- a/minibmg/ad/reverse.h
+++ b/minibmg/ad/reverse.h
@@ -49,12 +49,14 @@ requires Number<Underlying>
 class ReverseBody {
  public:
   Underlying primal;
-  std::list<Reverse<Underlying>> inputs;
+  std::vector<Reverse<Underlying>> inputs;
   Underlying adjoint = 0;
 
   /* implicit */ ReverseBody(double primal);
   /* implicit */ ReverseBody(Underlying primal);
-  ReverseBody(Underlying primal, const std::list<Reverse<Underlying>>& inputs);
+  ReverseBody(
+      Underlying primal,
+      const std::vector<Reverse<Underlying>>& inputs);
   ReverseBody(const ReverseBody<Underlying>& other);
   ReverseBody<Underlying>& operator=(const ReverseBody<Underlying>& other);
   virtual ~ReverseBody() {}
@@ -95,17 +97,17 @@ requires Number<Underlying> Reverse<Underlying>
 template <class Underlying>
 class PrecomputedGradients : public ReverseBody<Underlying> {
  public:
-  const std::list<Underlying> computed_partial_derivatives;
+  const std::vector<Underlying> computed_partial_derivatives;
   PrecomputedGradients(
       Underlying primal,
-      const std::list<Reverse<Underlying>>& inputs,
-      const std::list<Underlying>& computed_partial_derivatives)
+      const std::vector<Reverse<Underlying>>& inputs,
+      const std::vector<Underlying>& computed_partial_derivatives)
       : ReverseBody<Underlying>{primal, inputs},
         computed_partial_derivatives{computed_partial_derivatives} {}
   void backprop() override {
-    auto& /*std::list<Reverse<Underlying>>*/ t = this->inputs;
-    typename std::list<Reverse<Underlying>>::iterator it1 = t.begin();
-    typename std::list<Underlying>::const_iterator it2 =
+    auto& /*std::vector<Reverse<Underlying>>*/ t = this->inputs;
+    typename std::vector<Reverse<Underlying>>::iterator it1 = t.begin();
+    typename std::vector<Underlying>::const_iterator it2 =
         computed_partial_derivatives.begin();
     for (; it1 != t.end() && it2 != computed_partial_derivatives.end();
          ++it1, ++it2) {
@@ -132,7 +134,7 @@ requires Number<Underlying>
 void Reverse<Underlying>::reverse(double initial_adjoint) {
   // topologically sort the set of ReverseBody pointers reachable - these are
   // the nodes to which we need to backprop.
-  std::list<Bodyp> roots = {ptr};
+  std::vector<Bodyp> roots = {ptr};
   std::function<std::vector<Bodyp>(const Bodyp&)> predecessors =
       [&](const Bodyp& ptr) -> std::vector<Bodyp> {
     std::vector<Bodyp> result;
@@ -172,7 +174,7 @@ requires Number<Underlying> ReverseBody<Underlying>::ReverseBody(
 template <class Underlying>
 requires Number<Underlying> ReverseBody<Underlying>::ReverseBody(
     Underlying primal,
-    const std::list<Reverse<Underlying>>& inputs)
+    const std::vector<Reverse<Underlying>>& inputs)
     : primal{primal}, inputs{inputs} {}
 
 template <class Underlying>
@@ -185,8 +187,8 @@ operator+(const Reverse<Underlying>& left, const Reverse<Underlying>& right) {
   Underlying new_primal = left.ptr->primal + right.ptr->primal;
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       new_primal,
-      std::list<Reverse<Underlying>>{left, right},
-      std::list<Underlying>{1, 1})};
+      std::vector<Reverse<Underlying>>{left, right},
+      std::vector<Underlying>{1, 1})};
 }
 
 template <class Underlying>
@@ -194,8 +196,8 @@ requires Number<Underlying> Reverse<Underlying>
 operator-(const Reverse<Underlying>& left, const Reverse<Underlying>& right) {
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       left.ptr->primal - right.ptr->primal,
-      std::list<Reverse<Underlying>>{left, right},
-      std::list<Underlying>{1, -1})};
+      std::vector<Reverse<Underlying>>{left, right},
+      std::vector<Underlying>{1, -1})};
 }
 
 template <class Underlying>
@@ -203,8 +205,8 @@ requires Number<Underlying> Reverse<Underlying>
 operator-(const Reverse<Underlying>& x) {
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       -x.ptr->primal,
-      std::list<Reverse<Underlying>>{x},
-      std::list<Underlying>{-1})};
+      std::vector<Reverse<Underlying>>{x},
+      std::vector<Underlying>{-1})};
 }
 
 template <class Underlying>
@@ -212,8 +214,8 @@ requires Number<Underlying> Reverse<Underlying>
 operator*(const Reverse<Underlying>& left, const Reverse<Underlying>& right) {
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       left.ptr->primal * right.ptr->primal,
-      std::list<Reverse<Underlying>>{left, right},
-      std::list<Underlying>{right.ptr->primal, left.ptr->primal})};
+      std::vector<Reverse<Underlying>>{left, right},
+      std::vector<Underlying>{right.ptr->primal, left.ptr->primal})};
 }
 
 template <class Underlying>
@@ -224,8 +226,8 @@ operator/(const Reverse<Underlying>& left, const Reverse<Underlying>& right) {
 
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       new_primal,
-      std::list<Reverse<Underlying>>{left, right},
-      std::list<Underlying>{
+      std::vector<Reverse<Underlying>>{left, right},
+      std::vector<Underlying>{
           1 / right.ptr->primal, -new_primal / right.ptr->primal})};
 }
 
@@ -255,8 +257,8 @@ requires Number<Underlying> Reverse<Underlying> pow(
                          .derivative1;
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       new_primal,
-      std::list<Reverse<Underlying>>{base, exponent},
-      std::list<Underlying>{grad1, grad2})};
+      std::vector<Reverse<Underlying>>{base, exponent},
+      std::vector<Underlying>{grad1, grad2})};
 }
 
 template <class Underlying>
@@ -265,8 +267,8 @@ requires Number<Underlying> Reverse<Underlying> exp(
   Underlying new_primal = exp(x.ptr->primal);
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       new_primal,
-      std::list<Reverse<Underlying>>{x},
-      std::list<Underlying>{new_primal})};
+      std::vector<Reverse<Underlying>>{x},
+      std::vector<Underlying>{new_primal})};
 }
 
 template <class Underlying>
@@ -275,8 +277,8 @@ requires Number<Underlying> Reverse<Underlying> log(
   Underlying new_primal = log(x.ptr->primal);
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       new_primal,
-      std::list<Reverse<Underlying>>{x},
-      std::list<Underlying>{1 / x.ptr->primal})};
+      std::vector<Reverse<Underlying>>{x},
+      std::vector<Underlying>{1 / x.ptr->primal})};
 }
 
 template <class Underlying>
@@ -286,8 +288,8 @@ requires Number<Underlying> Reverse<Underlying> atan(
   Underlying new_derivative1 = 1 / (x.ptr->primal * x.ptr->primal + 1.0f);
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       new_primal,
-      std::list<Reverse<Underlying>>{x},
-      std::list<Underlying>{new_derivative1})};
+      std::vector<Reverse<Underlying>>{x},
+      std::vector<Underlying>{new_derivative1})};
 }
 
 template <class Underlying>
@@ -297,8 +299,8 @@ requires Number<Underlying> Reverse<Underlying> lgamma(
   Underlying new_derivative1 = polygamma(0, x.ptr->primal);
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       new_primal,
-      std::list<Reverse<Underlying>>{x},
-      std::list<Underlying>{new_derivative1})};
+      std::vector<Reverse<Underlying>>{x},
+      std::vector<Underlying>{new_derivative1})};
 }
 
 template <class Underlying>
@@ -309,8 +311,8 @@ requires Number<Underlying> Reverse<Underlying> polygamma(
   Underlying new_derivative1 = polygamma(n + 1, x.ptr->primal);
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       new_primal,
-      std::list<Reverse<Underlying>>{x},
-      std::list<Underlying>{new_derivative1})};
+      std::vector<Reverse<Underlying>>{x},
+      std::vector<Underlying>{new_derivative1})};
 }
 
 template <class Underlying>
@@ -321,8 +323,8 @@ requires Number<Underlying> Reverse<Underlying> log1p(
   Underlying new_derivative1 = 1.0 / (x_value + 1);
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       new_primal,
-      std::list<Reverse<Underlying>>{x},
-      std::list<Underlying>{new_derivative1})};
+      std::vector<Reverse<Underlying>>{x},
+      std::vector<Underlying>{new_derivative1})};
 }
 
 template <class Underlying>
@@ -340,8 +342,8 @@ requires Number<Underlying> Reverse<Underlying> if_equal(
       when_not_equal.ptr->primal);
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       new_primal,
-      std::list<Reverse<Underlying>>{when_equal, when_not_equal},
-      std::list<Underlying>{
+      std::vector<Reverse<Underlying>>{when_equal, when_not_equal},
+      std::vector<Underlying>{
           if_equal(value.ptr->primal, comparand.ptr->primal, 1, 0),
           if_equal(value.ptr->primal, comparand.ptr->primal, 0, 1)})};
 }
@@ -361,8 +363,8 @@ requires Number<Underlying> Reverse<Underlying> if_less(
       when_not_less.ptr->primal);
   return Reverse<Underlying>{std::make_shared<PrecomputedGradients<Underlying>>(
       new_primal,
-      std::list<Reverse<Underlying>>{when_less, when_not_less},
-      std::list<Underlying>{
+      std::vector<Reverse<Underlying>>{when_less, when_not_less},
+      std::vector<Underlying>{
           if_less(value.ptr->primal, comparand.ptr->primal, 1, 0),
           if_less(value.ptr->primal, comparand.ptr->primal, 0, 1)})};
 }
diff --git a/minibmg/dedup.h b/minibmg/dedup.h
index 307afdff8d..5334c49123 100644
--- a/minibmg/dedup.h
+++ b/minibmg/dedup.h
@@ -8,7 +8,6 @@
 #pragma once
 
 #include <concepts>
-#include <list>
 #include <memory>
 #include <type_traits>
 #include <unordered_map>
diff --git a/minibmg/fluid_factory.h b/minibmg/fluid_factory.h
index 62baeeebc2..a2c71331ef 100644
--- a/minibmg/fluid_factory.h
+++ b/minibmg/fluid_factory.h
@@ -39,7 +39,7 @@ class Graph::FluidFactory {
 
  private:
   std::vector<Nodep> queries;
-  std::list<std::pair<Nodep, double>> observations;
+  std::vector<std::pair<Nodep, double>> observations;
 };
 
 } // namespace beanmachine::minibmg
diff --git a/minibmg/graph.cpp b/minibmg/graph.cpp
index a74e2b6493..3b4bb1824b 100644
--- a/minibmg/graph.cpp
+++ b/minibmg/graph.cpp
@@ -6,7 +6,6 @@
  */
 
 #include "beanmachine/minibmg/graph.h"
-#include <list>
 #include <memory>
 #include <stdexcept>
 #include <vector>
@@ -19,8 +18,8 @@ using namespace beanmachine::minibmg;
 
 const std::vector<Nodep> roots(
     const std::vector<Nodep>& queries,
-    const std::list<std::pair<Nodep, double>>& observations) {
-  std::list<Nodep> roots;
+    const std::vector<std::pair<Nodep, double>>& observations) {
+  std::vector<Nodep> roots;
   for (auto& n : queries) {
     roots.push_back(n);
   }
@@ -28,7 +27,7 @@ const std::vector<Nodep> roots(
     if (!std::dynamic_pointer_cast<const ScalarSampleNode>(p.first)) {
       throw std::invalid_argument(fmt::format("can only observe a sample"));
     }
-    roots.push_front(p.first);
+    roots.push_back(p.first);
   }
   std::vector<Nodep> all_nodes;
   if (!topological_sort<Nodep>(roots, &in_nodes, all_nodes)) {
@@ -40,7 +39,7 @@ const std::vector<Nodep> roots(
 
 struct QueriesAndObservations {
   std::vector<Nodep> queries;
-  std::list<std::pair<Nodep, double>> observations;
+  std::vector<std::pair<Nodep, double>> observations;
   ~QueriesAndObservations() {}
 };
 
@@ -65,7 +64,7 @@ class NodeRewriteAdapter<QueriesAndObservations> {
       const QueriesAndObservations& qo,
       const std::unordered_map<Nodep, Nodep>& map) const {
     NodeRewriteAdapter<std::vector<Nodep>> h1{};
-    NodeRewriteAdapter<std::list<std::pair<Nodep, double>>> h2{};
+    NodeRewriteAdapter<std::vector<std::pair<Nodep, double>>> h2{};
     return QueriesAndObservations{
         h1.rewrite(qo.queries, map), h2.rewrite(qo.observations, map)};
   }
@@ -75,7 +74,7 @@ using dynamic = folly::dynamic;
 
 Graph Graph::create(
     const std::vector<Nodep>& queries,
-    const std::list<std::pair<Nodep, double>>& observations,
+    const std::vector<std::pair<Nodep, double>>& observations,
     std::unordered_map<Nodep, Nodep>* built_map) {
   for (auto& p : observations) {
     if (!std::dynamic_pointer_cast<const ScalarSampleNode>(p.first)) {
@@ -95,7 +94,7 @@ Graph::~Graph() {}
 Graph::Graph(
     const std::vector<Nodep>& nodes,
     const std::vector<Nodep>& queries,
-    const std::list<std::pair<Nodep, double>>& observations)
+    const std::vector<std::pair<Nodep, double>>& observations)
     : nodes{nodes}, queries{queries}, observations{observations} {}
 
 } // namespace beanmachine::minibmg
diff --git a/minibmg/graph.h b/minibmg/graph.h
index 2d01ef4017..e871bf7620 100644
--- a/minibmg/graph.h
+++ b/minibmg/graph.h
@@ -8,7 +8,7 @@
 #pragma once
 
 #include <folly/json.h>
-#include <list>
+#include <vector>
 #include "beanmachine/minibmg/dedup.h"
 #include "beanmachine/minibmg/graph_properties/container.h"
 #include "beanmachine/minibmg/node.h"
@@ -27,7 +27,7 @@ class Graph : public Container {
 
   static Graph create(
       const std::vector<Nodep>& queries,
-      const std::list<std::pair<Nodep, double>>& observations,
+      const std::vector<std::pair<Nodep, double>>& observations,
       std::unordered_map<Nodep, Nodep>* built_map = nullptr);
   ~Graph();
 
@@ -55,7 +55,7 @@ class Graph : public Container {
 
   // Observations of the model.  These are SAMPLE nodes in the model whose
   // values are known.
-  const std::list<std::pair<Nodep, double>> observations;
+  const std::vector<std::pair<Nodep, double>> observations;
 
  private:
   // A private constructor that forms a graph without validation.
@@ -63,7 +63,7 @@ class Graph : public Container {
   Graph(
       const std::vector<Nodep>& nodes,
       const std::vector<Nodep>& queries,
-      const std::list<std::pair<Nodep, double>>& observations);
+      const std::vector<std::pair<Nodep, double>>& observations);
 
  public:
   // A factory for making graphs, like the bmg API used by Beanstalk
@@ -99,7 +99,7 @@ class NodeRewriteAdapter<Graph> {
   Graph rewrite(const Graph& qo, const std::unordered_map<Nodep, Nodep>& map)
       const {
     NodeRewriteAdapter<std::vector<Nodep>> h1{};
-    NodeRewriteAdapter<std::list<std::pair<Nodep, double>>> h2{};
+    NodeRewriteAdapter<std::vector<std::pair<Nodep, double>>> h2{};
     return Graph::create(
         h1.rewrite(qo.queries, map), h2.rewrite(qo.observations, map));
   }
diff --git a/minibmg/graph_factory.cpp b/minibmg/graph_factory.cpp
index a03d9d4ec4..0cc8a7c057 100644
--- a/minibmg/graph_factory.cpp
+++ b/minibmg/graph_factory.cpp
@@ -10,7 +10,6 @@
 #include <stdexcept>
 #include <unordered_map>
 #include "beanmachine/minibmg/node.h"
-#include "node.h"
 
 namespace beanmachine::minibmg {
 
diff --git a/minibmg/graph_factory.h b/minibmg/graph_factory.h
index ce5960c3d3..ed3ec32461 100644
--- a/minibmg/graph_factory.h
+++ b/minibmg/graph_factory.h
@@ -8,7 +8,6 @@
 #pragma once
 
 #include <fmt/format.h>
-#include <list>
 #include <memory>
 #include <unordered_map>
 #include <vector>
@@ -123,7 +122,7 @@ class Graph::Factory {
   std::unordered_map<NodeId, Nodep> identifer_to_node;
   std::unordered_map<Nodep, NodeId> node_to_identifier;
   std::vector<Nodep> queries;
-  std::list<std::pair<Nodep, double>> observations;
+  std::vector<std::pair<Nodep, double>> observations;
   unsigned long next_identifier = 0;
 
   ScalarNodeId add_node(ScalarNodep node);
diff --git a/minibmg/graph_properties/observations_by_node.h b/minibmg/graph_properties/observations_by_node.h
index 2e19b8c0c1..39ddf0b177 100644
--- a/minibmg/graph_properties/observations_by_node.h
+++ b/minibmg/graph_properties/observations_by_node.h
@@ -7,7 +7,6 @@
 
 #pragma once
 
-#include <list>
 #include <unordered_set>
 #include "beanmachine/minibmg/graph.h"
 #include "beanmachine/minibmg/node.h"
diff --git a/minibmg/graph_properties/out_nodes.cpp b/minibmg/graph_properties/out_nodes.cpp
index b741a977c8..b5da96bd33 100644
--- a/minibmg/graph_properties/out_nodes.cpp
+++ b/minibmg/graph_properties/out_nodes.cpp
@@ -7,7 +7,6 @@
 
 #include "beanmachine/minibmg/graph_properties/out_nodes.h"
 #include <exception>
-#include <list>
 #include <map>
 
 namespace {
@@ -17,13 +16,13 @@ using namespace beanmachine::minibmg;
 class OutNodesProperty : public Property<
                              OutNodesProperty,
                              Graph,
-                             std::map<Nodep, std::list<Nodep>>> {
+                             std::map<Nodep, std::vector<Nodep>>> {
  public:
-  std::map<Nodep, std::list<Nodep>>* create(const Graph& g) const override {
-    std::map<Nodep, std::list<Nodep>>* data =
-        new std::map<Nodep, std::list<Nodep>>{};
+  std::map<Nodep, std::vector<Nodep>>* create(const Graph& g) const override {
+    std::map<Nodep, std::vector<Nodep>>* data =
+        new std::map<Nodep, std::vector<Nodep>>{};
     for (auto node : g) {
-      (*data)[node] = std::list<Nodep>{};
+      (*data)[node] = std::vector<Nodep>{};
       for (auto in_node : in_nodes(node)) {
         auto& predecessor_out_set = (*data)[in_node];
         predecessor_out_set.push_back(node);
@@ -38,8 +37,8 @@ class OutNodesProperty : public Property<
 
 namespace beanmachine::minibmg {
 
-const std::list<Nodep>& out_nodes(const Graph& graph, Nodep node) {
-  std::map<Nodep, std::list<Nodep>>& node_map = *OutNodesProperty::get(graph);
+const std::vector<Nodep>& out_nodes(const Graph& graph, Nodep node) {
+  std::map<Nodep, std::vector<Nodep>>& node_map = *OutNodesProperty::get(graph);
   auto found = node_map.find(node);
   if (found == node_map.end()) {
     throw std::invalid_argument("node not in graph");
diff --git a/minibmg/graph_properties/out_nodes.h b/minibmg/graph_properties/out_nodes.h
index 7d693929e2..137463315e 100644
--- a/minibmg/graph_properties/out_nodes.h
+++ b/minibmg/graph_properties/out_nodes.h
@@ -7,8 +7,8 @@
 
 #pragma once
 
-#include <list>
 #include <unordered_set>
+#include <vector>
 #include "beanmachine/minibmg/graph.h"
 #include "beanmachine/minibmg/node.h"
 
@@ -16,6 +16,6 @@ namespace beanmachine::minibmg {
 
 // return the set of nodes that have the given node as an input in the given
 // graph.
-const std::list<Nodep>& out_nodes(const Graph& graph, Nodep node);
+const std::vector<Nodep>& out_nodes(const Graph& graph, Nodep node);
 
 } // namespace beanmachine::minibmg
diff --git a/minibmg/json.cpp b/minibmg/json.cpp
index 0a687dcf44..fbd9a40f15 100644
--- a/minibmg/json.cpp
+++ b/minibmg/json.cpp
@@ -431,7 +431,7 @@ Graph json_to_graph2(folly::dynamic d) {
     }
   }
 
-  std::list<std::pair<Nodep, double>> observations;
+  std::vector<std::pair<Nodep, double>> observations;
   auto observation_nodes = d["observations"];
   if (observation_nodes.isArray()) {
     for (auto& obs : observation_nodes) {
diff --git a/minibmg/rewrite_adapter.h b/minibmg/rewrite_adapter.h
index 6df3ebfc13..95fdfe733c 100644
--- a/minibmg/rewrite_adapter.h
+++ b/minibmg/rewrite_adapter.h
@@ -7,7 +7,6 @@
 
 #pragma once
 
-#include <list>
 #include <utility>
 #include <vector>
 #include "beanmachine/minibmg/ad/real.h"
@@ -108,33 +107,6 @@ class NodeRewriteAdapter<std::vector<T>> {
 };
 static_assert(Rewritable<std::vector<Nodep>>);
 
-// A list can be deduplicated
-template <class T>
-requires Rewritable<T>
-class NodeRewriteAdapter<std::list<T>> {
-  NodeRewriteAdapter<T> t_helper{};
-
- public:
-  std::vector<Nodep> find_roots(const std::list<T>& roots) const {
-    std::vector<Nodep> result;
-    for (const auto& root : roots) {
-      auto more_roots = t_helper.find_roots(root);
-      result.push_back(more_roots.begin(), more_roots.end());
-    }
-    return result;
-  }
-  std::list<T> rewrite(
-      const std::list<T>& roots,
-      const std::unordered_map<Nodep, Nodep>& map) const {
-    std::list<T> result;
-    for (const auto& root : roots) {
-      result.push_back(t_helper.rewrite(root, map));
-    }
-    return result;
-  }
-};
-static_assert(Rewritable<std::list<Nodep>>);
-
 // A pair can be deduplicated
 template <class T, class U>
 requires Rewritable<T> && Rewritable<U>
diff --git a/minibmg/tests/ad/num3_test.cpp b/minibmg/tests/ad/num3_test.cpp
index 5d60daa4f6..60f543103c 100644
--- a/minibmg/tests/ad/num3_test.cpp
+++ b/minibmg/tests/ad/num3_test.cpp
@@ -9,7 +9,6 @@
 #include <cassert>
 #include <cmath>
 #include <cstdlib>
-#include <list>
 #include <random>
 
 #include "beanmachine/minibmg/ad/num2.h"
diff --git a/minibmg/tests/graph_properties/out_nodes_test.cpp b/minibmg/tests/graph_properties/out_nodes_test.cpp
index c58deb4335..7243a95f49 100644
--- a/minibmg/tests/graph_properties/out_nodes_test.cpp
+++ b/minibmg/tests/graph_properties/out_nodes_test.cpp
@@ -6,7 +6,7 @@
  */
 
 #include <gtest/gtest.h>
-#include <list>
+#include <vector>
 #include "beanmachine/minibmg/graph.h"
 #include "beanmachine/minibmg/graph_factory.h"
 #include "beanmachine/minibmg/graph_properties/out_nodes.h"
@@ -33,15 +33,15 @@ TEST(out_nodes_test, simple) {
   Nodep betan = gf[beta];
   Nodep samplen = gf[sample];
 
-  ASSERT_EQ(out_nodes(g, k12n), std::list{plusn});
-  ASSERT_EQ(out_nodes(g, k34n), (std::list{plusn}));
-  ASSERT_EQ(out_nodes(g, plusn), std::list<Nodep>{betan});
-  ASSERT_EQ(out_nodes(g, k56n), std::list{betan});
-  ASSERT_EQ(out_nodes(g, betan), (std::list{samplen}));
-  ASSERT_EQ(out_nodes(g, samplen), std::list<Nodep>{});
+  ASSERT_EQ(out_nodes(g, k12n), std::vector{plusn});
+  ASSERT_EQ(out_nodes(g, k34n), (std::vector{plusn}));
+  ASSERT_EQ(out_nodes(g, plusn), std::vector<Nodep>{betan});
+  ASSERT_EQ(out_nodes(g, k56n), std::vector{betan});
+  ASSERT_EQ(out_nodes(g, betan), (std::vector{samplen}));
+  ASSERT_EQ(out_nodes(g, samplen), std::vector<Nodep>{});
 
   ASSERT_EQ(g.queries, std::vector{samplen});
-  std::list<std::pair<Nodep, double>> expected_observations;
+  std::vector<std::pair<Nodep, double>> expected_observations;
   expected_observations.push_back(std::pair{samplen, 7.8});
   ASSERT_EQ(g.observations, expected_observations);
 }
diff --git a/minibmg/tests/localopt_test.cpp b/minibmg/tests/localopt_test.cpp
index 2a981c8cfc..358d744bb7 100644
--- a/minibmg/tests/localopt_test.cpp
+++ b/minibmg/tests/localopt_test.cpp
@@ -83,16 +83,16 @@ TEST(localopt_test, symbolic_derivatives) {
   printed << "d2 = ";
   printed << print_result.code[d2] << std::endl;
 
-  auto expected = R"(auto temp_1 = -pow(rvid.constrained, -2);
+  auto expected = R"(auto temp_1 = log(rvid.constrained);
 auto temp_2 = 1 - rvid.constrained;
-auto temp_3 = -pow(temp_2, -2);
+auto temp_3 = log(temp_2);
 auto temp_4 = 1 / rvid.constrained;
 auto temp_5 = -1 / temp_2;
-auto temp_6 = log(rvid.constrained);
-auto temp_7 = log(temp_2);
-log_prob = temp_6 + temp_7 + 1.791759469228055 + temp_6 + temp_6 + temp_7
+auto temp_6 = -pow(rvid.constrained, -2);
+auto temp_7 = -pow(temp_2, -2);
+log_prob = temp_1 + temp_3 + 1.791759469228055 + temp_1 + temp_1 + temp_3
 d1 = temp_4 + temp_5 + temp_4 + temp_4 + temp_5
-d2 = temp_1 + temp_3 + temp_1 + temp_1 + temp_3
+d2 = temp_6 + temp_7 + temp_6 + temp_6 + temp_7
 )";
   ASSERT_EQ(expected, printed.str());
 }
diff --git a/minibmg/tests/minibmg_test.cpp b/minibmg/tests/minibmg_test.cpp
index d0d4540501..7979f6570f 100644
--- a/minibmg/tests/minibmg_test.cpp
+++ b/minibmg/tests/minibmg_test.cpp
@@ -77,10 +77,10 @@ TEST(test_minibmg, dedupable_concept) {
   ASSERT_FALSE(Rewritable<std::vector<std::vector<int>>>);
   ASSERT_TRUE((Rewritable<std::pair<Nodep, double>>));
   ASSERT_FALSE((Rewritable<std::pair<int, double>>));
-  ASSERT_TRUE(Rewritable<std::list<Nodep>>);
-  ASSERT_TRUE(Rewritable<std::list<std::list<Nodep>>>);
-  ASSERT_FALSE(Rewritable<std::list<int>>);
-  ASSERT_FALSE(Rewritable<std::list<std::list<int>>>);
+  ASSERT_TRUE(Rewritable<std::vector<Nodep>>);
+  ASSERT_TRUE(Rewritable<std::vector<std::vector<Nodep>>>);
+  ASSERT_FALSE(Rewritable<std::vector<int>>);
+  ASSERT_FALSE(Rewritable<std::vector<std::vector<int>>>);
 }
 
 TEST(test_minibmg, graph_factory_nodeid_equality) {
diff --git a/minibmg/tests/topological_test.cpp b/minibmg/tests/topological_test.cpp
index 23bfe6ea8c..2b89090122 100644
--- a/minibmg/tests/topological_test.cpp
+++ b/minibmg/tests/topological_test.cpp
@@ -80,7 +80,7 @@ TEST(topological_test, ensure_sorted) {
     // topologically sort them.
     std::vector<Node*> result;
     auto sorted = topological_sort<Node*>(
-        std::list<Node*>{nodes.begin(), nodes.end()},
+        nodes,
         [](Node* node) {
           return std::vector<Node*>{
               node->successors.begin(), node->successors.end()};
@@ -105,7 +105,7 @@ TEST(topological_test, ensure_sorted) {
     // topologically sort them.  if there was a cycle, this should return false.
     result.clear();
     sorted = topological_sort<Node*>(
-        std::list<Node*>{nodes.begin(), nodes.end()},
+        nodes,
         [](Node* const& node) {
           return std::vector<Node*>{
               node->successors.begin(), node->successors.end()};
diff --git a/minibmg/topological.h b/minibmg/topological.h
index 4722e0dcfd..52692369a0 100644
--- a/minibmg/topological.h
+++ b/minibmg/topological.h
@@ -8,9 +8,9 @@
 #pragma once
 
 #include <functional>
-#include <list>
 #include <map>
 #include <set>
+#include <vector>
 
 namespace {
 
@@ -18,15 +18,24 @@ namespace {
 // given.
 template <class T>
 std::map<T, unsigned> count_predecessors_internal(
-    const std::list<T>& root_nodes,
+    const std::vector<T>& root_nodes,
     std::function<std::vector<T>(const T&)> successors,
-    std::list<T>& nodes) {
+    std::vector<T>& nodes,
+    bool include_roots = false) {
   std::map<T, unsigned> predecessor_counts;
-  std::list<T> to_count;
+  std::vector<T> to_count;
   std::set<T> counted;
   for (const auto& node : root_nodes) {
     to_count.push_back(node);
+    if (include_roots) {
+      if (!predecessor_counts.contains(node)) {
+        predecessor_counts[node] = 1;
+      } else {
+        predecessor_counts[node] = predecessor_counts[node] + 1;
+      }
+    }
   }
+  std::reverse(to_count.begin(), to_count.end());
 
   while (!to_count.empty()) {
     auto node = to_count.back();
@@ -64,10 +73,10 @@ template <class T>
 bool topological_sort_internal(
     std::map<T, unsigned>& predecessor_counts,
     std::function<std::vector<T>(const T&)> successors,
-    std::list<T>& nodes,
+    std::vector<T>& nodes,
     std::vector<T>& result) {
   // initialize the ready set with those nodes that have no predecessors
-  std::list<T> ready;
+  std::vector<T> ready;
   for (auto node : nodes) {
     if (predecessor_counts[node] == 0) {
       ready.push_back(node);
@@ -104,9 +113,9 @@ namespace beanmachine::minibmg {
 // given.
 template <class T>
 std::map<T, unsigned> count_predecessors(
-    const std::list<T>& root_nodes,
+    const std::vector<T>& root_nodes,
     std::function<std::vector<T>(const T&)> successors) {
-  std::list<T> ready;
+  std::vector<T> ready;
   return count_predecessors_internal<T>(root_nodes, successors, ready);
 }
 
@@ -116,10 +125,10 @@ std::map<T, unsigned> count_predecessors(
 // sorted result in the `result` parameter.
 template <class T>
 bool topological_sort(
-    const std::list<T>& root_nodes,
+    const std::vector<T>& root_nodes,
     std::function<std::vector<T>(const T&)> successors,
     std::vector<T>& result) {
-  std::list<T> ready;
+  std::vector<T> ready;
   // count the predecessors of each node.
   std::map<T, unsigned> predecessor_counts =
       count_predecessors_internal<T>(root_nodes, successors, ready);