[features] change SHOT estimation to a dynamic schedule#6400
[features] change SHOT estimation to a dynamic schedule#6400larshg merged 4 commits intoPointCloudLibrary:masterfrom
Conversation
benchmarks/CMakeLists.txt
Outdated
|
|
||
| PCL_ADD_BENCHMARK(features_shot FILES features/shot.cpp | ||
| LINK_WITH pcl_io pcl_search pcl_features | ||
| ARGUMENTS "${PCL_SOURCE_DIR}/test/milk.pcd") |
There was a problem hiding this comment.
Lets used milk_cartoon_all_small_clorox instead? Its a full kinect scan with 300.000 points with color?
There was a problem hiding this comment.
milk.pcd is only a supset of the one I mention, with 13700 points. Still more than bun0.pcd(about 400 pts), but not that much still really.
|
@larshg I tried |
There was a problem hiding this comment.
Pull request overview
This PR changes SHOT (Signature of Histograms of OrienTations) feature estimation to use OpenMP dynamic scheduling instead of the default static scheduling, addressing issue #5785 which identifies that loops with varying iteration times (like neighbor searches) can benefit from dynamic scheduling. The PR also adds benchmarks to measure the performance impact of different chunk sizes.
Changes:
- Added configurable chunk_size parameter to SHOTEstimationOMP and SHOTColorEstimationOMP constructors for dynamic scheduling
- Modified OpenMP pragmas to use
schedule(dynamic, chunk_size_)instead of default static scheduling - Added comprehensive benchmarks for both SHOT352 and SHOT1344 descriptors with varying chunk sizes
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| features/include/pcl/features/shot_omp.h | Added chunk_size parameter to constructors and member variables for both SHOTEstimationOMP and SHOTColorEstimationOMP classes |
| features/include/pcl/features/impl/shot_omp.hpp | Updated OpenMP parallel for directives to use dynamic scheduling with configurable chunk size |
| benchmarks/features/shot.cpp | New benchmark file testing SHOT352 and SHOT1344 estimation with different chunk sizes (64, 128, 256) |
| benchmarks/CMakeLists.txt | Added features_shot benchmark to the build system |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * \param nr_threads the number of hardware threads to use (0 sets the value back to automatic) | ||
| * \param chunk_size PCL will use dynamic scheduling with this chunk size. Setting it too low will lead to more parallelization overhead. Setting it too high will lead to a worse balancing between the threads. | ||
| */ | ||
| explicit SHOTEstimationOMP (unsigned int nr_threads = 0, int chunk_size = 256) |
There was a problem hiding this comment.
The explicit keyword is inconsistent with similar OMP feature estimation classes in the codebase. Other classes like NormalEstimationOMP (normal_3d_omp.h:77) and PrincipalCurvaturesEstimation (principal_curvatures.h:82) do not use explicit for their constructors with default parameters. For consistency with the established pattern, consider removing explicit from this constructor.
There was a problem hiding this comment.
@kai-waang Is there a reason why you made this constructor explicit?
There was a problem hiding this comment.
Considering potential implicit conversions, I made this constructor as explicit. However, given that many constructors in the PCL library do not use explicit (as Copilot discovered), it might be better to follow PCL's conventions. @mvieth
There was a problem hiding this comment.
I was only wondering if there is a specific implicit conversion you are worried about? Like do you have an example what could go wrong without explicit? Otherwise I think it would indeed be better to remove explicit, for consistency.
There was a problem hiding this comment.
Yes, I think we should remove this explicit. And I couldn't actually come up with an example where implicit conversion would be used unexpectedly.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
mvieth
added
changelog: enhancement
3 participants
As discussed in #5785, change SHOT estimation to a dynamic schedule. Benchmarks for SHOT estimation is also added.