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

Unoriented sphere der #134

Merged
merged 11 commits into from
Apr 11, 2024

Conversation

ThibaultLejemble
Copy link
Collaborator

This PR implements de differentiation of the UnorientedSphereFit.

  • the eigen solver and the matrix Q are now stored as class data members because they are needed by the UnorientedSphereDerImpl derived class
  • the test fit_unoriented uses the new differentiation but only checks the coherence of the results when normal vectors are swapped. I tried to check that the curvature values are correct, but I couldn't find an appropriate epsilon value that is small enough for the comparison to work...

@jmaleo
Copy link
Contributor

jmaleo commented Jan 25, 2024

I have conducted a series of experiments using the newly implemented derivative class, focusing on evaluating the coherence of results under different conditions of normal flipping.

The parameters employed in these tests included:

Three different ratios for normal flipping: 0.0, 0.25, and 0.5.

  • A variety of radii.
  • two iterations of MLS for each test scenario.
  • Three levels of noise applied to normals, acknowledging that higher noise levels typically degrade results. It's important to note that the same noise level was maintained across all flip ratio tests to ensure consistency in the comparative analysis.

Upon analysis, I observed that there were no difference in the outcomes across the various flip ratios.

@nmellado
Copy link
Contributor

Do you mean that your experiments validate the estimator (wrt. to those already implemented) ?

@ThibaultLejemble
Copy link
Collaborator Author

Do you mean that your experiments validate the estimator (wrt. to those already implemented) ?

I think @jmaleo was saying that he performed some extra tests that validates the invariance on normal directions.

@ThibaultLejemble
Copy link
Collaborator Author

We believe there is a small error in one of the mathematical equation, I will clarify this, so this is not ready for review and merging.

@nmellado
Copy link
Contributor

nmellado commented Apr 3, 2024

Shall we undraft it and review it ?

@ThibaultLejemble
Copy link
Collaborator Author

Shall we undraft it and review it ?

Yes, ready for review!

@ThibaultLejemble ThibaultLejemble marked this pull request as ready for review April 3, 2024 19:01
Ponca/src/Fitting/unorientedSphereFit.hpp Show resolved Hide resolved
Ponca/src/Fitting/unorientedSphereFit.hpp Show resolved Hide resolved
Ponca/src/Fitting/unorientedSphereFit.hpp Outdated Show resolved Hide resolved
Ponca/src/Fitting/unorientedSphereFit.hpp Outdated Show resolved Hide resolved
tests/src/fit_unoriented.cpp Show resolved Hide resolved
@nmellado
Copy link
Contributor

nmellado commented Apr 5, 2024

Sounds good, can you please clean the history ? then we will be good to go I think.

@nmellado nmellado merged commit e26a0e8 into poncateam:master Apr 11, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants