fix: don't use variables in sparse matrices (also make variables upda…

…te on getValue())

1. that's kinda sus (what is 0 for a variable that can change)
2. it causes memory leaks (idk why tf this happens but it's not my problem™ anymore)

include/suboptimal/autodiff/Variable.h

@@ -45,7 +45,7 @@ class Variable {
    * Updates the value of the variable, traversing the expression tree and updating all expressions and variables this
    * variable depends on
    */
-  void update();
+  void update() const;
 
   /**
    * Sets the value of the variable. No-op if the variable is dependent on other variables/expressions
@@ -55,11 +55,10 @@ class Variable {
   bool setValue(double value);
 
   /**
-   * Gets the current stored value of the variable. If the variable is dependent and the variables/expressions it
-   * depends on may have changed, call update() first to get the correct value
+   * Gets the current stored value of the variable, first updating its expression tree
    * @return the value of the variable
    */
-  double getValue() const { return expr->value; }
+  double getValue() const;
 
   /**
    * Gets the degree of the expression this variable represents
@@ -251,7 +250,10 @@ Variable atan2(const Y& y, const X& x) {
 }
 }  // namespace suboptimal
 
+
+
 namespace Eigen {
+// Add Variable as an Eigen scalar type
 template <>
 struct NumTraits<suboptimal::Variable> : NumTraits<double> {
   using Real = suboptimal::Variable;
@@ -286,7 +288,6 @@ using Matrix4v = Eigen::Matrix4<Variable>;
 
 /**
  * Returns a matrix holding the values of the Variables in the input matrix
- * @tparam Derived the derived type of the input matrix
  * @param var_mat the input matrix of Variables
  * @return a double matrix holding the values of the Variables in the input matrix
  */
@@ -305,25 +306,23 @@ Eigen::Matrix<double, Derived::RowsAtCompileTime, Derived::ColsAtCompileTime> ge
 
 
 /**
-* Returns a sparse matrix holding the values of the Variables in the sparse input matrix
-* @tparam Scalar the scalar type of the input matrix
-* @tparam Options the storage scheme of the input matrix
-* @tparam StorageIndex the storage index type of the input matrix
-* @param var_mat the sparse matrix of Variables
-* @return a sparse double matrix holding the values of the Variables in the input matrix
-*/
-template <typename Scalar, int Options, typename StorageIndex>
-  requires std::same_as<Scalar, Variable>
-Eigen::SparseMatrix<double> getValues(const Eigen::SparseMatrix<Scalar, Options, StorageIndex>& var_mat) {
-  using InputMatType = Eigen::SparseMatrix<Scalar, Options, StorageIndex>;
-  using OutputMatType = Eigen::SparseMatrix<double, Options, StorageIndex>;
-
-  OutputMatType result(var_mat.rows(), var_mat.cols());
+ * Returns a sparse matrix holding the values of the Variables in the input matrix
+ * @param var_mat the input matrix of Variables
+ * @return a sparse double matrix holding the values of the Variables in the input matrix
+ */
+template <typename Derived>
+  requires std::same_as<typename Derived::Scalar, Variable>
+Eigen::SparseMatrix<double> getValuesSparse(const Eigen::MatrixBase<Derived>& var_mat) {
+  Eigen::SparseMatrix<double> result(var_mat.rows(), var_mat.cols());
   std::vector<Eigen::Triplet<double>> triplets{};
-  triplets.reserve(var_mat.nonZeros());
-  for (Eigen::Index i = 0; i < var_mat.outerSize(); i++) {
-    for (typename InputMatType::InnerIterator it(var_mat, i); it; ++it) {
-      triplets.push_back(Eigen::Triplet<double>(it.row(), it.col(), it.value().getValue()));
+
+  for (Eigen::Index i = 0; i < var_mat.rows(); i++) {
+    for (Eigen::Index j = 0; j < var_mat.cols(); j++) {
+      const double value = var_mat(i, j).getValue();
+      if (value == 0) {
+        continue;
+      }
+      triplets.emplace_back(i, j, value);
     }
   }
   result.setFromTriplets(triplets.begin(), triplets.end());

src/autodiff/Variable.cpp

@@ -12,7 +12,7 @@ Variable::Variable(const ExpressionPtr& expr) : expr{expr} {}
 
 Variable::Variable(ExpressionPtr&& expr) : expr{std::move(expr)} {}
 
-void Variable::update() {
+void Variable::update() const {
   expr->update();
 }
 
@@ -24,6 +24,11 @@ bool Variable::setValue(const double value) {
   return false;
 }
 
+double Variable::getValue() const {
+  update();
+  return expr->value;
+}
+
 Variable operator+(const Variable& x) {
   return x;
 }

test/autodiff/Variable_test.cpp

@@ -14,7 +14,7 @@
 using namespace suboptimal;
 
 TEST_CASE("Autodiff - Variable constructor", "[autodiff]") {
-  const Variable x{1.0};
+  Variable x{1.0};
   CHECK(x.getValue() == 1.0);
   CHECK(x.getType() == ExpressionType::Linear);
 
@@ -49,7 +49,6 @@ TEST_CASE("Autodiff - Variable basic arithmetic", "[autodiff]") {
     x.setValue(x_val);
     y.setValue(y_val);
 
-    f.update();
     CHECK_THAT(f.getValue(),
                Catch::Matchers::WithinAbs(1.0 + (x_val + y_val) * (x_val - y_val) / (x_val * y_val), 1e-9));
   }
@@ -84,7 +83,6 @@ TEST_CASE("Autodiff - Variable STL functions", "[autodiff]") {
 
     x.setValue(x_val);
     y.setValue(y_val);
-    f.update();
 
     const double f_val =
         std::sin(x_val) + std::cos(y_val) +
@@ -107,34 +105,7 @@ TEST_CASE("Autodiff - Eigen Variable support", "[autodiff]") {
                      {Variable{7}, Variable{8}}};
     const Matrix2v f = x * y;
     CHECK(suboptimal::getValues(f).isApprox(f_val));
-  }
-
-  SECTION("Sparse matrix operations") {
-    Eigen::SparseMatrix<double> x_val(2, 2);
-    x_val.insert(0, 0) = 1;
-    x_val.insert(0, 1) = 2;
-    x_val.insert(1, 0) = 3;
-    x_val.insert(1, 1) = 4;
-    Eigen::SparseMatrix<double> y_val(2, 2);
-    y_val.insert(0, 0) = 5;
-    y_val.insert(0, 1) = 6;
-    y_val.insert(1, 0) = 7;
-    y_val.insert(1, 1) = 8;
-    const Eigen::SparseMatrix<double> f_val = x_val * y_val;
-
-    Eigen::SparseMatrix<Variable> x(2, 2);
-    x.insert(0, 0) = Variable{1};
-    x.insert(0, 1) = Variable{2};
-    x.insert(1, 0) = Variable{3};
-    x.insert(1, 1) = Variable{4};
-    Eigen::SparseMatrix<Variable> y(2, 2);
-    y.insert(0, 0) = Variable{5};
-    y.insert(0, 1) = Variable{6};
-    y.insert(1, 0) = Variable{7};
-    y.insert(1, 1) = Variable{8};
-    const Eigen::SparseMatrix<Variable> f = x * y;
-
-    CHECK(suboptimal::getValues(f).isApprox(f_val));
+    CHECK(suboptimal::getValuesSparse(f).isApprox(f_val));
   }
 
   SECTION("Vector operations") {