Re: [eigen] Propagating Max*AtCompileTime template parameters to decompositions

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

2010/3/4 Adolfo Rodríguez Tsouroukdissian <dofo79@xxxxxxxxx>:
>
>
> On Thu, Mar 4, 2010 at 6:00 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx>
> wrote:
>>
>> Supreme refinement is to add a unit test for that. We already have the
>> skeleton in the nomalloc test (test/nomalloc.cpp). Indeed, when
>> EIGEN_NO_MALLOC is defined, any heap allocation will cause an assert
>> failure, which in the context of unit tests we convert into an
>> exception.
>>
>> So don't hesitate to expand the nomalloc.cpp test, just with new
>> executable targets (CALL_SUBTEST_x for a new value of x up to 16) so
>> that no single executable gets too big. Testing only 'float' is good
>> enough.
>
>
> I was just thinking of that. I'll try to squeeze some time for that, too ;).
> Thanks for pointing me to the skeleton, that will speed up things. Should
> EIGEN_STACK_ALLOCATION_LIMIT 0 be defined as well, or am I good to go with
> only EIGEN_NO_MALLOC?.

#define EIGEN_STACK_ALLOCATION_LIMIT 0 is really important. Indeed, on
Linux, this is really a stack allocation hence bypasses malloc
completely. So if you don't define this to 0, you could have a unit
test that passes on Linux but fails (because a heap allocation
happens) on other platforms.

Benoit

> The tests I've been playing with were triggering
> assertions made by EIGEN_NO_MALLOC only.
>
>>
>> Benoit
>>
>> 2010/3/4 Benoit Jacob <jacob.benoit.1@xxxxxxxxx>:
>> > 2010/3/4 Adolfo Rodríguez Tsouroukdissian <dofo79@xxxxxxxxx>:
>> >>
>> >>
>> >> On Thu, Mar 4, 2010 at 5:25 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx>
>> >> wrote:
>> >>>
>> >>> Hi Adolfo,
>> >>>
>> >>> Many thanks for this patch. It's been sitting in the back of my mind
>> >>> nagging me for a while :) These fixes are definitely wanted.
>> >>>
>> >>> Just 2 things:
>> >>>
>> >>> 1. All the MatrixType:: is a bit heavy so could you please introduce
>> >>> enums like is being done in ColPivHouseholderQR.h, in other
>> >>> decompositions? So MatrixType::MaxBlabla becomes just MaxBlaBla.
>> >>
>> >> OK, no problem. The enum approach is definitely much cleaner. I just
>> >> didn't
>> >> want to impose it if it was not used in some files :)
>> >>
>> >> One more thing, now that I'm on it, are there any other files that I
>> >> might
>> >> have overlooked that could benefit from similar changes?
>> >
>> > Any decomposition is affected in the same way. Have you had a look at
>> > the Eigenvalues directory?
>> >
>> >
>> >>
>> >>>
>> >>> 2. You generated this patch with "diff" so it doesn't credit you, and
>> >>> you haven't written your commit msg. Could you please follow the
>> >>> instructions given there:
>> >>>
>> >>>
>> >>> http://eigen.tuxfamily.org/index.php?title=Developer%27s_Corner#Generating_a_patch
>> >>>
>> >>
>> >> Ack.
>> >>
>> >> Adolfo
>> >>
>> >>>
>> >>> Yes, this mailing list is the right place for such patches... the
>> >>> issue tracker is useful to report bugs without being immediately able
>> >>> to fix them.
>> >>>
>> >>> Benoit
>> >>>
>> >>>
>> >>> 2010/3/4 Adolfo Rodríguez Tsouroukdissian <dofo79@xxxxxxxxx>:
>> >>> > Hi all,
>> >>> >
>> >>> > I've been performing some tests with dynamic matrices that have a
>> >>> > compile-time max size, and found out that for most matrix
>> >>> > decompositions,
>> >>> > the MaxRowsAtCompileTime and MaxColsAtCompileTime template
>> >>> > parameters of
>> >>> > the
>> >>> > input matrix are not propagated to the decomposition's members and
>> >>> > temporaries. This leads to undesired and preventable heap
>> >>> > allocations.
>> >>> > The
>> >>> > following is a minimal example of what I mean:
>> >>> >
>> >>> > typedef Matrix<double, dynamic, dynamic, 0, 10, 10> Mat;
>> >>> > Mat A(Mat::Random(5, 5));
>> >>> >
>> >>> > FullPivLU<Mat> ldlt(A); // Allocates 4 temps
>> >>> > LDLT<Mat> ldlt(A);      // Allocates 3 temps
>> >>> >
>> >>> > I'm attaching a patch against the devel branch (commit
>> >>> > 2589:8a8f4e9e04bf)
>> >>> > that deals with this issue for the Cholesky, LU, QR and SVD modules.
>> >>> > Since
>> >>> > most of these modules are experimental and undergoing heavy work,
>> >>> > the
>> >>> > style
>> >>> > of the modifications changes form file to file (some use convenience
>> >>> > enums,
>> >>> > some not). However, I tried to stay consistent within the context of
>> >>> > each
>> >>> > file. Please let me know your thoughts on the matter.
>> >>>
>> >>> Yes so actually, please introduce enums everywhere :)
>> >>>
>> >>> > One very specific
>> >>> > note: in line 65 of HouseHolderQR.h I left the Options template
>> >>> > parameter
>> >>> > as-is, but don't know if it should be changed to that of MatrixType.
>> >>>
>> >>> Great catch! Please change that to Options (as defined on line 57).
>> >>>
>> >>> Thanks
>> >>> Benoit
>> >>>
>> >>> >
>> >>> > Cheers,
>> >>> >
>> >>> > Adolfo.
>> >>> >
>> >>> > P.S. Is there a better way to submit small patches, something like a
>> >>> > bugtracker, or is this the currently recommended way?
>> >>> >
>> >>> > --
>> >>> > Adolfo Rodríguez Tsouroukdissian, Ph. D.
>> >>> >
>> >>> > Robotics engineer
>> >>> > PAL ROBOTICS S.L
>> >>> > http://www.pal-robotics.com
>> >>> > Tel. +34.93.414.53.47
>> >>> > Fax.+34.93.209.11.09
>> >>> > AVISO DE CONFIDENCIALIDAD: Este mensaje y sus documentos adjuntos,
>> >>> > pueden
>> >>> > contener información privilegiada y/o confidencial que está dirigida
>> >>> > exclusivamente a su destinatario. Si usted recibe este mensaje y no
>> >>> > es
>> >>> > el
>> >>> > destinatario indicado, o el empleado encargado de su entrega a dicha
>> >>> > persona, por favor, notifíquelo inmediatamente y remita el mensaje
>> >>> > original
>> >>> > a la dirección de correo electrónico indicada. Cualquier copia, uso
>> >>> > o
>> >>> > distribución no autorizados de esta comunicación queda estrictamente
>> >>> > prohibida.
>> >>> >
>> >>> > CONFIDENTIALITY NOTICE: This e-mail and the accompanying document(s)
>> >>> > may
>> >>> > contain confidential information which is privileged and intended
>> >>> > only
>> >>> > for
>> >>> > the individual or entity to whom they are addressed.  If you are not
>> >>> > the
>> >>> > intended recipient, you are hereby notified that any disclosure,
>> >>> > copying,
>> >>> > distribution or use of this e-mail and/or accompanying document(s)
>> >>> > is
>> >>> > strictly prohibited.  If you have received this e-mail in error,
>> >>> > please
>> >>> > immediately notify the sender at the above e-mail address.
>> >>> >
>> >>>
>> >>>
>> >>
>> >>
>> >
>>
>>
>
>
>
> --
> Adolfo Rodríguez Tsouroukdissian, Ph. D.
>
> Robotics engineer
> PAL ROBOTICS S.L
> http://www.pal-robotics.com
> Tel. +34.93.414.53.47
> Fax.+34.93.209.11.09
> AVISO DE CONFIDENCIALIDAD: Este mensaje y sus documentos adjuntos, pueden
> contener información privilegiada y/o confidencial que está dirigida
> exclusivamente a su destinatario. Si usted recibe este mensaje y no es el
> destinatario indicado, o el empleado encargado de su entrega a dicha
> persona, por favor, notifíquelo inmediatamente y remita el mensaje original
> a la dirección de correo electrónico indicada. Cualquier copia, uso o
> distribución no autorizados de esta comunicación queda estrictamente
> prohibida.
>
> CONFIDENTIALITY NOTICE: This e-mail and the accompanying document(s) may
> contain confidential information which is privileged and intended only for
> the individual or entity to whom they are addressed.  If you are not the
> intended recipient, you are hereby notified that any disclosure, copying,
> distribution or use of this e-mail and/or accompanying document(s) is
> strictly prohibited.  If you have received this e-mail in error, please
> immediately notify the sender at the above e-mail address.
>