Re: [eigen] Nesting by reference of by value ?

[Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx>]

First, thanks for the info. 

Second, its done! :) In the fork, all occurences of NestByValue and .nestByValue() and their sparse counterparts have been removed. Only ei_nested is remaining and will since we rely on it.

Most of the code should be clean and ready to merge. Currently, I am doing a full rebuild on the unit tests on Cygwin (GCC 4.4) and afterwards under Windows - it might take a while but since almost all intermediate commits have been checked I am confident. I will get back to you when I can confirm that everything is working.

- Hauke

On Tue, Dec 1, 2009 at 11:24 AM, Gael Guennebaud <gael.guennebaud@xxxxxxxxx> wrote:

This is a bit complicated to explain. Let's pick the following example:

sparse_mat = sparse_mat * diagonal_mat

This product can be efficiently implemented in term of scalar multiple operation. Therefore SparseDiagonalProduct::InnerIterator inherits CwiseUnaryOp::InnerIterator, which itself inherits SparseMatrix::InnerIterator (because the nested _expression_ type is SparseMatrix), which itself has to store a reference to the SparseMatrix object (actually for optimization purpose it directly stores a pointer to data of the sparse object, and it is reasonnable to assume that such an object with large data is valid troughout the lifetime of the _expression_). Since CwiseUnaryOp::InnerIterator requires a CwiseUnaryOp to be constructed, in the ctor of SparseDiagonalProduct::InnerIterator we have to construct a temporary CwiseUnaryOp object. If SparseMatrix is nested by value, then this CwiseUnaryOp object create a temporary SparseMatrix copy of the initial sparse matrix. This temporary is then referenced by the most nested iterator. However, this temporary object is immediately deleted leading to invalid pointers.... As I said, it's a bit tricky.

gael.

On Tue, Dec 1, 2009 at 9:00 AM, Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx> wrote:

On a second thought I am confused. Why would code crash, when objects are stored by value? Normally storing by reference involves the possibility of crashing your app (using  ref's to temporaries) - why should that happen to sparse matrices? As a matter of fact, as I wrote in the comments of the ei_ref_selector for Matrices, storing references to Matrices as opposed to copies is considered to be an optimization strategy - it should not be required.

Can you enlighten me?

- Hauke

On Tue, Dec 1, 2009 at 6:22 AM, Gael Guennebaud <gael.guennebaud@xxxxxxxxx> wrote:

Hi,

this is because ei_ref_selector was not specialized for sparse matrix types. Problem fixed in your fork.

gael.

On Mon, Nov 30, 2009 at 9:08 PM, Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx> wrote:

Hi,

I wanted to attack NestByValue today. First, I fixed the unit tests and then I created a clean fork and finally I found out, that the current implementation as I have it in the fork (https://bitbucket.org/hauke/nesting-refactoring/) is causing the sparse_product unit tests to fail.

Gael, since you've already played with it, could you please take a look? It seems to have todo with

  SparseMatrix& operator=(const SparseMatrixBase<OtherDerived>& other)

and there

  typedef typename ei_nested<OtherDerived,2>::type OtherCopy;

So far it seems as if Eigen::SparseTranspose<class Eigen::SparseMatrix<double,0> > must be nested by reference.

I have no experience with the sparse part of Eigen and any help would be appreciated.

- Hauke

On Fri, Nov 20, 2009 at 1:32 PM, Gael Guennebaud <gael.guennebaud@xxxxxxxxx> wrote:

On Fri, Nov 20, 2009 at 1:20 PM, Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx> wrote:

On Wed, Nov 18, 2009 at 7:35 PM, Gael Guennebaud <gael.guennebaud@xxxxxxxxx> wrote:

Now it would be interesting to bench MSVC as well since it seems this compiler has more difficulties to manage Eigen's code, but this is something I cannot do.

When that were done and we'ld agree upon using nesting by value the next step would probably be cleaning up the locations where NestByValue is used but not required anymore, right?

yes and basically the idea is to completely remove the NestByValue class. Well, actually we will move it to the Eigen2Support module once I merged my fork. So currently we still need to keep it in Core.

gael.