gabmoreira / maks

Motion Averaging
MIT License
55 stars 5 forks source link

PardisoSimplicialLDLT vs Eigen::SimplicialLDLT #4

Closed rjanvier closed 2 years ago

rjanvier commented 2 years ago

Hello, thank you for making your code public it's indeed the fastest rotation averaging method I know. I have a small question regarding the use of MKL solver vs. Eigen solver. I have adapted your code by replacing PardisoLDLT solver by what I think is an Eigen equivalent (Eigen::SimplicalLDLT). Eigen solver is ok on small size datasets. But apart for the speed difference (MKL solver if of course faster) I notice your code with Eigen solver plugin in produce wrong results on some medium to big datasets (lot of rotations putted at identity). So my questions are : have you already tested your code with Eigen solvers ? Have you choosen Pardiso only for speed or are there some accuracy difference ? is there anything that need to be tuned to make it work with Eigen ? thank in advance !

rjanvier commented 2 years ago

Ok I found the culprit when using Eigen solver

gabmoreira commented 2 years ago

Hi, Thanks for the feedback. The choice between MKL Pardiso and Eigen::SimplicalLDLT was purely based on performance since the code was initially implemented on an i7 and there was a considerable difference between the two. Regarding accuracy, it works best in sparse graphs with a moderate noise level in the rotations (smallest 3 eigenvalues of the Rotations Laplacian close to zero).

You may want to try different eigenvalue targets (sigma) of the symKrylovSchurLDL function, which is set at -1e-6. Tweaking this may lead to instability in the LDL decomposition in some cases, however.

rjanvier commented 2 years ago

Hi, thanks for your answer it is very appreciated! I made a cmake file for this project, taking advantage of "modern and robust" detection of recent version of MKL (the versions with the "oneAPI" branding) that fallback to Eigen::SimplicialLDLT if MKL is not found. I think it could be a good way for scientists and practitioners to test MAKS if they do not want to mess with MKL. Are you willing to integrate this to the repository?