bgrimstad/splinter

Potential InnerIterator bug

berndbischl opened this issue · 1 comments

Hi, I noted this in the R interface, see #87 and berndbischl/splinter-r-pkg#3

NB: I am very new to the project, and not the greatest expert on Eigen, so if this is bullshit: sorry.

Look at this local fix I did to your codebase:

--- a/src/bsplinebasis.cpp
+++ b/src/bsplinebasis.cpp
@@ -119,7 +119,7 @@ SparseMatrix BSplineBasis::evalBasisJacobian(DenseVector &x) const

         // Fill out column
         for (int k = 0; k < Ji.outerSize(); ++k)
-        for (SparseMatrix::InnerIterator it(Ji,k); it; ++it)
+        for (SparseVector::InnerIterator it(Ji,k); it; ++it)

The original line of yours looks wrong. Ji is a SparseVector not a SparseMatrix.
With your original code I cannot even compile. When I link to RcppEigen (which maybe uses a newer version of Eigen than you guys?) I end up in this PRIVATE part of code

From:
include/Eigen/src/SparseCore/SparseCompressedBase.h

207   private:                                                                                                                                                         
208     // If you get here, then you're not using the right InnerIterator type, e.g.:                        
209     //   SparseMatrix<double,RowMajor> A;                                                                                                        
210     //   SparseMatrix<double>::InnerIterator it(A,0);                                                                                            
211     template<typename T> InnerIterator(const SparseMatrixBase<T>&, Index outer);                         
212 };                                                                                                                                               
213       

Thank you. This code has been rewritten in preparation of SPLINTER v4.0. It looks like we are using SparseVector::InnerIterator now, as you suggested. The code runs through our CI without any problems so I will proceed with closing this.

Edit: From the example in Eigen's documentation it looks like SparseMatrix::InnerIterator is the correct usage.