Skip to content

[SAMPLE_CONSENSUS] Refactor Ellipse3D optimization kernel to internal namespace#6430

Merged
mvieth merged 1 commit intoPointCloudLibrary:masterfrom
AlrIsmail:refactor-ellipse3d-levmarq-to-internal
May 1, 2026
Merged

[SAMPLE_CONSENSUS] Refactor Ellipse3D optimization kernel to internal namespace#6430
mvieth merged 1 commit intoPointCloudLibrary:masterfrom
AlrIsmail:refactor-ellipse3d-levmarq-to-internal

Conversation

@AlrIsmail
Copy link
Copy Markdown
Contributor

@AlrIsmail AlrIsmail commented Apr 19, 2026

Summary
Refactor SampleConsensusModelEllipse3D to move the heavy Eigen::LevenbergMarquardt optimization
kernel into a non-templated source file. See #6428 for more.

Technical Changes

  • Encapsulated LevenbergMarquardt in pcl::internal::optimizeModelCoefficientsEllipse3D.
  • Math helpers moved to pcl::internal as inline header functions for performance.
  • Added explicit template instantiations for core point types in the source file.

@AlrIsmail AlrIsmail force-pushed the refactor-ellipse3d-levmarq-to-internal branch from ea89929 to 21bcec1 Compare April 19, 2026 19:39
Copy link
Copy Markdown
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some first review comments, I will probably hove more comments later.
Another question: you claim ~24% reduction in build time, what is that number based on? In the two clang build analyzer reports, total build time of the baseline is 105.3s+197.6s=302.9s (frontend+backend), while afterwards it is 108.7s+198.6s=307.3s (so actually more?). Looking only at the build time of sac_model_ellipse3d.cpp, for the baseline it is 40.3s (again frontend+backend), afterwards it is 37.9s, so about 2.5s or 6% difference.

Comment thread sample_consensus/src/sac_model_ellipse3d.cpp
Comment thread sample_consensus/src/sac_model_ellipse3d.cpp Outdated
Comment thread sample_consensus/include/pcl/sample_consensus/sac_model_ellipse3d.h Outdated
Comment thread sample_consensus/include/pcl/sample_consensus/sac_model_ellipse3d.h Outdated
Comment thread sample_consensus/include/pcl/sample_consensus/impl/sac_model_ellipse3d.hpp Outdated
@AlrIsmail
Copy link
Copy Markdown
Contributor Author

Some first review comments, I will probably hove more comments later. Another question: you claim ~24% reduction in build time, what is that number based on? In the two clang build analyzer reports, total build time of the baseline is 105.3s+197.6s=302.9s (frontend+backend), while afterwards it is 108.7s+198.6s=307.3s (so actually more?). Looking only at the build time of sac_model_ellipse3d.cpp, for the baseline it is 40.3s (again frontend+backend), afterwards it is 37.9s, so about 2.5s or 6% difference.

Sorry for the misattribution; the 24% is actually based on wall-clock rebuild time for the pcl_sample_consensus target.

Target: pcl_sample_consensus
Command: time make pcl_sample_consensus -j8
----
Before Changes:
 real    0m23.671s
 user    0m22.813s
 sys     0m1.097s

After Changes:
 real    0m17.473s
 user    0m16.644s
 sys     0m0.996s

While the serial aggregate in the reports is flat, this change reduced LevenbergMarquardt instantiations from 61 down
to 44

@AlrIsmail AlrIsmail force-pushed the refactor-ellipse3d-levmarq-to-internal branch from 21bcec1 to 45a5300 Compare April 21, 2026 23:00
@AlrIsmail
Copy link
Copy Markdown
Contributor Author

AlrIsmail commented Apr 21, 2026

new values after taking into account review comments:
analysis_baseline.txt
analysis_optimized.txt

Metric Category Pre-Optimization Post-Optimization Impact / Gain
Build: Total Time (File) 36,294 ms 34,051 ms -6.2% Reduction
Template Instantiations 61 instantiations 44 instantiations -27.8% Reduction
Runtime 1,684.2 μs 1,532.6 μs +9.0% Speedup

@AlrIsmail AlrIsmail force-pushed the refactor-ellipse3d-levmarq-to-internal branch from 45a5300 to 70a08ba Compare April 22, 2026 20:54
@AlrIsmail
Copy link
Copy Markdown
Contributor Author

@mvieth, I've undone any unrelated modifications as requested (except for loops, since I get clang-tidy errors). Could you check?

Comment thread sample_consensus/include/pcl/sample_consensus/sac_model_ellipse3d.h
Comment thread sample_consensus/include/pcl/sample_consensus/sac_model_ellipse3d.h
Comment thread sample_consensus/src/sac_model_ellipse3d.cpp Outdated
Comment thread sample_consensus/src/sac_model_ellipse3d.cpp Outdated
Comment thread sample_consensus/include/pcl/sample_consensus/sac_model_ellipse3d.h
Comment thread sample_consensus/include/pcl/sample_consensus/impl/sac_model_ellipse3d.hpp Outdated
@AlrIsmail AlrIsmail force-pushed the refactor-ellipse3d-levmarq-to-internal branch 5 times, most recently from 72b88b0 to f01f2c4 Compare April 29, 2026 19:36
@azure-pipelines
Copy link
Copy Markdown
Contributor

Commenter does not have sufficient privileges for PR 6430 in repo PointCloudLibrary/pcl

…ntiations

Move the Levenberg-Marquardt optimization logic into a non-templated
pcl::internal::optimizeModelCoefficientsEllipse3D function in the
source file, reducing redundant template instantiations from 61 to 44.
This results in a ~7% reduction in build time and 10% runtime speedup
for the pcl_sample_consensus library.

Move the get_ellipse_point, dvec2ellipse, and golden_section_search
math helpers into pcl::internal in the header to allow shared access
without exposing them as class members.

Additionally:
- Fix projectPoints to write to projected_points[inlier] instead of
  projected_points[i++] when copy_data_fields is true, consistent with
  other SAC models
- Fix modernize-return-braced-init-list lint warning
@AlrIsmail AlrIsmail force-pushed the refactor-ellipse3d-levmarq-to-internal branch from f01f2c4 to 4ddd51a Compare April 29, 2026 22:08
@AlrIsmail AlrIsmail requested a review from mvieth May 1, 2026 10:54
Copy link
Copy Markdown
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I am happy with the changes. Thanks!

@larshg I am going to leave this PR open a few more days, so you have the opportunity to review again, if you want to. If you don't want to or don't have the time, I would merge by end of next week.

@mvieth mvieth added changelog: enhancement Meta-information for changelog generation module: sample_consensus labels May 1, 2026
Copy link
Copy Markdown
Contributor

@larshg larshg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@mvieth mvieth merged commit ae58fbb into PointCloudLibrary:master May 1, 2026
13 checks passed
@mvieth
Copy link
Copy Markdown
Member

mvieth commented May 3, 2026

@AlrIsmail BTW, after looking at the clang build analyzer output, I believe modifying this line might also improve build time and run time: the currently used constructor internally calls compute, which requires the instantiation of several expensive templates. If we switch to a different constructor and call computeDirect instead, that may improve things. Though it may be necessary to switch the solver from float to double, to stay accurate enough. See also the documentation of SelfAdjointEigenSolver. I just wanted to mention it, in case you are interested in testing how this change would influence build time and run time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: enhancement Meta-information for changelog generation module: sample_consensus

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants