New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Before reporting a bug, please make sure that your Eigen version is up-to-date!
Bug 897 - IterativeSolver and UmfPack do not work with MappedSparseMatrix
Summary: IterativeSolver and UmfPack do not work with MappedSparseMatrix
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Sparse (show other bugs)
Version: 3.3 (current stable)
Hardware: All All
: High Crash
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on: 910
Blocks: 3.3
  Show dependency treegraph
 
Reported: 2014-10-21 15:15 UTC by Christoph Hertzberg
Modified: 2015-06-09 21:30 UTC (History)
2 users (show)



Attachments

Description Christoph Hertzberg 2014-10-21 15:15:17 UTC
The problem is that in IterativeSolverBase::compute(const MatrixType& A) the address of a temporary object is retrieved. Even worse, the temporary is an unnecessary `SparseMatrix` copy of the `MappedSparseMatrix`.

Maybe IterativeSolverBase::mp_matrix should be some kind of Ref<SparseMatrix> object (not supported yet, I guess?) and compute shall accept arbitrary Matrices, which are either copied or mapped into mp_matrix.

NB: The problem was exposed after new unit tests from bug 670.
Comment 1 Christoph Hertzberg 2014-10-21 15:55:44 UTC
Addendum: In general, all sparse solvers could accept SparseMatrixBase<...> directly in order to avoid unnecessary copies of MappedSparseMatrices. Some matrices still require copying, so some kind of Nested<> mechanism might be required. The latter is not too important, since performance impact of copying a matrix is usually rather small compared to decomposing.

Another note: For iterative solvers the name `compute` is a bit misleading (since no actual computation happens), but I guess we shall keep that name for consistency.
Comment 2 Gael Guennebaud 2014-10-21 17:17:57 UTC
Right, a Ref<SparseMatrix> is definitely what we need here.



Regrading the name 'compute', we kept it for consistency with other factorization based solver classes, and for iterative solvers you can read it as 'compute the pre-conditioner' which in most cases boils known to some kind of decompositions (incomplete factorization, diag+rest, L+D+U, etc.)
Comment 3 Christoph Hertzberg 2014-10-28 13:53:09 UTC
umfpack_support fails as well for mapped matrices, because by default it iteratively refines its result.
Comment 4 Gael Guennebaud 2014-12-02 14:01:55 UTC
Internally, UmfPackLU already used something like a Ref<SparseMatrix>, so fixing it properly was easy:

https://bitbucket.org/eigen/eigen/commits/8ba3b2cf2df8/
Changeset:   8ba3b2cf2df8
User:        ggael
Date:        2014-12-02 12:57:13+00:00
Summary:     Bug 897: fix UmfPack usage with mapped sparse matrices

3.2: https://bitbucket.org/eigen/eigen/commits/5ea731276380/
Comment 5 Gael Guennebaud 2015-02-09 11:43:48 UTC
Done for conjugate gradient and BiCGSTAB:

https://bitbucket.org/eigen/eigen/commits/6643f0989baa/

Still have to fix the unsupported ones.
Comment 6 Gael Guennebaud 2015-02-16 15:26:32 UTC
For the unsupported ones (*RES):

https://bitbucket.org/eigen/eigen/commits/aee8ffb
Comment 7 Christoph Hertzberg 2015-02-16 15:34:33 UTC
There is a little problem left when IterativeSolverBase is constructed by directly passing the matrix which is to be solved.
We could either allow Ref to be default-constructed (Referring to an empty matrix) -- that would also avoid the m_dummy member -- or just copy the behavior of the default constructor of IterativeSolverBase.
This is not covered by unit tests, but causes the documentation not to build, because this line causes an error:
https://bitbucket.org/eigen/eigen/src/b5be5e10eb/doc/snippets/BiCGSTAB_step_by_step.cpp?at=default#cl-5
Comment 8 Gael Guennebaud 2015-02-16 17:09:33 UTC
Fixed:

https://bitbucket.org/eigen/eigen/commits/0eee51587ab9/
Summary:     Bug 897: fix regression in BiCGSTAB(mat) ctor (an all other iterative solvers).
Add respective regression unit test.


If we strive to make Ref<> as close as possible to a C++ reference, then we should stick to the dirty 'dummy' trick.
Comment 9 Gael Guennebaud 2015-06-09 21:30:09 UTC
https://bitbucket.org/eigen/eigen/commits/a4ae626001bc/
Summary:     Bug 897: make umfpack support use Ref<>

Note You need to log in before you can comment on or make changes to this bug.