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

[Benoit Jacob <jacob.benoit.1@xxxxxxxxx>]

> 16 new changesets in eigen:

\o/

My hero !!!!

Seriously, it's been a while since the last time that Eigen's
expression templates received such a fundamental improvement!

Benoit

2009/12/2 Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx>:
> like this?
>
> https://bitbucket.org/hauke/nesting-refactoring/changeset/eb8160b3e202/
>
> now i am really curious how the mergin will work out...
>
> - hauke
>
> On Wed, Dec 2, 2009 at 10:36 AM, Gael Guennebaud <gael.guennebaud@xxxxxxxxx>
> wrote:
>>
>>
>> On Wed, Dec 2, 2009 at 10:29 AM, Gael Guennebaud
>> <gael.guennebaud@xxxxxxxxx> wrote:
>>>
>>> ok good. So now make sure the nestByValue() function and NestByValue
>>> class are still present for backward compatibility
>>
>> only for Core, not for the Sparse module.
>>
>> gael.
>>
>>
>>>
>>> (I'll move them to a separate module when I'll merge my fork). Then you
>>> can simply merge your fork in the last default branch and push!
>>>
>>> gael.
>>>
>>> On Wed, Dec 2, 2009 at 9:13 AM, Hauke Heibel
>>> <hauke.heibel@xxxxxxxxxxxxxx> wrote:
>>>>
>>>> After removing the method I am getting the following results for the
>>>> unit tests under windows (vc9 sp1, 32bit):
>>>>
>>>> Release
>>>> -------
>>>> 99% tests passed, 2 tests failed out of 331
>>>> Total Test time (real) = 298.55 sec
>>>> The following tests FAILED:
>>>>  168 - stable_norm_2 (Failed)
>>>>  169 - stable_norm_3 (Failed)
>>>> Errors while running CTest
>>>> Project : error PRJ0019: A tool returned an error code from "Performing
>>>> Post-Build Event..."
>>>> Build Time 4:58
>>>>
>>>> Debug
>>>> -----
>>>> 100% tests passed, 0 tests failed out of 331
>>>> Total Test time (real) = 1400.43 sec
>>>> Build Time 23:20
>>>>
>>>> To me it looks good. How shall we proceed?
>>>>
>>>> - Hauke
>>>>
>>>> On Tue, Dec 1, 2009 at 4:45 PM, Gael Guennebaud
>>>> <gael.guennebaud@xxxxxxxxx> wrote:
>>>>>
>>>>> ok, actually this function can be entirely removed. I guess I added it
>>>>> for debug purpose.
>>>>>
>>>>> gael.
>>>>>
>>>>> 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.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>
>