-
Notifications
You must be signed in to change notification settings - Fork 13
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
fix pcl version 1.10 compatibility with ndt #190
Conversation
I performed some comparison runs using translation epsilons of 0.001 and 0.0001 respectively. Both gave similar over all NDT score values. The first value required fewer iterations overall and hit the max less often while the second value more often hit the max. Both ran within reasonable time frames though the second value did go above 100ms more frequently. Since I did not see a noticeable improvement in NDT score, I plan to continue with 0.001 for now as it stayed under the 100ms execution time target more reliably. However, I have left a comment describing that 0.0001 can work as documentation of its feasibility. |
|
||
static double trans_eps = 0.001; // Transformation epsilon. In PCLv1.10 (ros noetic) this value is squared error not base epsilon | ||
// NOTE: A value of 0.0001 can work as well. | ||
// This will increase the required iteration count (and therefore execution time) but might increase performance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, the mentioned performance here is in terms of accuracy? So the tradeoff is speed vs accuracy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll clarify
|
||
if (_method_type == MethodType::PCL_GENERIC) | ||
if (_method_type == MethodType::PCL_GENERIC) { | ||
ROS_INFO_STREAM("Using tramslation threshold of " << trans_eps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo? tramslation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
PR Details
Description
This PR resolves usdot-fhwa-stol/carma-platform#1559 by updating the NDT optimization parameters to reflect the new PCL 1.9+ API changes.
ROS Noetic targets PCL v 1.10 whereas ROS Kinetic target PCL v 1.7.2
It seems that in v1.9 the API for the error epsilons was changed (see PointCloudLibrary/pcl#1724) Now the translation epsilon is actually a squared error and there is an additional epsilon which is the rotation angle error in an axis-angle rotation representation. This PR updates the translation error setting in NDT to therefore be 0.001 which makes the optimization selectivity closer to the old value (TODO I'm still evaluating if 0.001 or 0.0001 (which is 0.01*0.01) is better) (No longer investigating, see comment below). The rotation error is computed as 1.0-translation error which is the default used technique in the ICP matching algorithm (seems they didn't add a default for NDT matching). This may seem strange but actually makes sense as the input is cos(angle error). And arccos(0.999) =0.044 rad or 2.5 deg error.
This PR also removes the --sequential build argument from the docker image to hopefully speed up build times.
Related Issue
usdot-fhwa-stol/carma-platform#1559
Motivation and Context
Stable localization in Elise
How Has This Been Tested?
Initial testing conducted on Blue lexus shows good results.
Types of changes
Checklist:
CARMA Contributing Guide