Minor enhancements to custom LDLT implementations

[peddie]

This change is in support of PIC.

[akleeman]

What happens at the moment if you do:

Eigen::MatrixXd a, b;
block_diag.solve(a.cwiseProduct(b));

I would have guessed it'd find the solve(Matrix<_Scalar, _Rows, _Cols>) call here and do effectively the same thing as these new methods?

What if you change Matrix<...> to MatrixBase<...>?

[peddie]

MatrixBase<Derived> doesn't cover CWiseBinaryOp for sure. But I think it could cover Block and SparseMatrix.

[peddie]

There are a couple problems here. One is that we have to declare a type for output. So if we use MatrixBase<Derived>, then if you pass it a CWiseBinaryOp, it tries to declare one and assign to it. This is clearly no good as CWiseBinaryOps are never lvalues. But how else can we determine the return type so as to preserve sparsity? We don't care about that for rcond(), but we do care about it (I think) in PIC.

[peddie]

The Right Thing to do here is to make BlockDiagonalLDLT cooperate with the Eigen expression system.

I think I'm going to just make it return MatrixXd for now. But also I was wrong above that MatrixBase covers Block and SparseMatrix.

[peddie]

This change is up, which allows removing only the overload for CWiseBinaryOp.

[akleeman]

what happens if you do; solve(rhs_orig.eval())?

[peddie]

I can test it, but I don't think forcing an expression type is enough here.

[peddie]

Thanks for the careful review. I wrote this code in mad prototyping mode and failed to go back through some of these small patches in detail. I'll rework this.

[peddie]

This should be good to go if you are happy with the rewrite.

[akleeman]

There must be a way to do this via Eigen types such that if the input is static sized the output will be too? Eigen::Matrix<_Scalar, _Rows, _Cols> would have allowed for that, but Eigen::MatrixXd will force all output to be dynamic doubles. I don't think it's a problem for us, and feel free to proceed without changing this, it just feels like there should be a way, Eigen::DenseBase<Derived>::PlainMatrix or something like that?

[peddie]

I think the problem is not all expression types involved here derive from even DenseBase.

[akleeman]

MatrixBase inherits from DenseBase so it should right? https://gitlab.com/libeigen/eigen/-/blob/master/Eigen/src/Core/MatrixBase.h#L52

[peddie]

Yes, but this gets used with things other than a MatrixBase.

[akleeman]

I guess I meant that if the input argument is a MatrixBase<Derived> then maybe you could get the PlainMatrix from it?

I doubt this would work but if Eigen::MatrixBase<Derived> works, then Eigen::DenseBase<Derived> should right?

template <typename Derived>
Eigen::DenseBase<Derived>::PlainMatrix BlockDiagonalLDLT::solve(const Eigen::MatrixBase<Derived> &rhs) const {

Anyway everytime I dig into Eigen I start to think I understand how to extend it, then I find something like Solve.h and get thoroughly lost. This isn't important enough to spend the time figuring it out.

[peddie]

Sorry . . . I think the long-term "right way" is to make our solve() members look just like Eigen's ones and only overload _solve_impl() or whatever it is that takes an RhsType reference.