Re: [eigen] Geometry module - Quaternion fitting alternative

[Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx>]

Thank you for the detailed review.

The points covered by your review:
- KDE comment is gone
- RealScalar has been introduced for the statistics normalization
- Code is bound to identical floating point types, i.e. all casts are gone
- Introduced squaredNorm instead of dot()
- Introduced ei_isMuchSmallerThan for the rank computation
- Introduced coef instead of operator()

Additional changes:
- Added missing preproc guard against multiple includes
- Changed the expression/template helper names to be Eigen conform ei_...
- Added a specialized version for complex numbers that does nothing but
  issue a compile time error when using it
- Added doxygen guard for the template helpers in the anonymous namespace
- Added doxygen guard for the complex specialization
- Improved the return type deduction - it should now return minimal sized
  transformations
- Improved to docs (added ingroup stuff)
- Modified doc/Doxyfile.in to support the amsmath package
- Added fixed size tests to the unit test

The modified doxygen stuff allows to use latex environments like
'align' and adds support for matrix, bmatrix, etc. environments. The
attached screenshot shows the docs.

> +  for (int i=0; i<n; ++i)
> +  {
> +    src_mean += src.col(i).cast<Scalar>();
> +    dst_mean += dst.col(i).cast<Scalar>();
> +  }
>
> OK, this is a rowwise().sum() but you didn't want to #include Array
> just for that, right? makes sense.

Here it would be interesting to know, just performance wise, whether
looping twice over the data yields better performance than doing it
just once. It probably depends if the vectorization can process four
scalars at once or just two.

> +  for (int i=0; i<n; ++i)
> +  {
> +    src_demean.col(i) = src.col(i).cast<Scalar>() - src_mean;
> +    dst_demean.col(i) = dst.col(i).cast<Scalar>() - dst_mean;
> +  }
> again i understand that you prefer to avoid including Array.

You can really do this with colwise? This is a vector valued operation
not scalar valued - I thought colwise only supports scalar valued
operations.

By the way, did you realize that I did not add the file to the
unsupported directory but dared to put it right into the Geometry
module? Is that ok as is?

Also, just out of curiosity, is dot() slower than squaredNorm(). My
assumption was that it should vectorize as well as squaredNorm() does.

Regarding the complex support I tried to make the two helper methods
from the unit test generic in the sense that they can now create
random matrices belonging to U(n) and SU(n) but the actual algorithm
has a few lines left for which I am unsure on how to handle the
complex case. Also, since I do not fully understand what this type of
transformation means in the complex domain I think I am not the right
person to adapt the code. Currently, the usage of complex matrices
will result in a compile time assert.

In case you or anybody else wants to check the code once more from an
existing repository you need to do:

hg qpop -a // unapply all patches
cd .hg/patches
hg pull // retrieve the new patches
cd ../..
hg qpush -a // reapply the updated patches

In case it is a fresh checkout just do:

hg qclone https://hauke@xxxxxxxxxxxxx/hauke/umeyama-integration/ eigen_umeyama
cd eigen_umeyama
hg qpush -a

Cheers,
Hauke

Attachment: umeyama_docs.jpg
Description: JPEG image