Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sphere fit eigensolver #112

Closed
ThibaultLejemble opened this issue Oct 6, 2023 · 1 comment · Fixed by #115
Closed

Sphere fit eigensolver #112

ThibaultLejemble opened this issue Oct 6, 2023 · 1 comment · Fixed by #115
Labels
bug Something isn't working

Comments

@ThibaultLejemble
Copy link
Collaborator

I believe there is an error in SphereFitImpl::finalize() when solving the eigenvalue problem:
https://github.com/poncateam/ponca/blob/d2cda8378fcc27c8f27be1f94bca85f90e35796a/Ponca/src/Fitting/sphereFit.hpp#L68C28-L76C28.

The vector u = [uc ul^T uq]^T of the sphere is the eigenvector associated with the minimal eigenvalue of the general eigenvalue problem Au = lambda Cu (in the code A = m_matA and C = matC).

The method computes M = inv(C) A to solve the standard eigenvalue problem Mu = lambda u by using the self-adjoint eigensolver on M^T M. However, I think that the eigenvectors of M and M^T M are not the same (they are if M is symmetric, which is not the case here).

I propose to use the Eigen::GeneralizedEigenSolver to solve Au = Bu and then take the real part of u (I think the imaginary part is null since A and C are symmetric).
I quickly tested it, and it gives visual results that are slightly better (when I look at points projected onto their locally fitted sphere).

@nmellado nmellado added the bug Something isn't working label Oct 6, 2023
@nmellado
Copy link
Contributor

nmellado commented Oct 6, 2023

Thanks a lot.
Seems to be fine.
To be able to review it, could you please add mathematical details in the user documentation associated to this function ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants