Re: [eigen] Unifying decomposition interfaces

[Benoit Jacob <jacob.benoit.1@xxxxxxxxx>]

OK, I finally had time to review your branch....

first of all it was very simple to merge your named branch: i just
cloned your repo, and in my clone i did:
hg merge "Decomposition Interface Refactoring"
hg ci

(that said, for other people, for future reference: don't use named
branches for development! do a separate clone for every separate
project).

that's all -- i was then able to just do "hg export tip" and examine the diff.

It looks very good; the only remaining issue is that after all,
checking the objects size != 0 is /not/ a viable way of checking
whether they are initialized... because it only applies to
dynamic-size matrices. For example,

     /** \returns the coefficients of the diagonal matrix D */
-    inline Diagonal<MatrixType,0> vectorD(void) const { return
m_matrix.diagonal(); }
+    inline Diagonal<MatrixType,0> vectorD(void) const
+    {
+      ei_assert(m_matrix.size() > 0 && "LDLT is not initialized.");
+      return m_matrix.diagonal();
+    }

here, if m_matrix is fixed-size, size() always returns the same
positive constant, even if m_matrix was uninitialized.

So there's no way around adding bool m_initialized in general. On
second thought, this also makes code easier to read.

(Side note: our unit tests for lu and qr currently only test
dynamic-size, that's bad)

Another small remark is that the default constructors you added need
to have some (even very short) documentation. It's just scary for the
user when an item is undocumented.

OK then it's great to see that you fixed a few quirks in passing like
the unused m_originalMatrix in PartialLU, and how you extended the
unit tests.

So when the remaining issue is fixed, feel free to push!

Benoit