Re: [eigen] Re: Student contribution: Pull Request

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

Hi,

this is indeed pretty though to follow. You should really consider
adding comments in the code to explain what is the purpose of each
piece of code. Which books/papers did you use as a reference? You
should add these references into the comments as well.  I'll look at
the algorithmic level later (need your references first), but here are
a few suggestions to make a better use of Eigen.

A loop like:

//compute U
for (int k=0; k<n; k++) {
  mU(k,i) = temp(k)/coeffU;
}

can be written as:

mU.col(j) = temp/coeffU;

There are many of them in specialSVD(). Note that I also removed the
one-line comment that is useless here as the code is self-explanatory
here. On the other hand I'd to know what specialSVD is doing. What are
its inputs? outputs? etc.


Another one:

// echange V columns
    temp = this->m_naiveV.col(I).segment(Zero, lastCol + shift);
    this->m_naiveV.col(I).segment(Zero, lastCol + shift) <<
this->m_naiveV.col(J).segment(Zero, lastCol + shift);
    this->m_naiveV.col(J).segment(Zero, lastCol + shift) << temp;

can be simplified as:

 this->m_naiveV.col(I).segment(Zero, lastCol + shift).swap(
this->m_naiveV.col(J).segment(Zero, lastCol + shift) );

When you work on a column (or row), use the col(.) method instead of
bock(....): it is faster and it makes it clearer you are working on a
part of a column and not on a general sub-matrix. E.g.:

this->m_naiveU.block(firstCol,firstCol,k + 1,1)

would become:

this->m_naiveU.col(k+1).segment(firstCol,firstCol)


Also be careful to the name of your variable. For instance, the
variable "lastNonNul" actually represents the number of non-zero
singular values.

cheers,
gael

On Wed, Jun 12, 2013 at 9:45 PM, BRUN Gauthier <brunga@xxxxxxxxxxxxxx> wrote:
>
> Hello,
> Our project is in progress, but we have to finish our work this friday.
> We would like to have advice about our work :
>
> There are three steps in our algorithm
> - Divide : implementation of the division and fusion
> - Deflation : during the fusion, we have to compute the SVD of a matrix M as
> big as the input matrix. The deflation step transforms M in order to apply
> the third step
> - Special SVD : do a SVD on the transformed M matrix (and only works on this
> one). this SVD is much faster than others SVD.
>
> The divide step is finished but works only on matrix defined with dynamic
> rows and columns, and we are currently debugging the two others steps.
> Also we are afraid that our code may be hard to read, so we would appreciate
> if you could check it (mainly the Divide and Deflation parts) and tell us
> what you think of it.
>
> Here are the links to our work: https://bitbucket.org/gl27/eigen
>
>
>
> Best regards,
> Team GL27
> Gauthier Brun, Nicolas Carre, Jean Ceccato, Pierre Zoppitelli
>
>