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

On Tue, Dec 1, 2009 at 4:35 PM, Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx> wrote:

Ok, it is also happening in the default branch (VC.net) but it is not happening with GCC 4.4 when running

./debug
./check notemp

Changing the implementation to

    PlainMatrixType eval() const
    {
      return PlainMatrixType(derived());
      //PlainMatrixType res(rows(), cols());
      //res.setZero();
      //derived().evalTo(res);
      //return res;
    }

solves the issue. What am I missing? Isn't that valid as well? I did not run all the unit tests...

- Hauke

On Tue, Dec 1, 2009 at 4:09 PM, Gael Guennebaud <gael.guennebaud@xxxxxxxxx> wrote:

hm I'm confused. Now I don't understand why this test does not fail with gcc in debug mode nor with with MSVC and the default branch since this issue is not related to your changes.

gael..

On Tue, Dec 1, 2009 at 3:21 PM, Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx> wrote:
It comes from

PlainMatrixType ProductBase::eval() const

since it is returning by value. I assume that in release mode this function gets inlined.

- Hauke

On Tue, Dec 1, 2009 at 3:10 PM, Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx> wrote:

Here are the unit test results.

a) Under Cygwin the tests 'stable_norm_2/3/4' are failing. They also fail in the default branch so it has nothing to do with the nesting.
b) Under Visual Studio, line 71 in product_notemporary.cpp fails in debug mode and in debug mode only. All other no-temporary tests pass.

This _expression_ here

m3 = m1 * m2.adjoint()

leads to 2 temporaries where only 1 temporary is expected according to the unit test. As I said, all other tests pass, i.e.

m3.noalias() = m1 * m2.adjoint()

leads to zero temporaries...

I did not yet have time to investigate the issue but it seems to be a minor problem.

- Hauke

On Tue, Dec 1, 2009 at 1:35 PM, Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx> wrote:

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.