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

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?

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.
>