Convex and concave surfaces wrong principal component analysis

[fran6co]

Original PR #1565. I think this fix is a bit more important than the other one and should be included for the 1.8

[fran6co]

There is a big machine precision problem with the way computeMeanAndCovarianceMatrix works. For example:

-0.0688534  -0.179019   2.618
-0.0642632  -0.179019   2.618
-0.0734436  -0.174429   2.618
-0.0688534  -0.174429   2.618
-0.0642632  -0.174429   2.618
-0.0688534  -0.169838   2.618
-0.0642632  -0.169838   2.618
-0.0688534  -0.165248   2.618
-0.0642632  -0.165248   2.618
-0.0688534  -0.160658   2.618
-0.0642632  -0.160658   2.618
-0.0688534  -0.156068   2.618
-0.0642632  -0.156068   2.618
-0.0688534  -0.151477   2.618
-0.0642632  -0.151477   2.618

with computeMeanAndCovarianceMatrix calculates:

7.866123300*10^-6   3.933207600*10^-6   9.534479900*10^-10
3.933207600*10^-6   0.00008390877300    3.662929300*10^-9
9.534479900*10^-10  3.662929300*10^-9   1.146545400*10^-7

by using compute3DCentroid and computeCovarianceMatrixNormalized

7.8661*10^-6    3.93325*10^-6   0
3.93325*10^-6   0.0000839087    0
0               0               0

[fran6co]

And it gets worse when looking for eigen values:

1.1465429924929773e-07
7.6632244080314903e-06
8.4111674339878013e-05

should be:

0
7.6632007713667789e-06
8.4111565364906172e-05

and it's particularly bad when comparing the smallest to the biggest eigen value as the ratio goes from 0 to 0.00136311992

[SergioRAgostinho]

Thanks for this. Now I'm just wondering if the best solution is to adopt your changes or have a go at computeMeanAndCovarianceMatrix instead, which is the actual responsible for this problem.

[SergioRAgostinho]

So at first sight it looks like a precision issue in computeMeanAndCovarianceMatrix. In order to restrict the algorithm to a single pass, they used the analytical equivalent expression E[(X-E[X])(Y-E[Y])] = E[XY] - E[X]*E[Y] which is correct but has precision problems, even using double precision -_-. Just recalled this is the long lasting problem discussed in #560 and #1407.

Based on this comment what you proposed makes sense. Any particular reason for having removed the aligned allocation? I ask although being aware that compute3DCentroid and computeMeanAndCovarianceMatrix are not really using SIMD operations to their full possible extend.

Overall this LGTM 👍 Just do a double check on the spacings of the parenthesis. I've seen a bunch of them missing.

[fran6co]

@SergioRAgostinho No reason for the aligned, I'll add it back and fix the code style issues during this week.
Not sure if it would be better to actually change computeMeanAndCovarianceMatrix to do the slow but accurate way just for the sake of API and eventually remove computeMeanAndCovarianceMatrix or make it a lot more explicit that's not reliable and to use only when you know what you are doing.

[SergioRAgostinho]

I would prefer to adopt something like I described over here. Although we might still need to reach a consensus on what's the best way to do this and how to make the user aware and have control over it.

[fran6co]

@SergioRAgostinho rebased, added back the aligns and fixed the spaces.

[SergioRAgostinho]

Guys, any last comments on this with respect to what was discussed in #1407? If not, it's good to merge.