Skip to content

[registration] Document that setEuclideanFitnessEpsilon is a relative MSE threshold - #6501

Open
ump45nose wants to merge 1 commit into
PointCloudLibrary:masterfrom
ump45nose:docs/euclidean-fitness-epsilon
Open

ump45nose wants to merge 1 commit into
PointCloudLibrary:masterfrom
ump45nose:docs/euclidean-fitness-epsilon

Conversation

@ump45nose

Copy link
Copy Markdown
Contributor

Fixes #5868.

As discussed in the issue, the doc of Registration::setEuclideanFitnessEpsilon says it is the "maximum allowed Euclidean error between two consecutive steps", but that is not how it is used. IterativeClosestPoint::computeTransformation (also used by IterativeClosestPointNonLinear) and JointIterativeClosestPoint pass it to DefaultConvergenceCriteria::setRelativeMSE, so convergence is detected when

|MSE_current - MSE_previous| / MSE_previous < epsilon

where the MSE is the mean of the correspondence distances (squared Euclidean distances with the default correspondence estimation). So e.g. 0.01 means "the MSE changed by less than 1%", not "the error is below 0.01".

Changes (documentation only):

  • registration.h: reword the setEuclideanFitnessEpsilon / getEuclideanFitnessEpsilon docs to describe the relative MSE change, which classes use it this way, and that other registration methods may use it differently or ignore it (e.g. GICP has its own loop, FRICP documents that it is not evaluated).
  • icp.h, icp_nl.h: the third termination criterion in the class docs said "the sum of Euclidean squared errors is smaller than a user defined threshold"; changed it to the relative change of the MSE, and updated the matching comment in the ICP usage example.

Not changed: the usage example in icp.h still calls setEuclideanFitnessEpsilon (1). With a relative threshold, 1 is very loose (almost any iteration counts as converged on the MSE criterion), so maybe a smaller value would be a better example, but I left the code as is. The modeler app's parameter tooltip (apps/modeler/src/icp_registration_worker.cpp) uses the old wording too; I can update it here if you'd like.

Checked: the three headers are unchanged by clang-format 14 (registration is in the format whitelist), and a small program including icp.h, icp_nl.h and joint_icp.h compiles with GCC.

I used an AI assistant (Grok Bot) to help trace how the value is used and to draft this change; I checked the code paths and the wording myself.

ICP, ICP-NL and JointICP pass euclidean_fitness_epsilon_ to
DefaultConvergenceCriteria::setRelativeMSE, so it is compared against the
relative change of the correspondence MSE between two iterations, not an
absolute Euclidean error. Update the setter/getter docs and the
termination criteria lists in icp.h and icp_nl.h accordingly.

Fixes PointCloudLibrary#5868.

This branch has not been deployed

No deployments
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.

Documentation about Registration::setEuclideanFitnessEpsilon() misleading

1 participant