Created attachment 809 [details] Demonstrating that Q is applied incorrectly in some cases by using an identity RHS HouseholderQR and ColPivHouseholderQR apply Q incorrectly on the right side, and Q' incorrectly on the left side, for the case of complex numbers. The latter case impacts solving as well, because Eigen needs to calculate Q'*RHS. A simple test is to calculate the decomposition of a small matrix, and then apply it to the left and right of an identity matrix of the appropriate size. Both answers should be the same, but they are not. I've attached an example.
My proposed fix is https://bitbucket.org/eigen/eigen/pull-requests/355
The current behavior indeed can't be right. Most importantly the patch should include unit tests to prevent regressions (extent the tests in test/householder.cpp)
I finally pushed a fix (to the default branch only): https://bitbucket.org/eigen/eigen/commits/25b342390facc827 It does slightly change the behavior for several .apply*OnThe{Left,Right}() methods (only for complex scalars), so I'm not 100% sure if this should be backported to 3.3
Thank you for the fix. The documentation of setTrans()/trans() are now incorrect, we should really rename them setReverseFlag/reverseFlag(). Ideally we would keep setTrans()/trans() for compatibility with only a deprecation warning for real scalar types, and a compilation error for complex types, because in the later case the user code is likely broken.
I agree, we should rename those (and the m_trans member) -- alternative encode the transposing into the type. I'm not sure if we really need to keep the setTrans()/trans() methods. Those were protected members, so at least they were not easy to access. The public interface have been the .transpose()/.adjoint() methods for a while.
Renaming trans to reverseFlag pushed here: https://bitbucket.org/eigen/eigen/commits/6acd5a89a93cdc Btw, are these constructs necessary? typename internal::conditional<NumTraits<Scalar>::IsComplex, typename internal::remove_all<typename VectorsType::ConjugateReturnType>::type, VectorsType>::type, It seems that the same logic already happens inside `VectorsType::ConjugateReturnType` -- everything works fine with, e.g.: typedef HouseholderSequence< typename internal::remove_all<typename VectorsType::ConjugateReturnType>::type, typename internal::remove_all<typename CoeffsType::ConjugateReturnType>::type, Side > ConjugateReturnType;
-- GitLab Migration Automatic Message -- This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/1493.