Make omp naming and functionality consistent. #6293

larshg · 2025-06-13T20:26:17Z

No description provided.

larshg · 2025-06-13T20:31:23Z

features/include/pcl/features/intensity_gradient.h

-      ///number of threads to be used, default 0 (auto)
-      unsigned int threads_{0};
+      ///number of threads to be used
+      unsigned int num_threads_{1};


This defaulted to 0, ie. automatic number of threads, but now it defaults to 1 (as majority of the algorithms did) - but I guess we don't have any rule here, whether or not to include it?

Copilot

Pull Request Overview

This PR standardizes the OpenMP thread‐count parameter name to num_threads, updates default values to 1, adds auto‐detection of hardware threads when requests are zero, and replaces all threads_ references in pragmas with num_threads_.

Renamed threads_ to num_threads_ and updated constructors/signatures.
Changed setNumberOfThreads logic to use num_threads and omp_get_num_procs().
Updated all OpenMP pragmas to use num_threads(num_threads_).

Reviewed Changes

Copilot reviewed 65 out of 65 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
filters/include/pcl/filters/pyramid.h	Renamed and rewrote `setNumberOfThreads`, member renaming
filters/include/pcl/filters/impl/pyramid.hpp	Updated OMP pragmas to use `num_threads_`
filters/include/pcl/filters/impl/fast_bilateral_omp.hpp	Renamed parameter, updated logic and pragmas
filters/include/pcl/filters/impl/convolution_3d.hpp	Updated OMP pragmas
filters/include/pcl/filters/impl/convolution.hpp	Updated OMP pragmas
filters/include/pcl/filters/fast_bilateral_omp.h	Renamed constructor and parameter
filters/include/pcl/filters/convolution_3d.h	Renamed and rewrote `setNumberOfThreads`, member renaming
filters/include/pcl/filters/convolution.h	Renamed and rewrote `setNumberOfThreads`, member renaming
features/include/pcl/features/shot_omp.h	Renamed constructor and parameter, member renaming
features/include/pcl/features/shot_lrf_omp.h	Renamed parameter and member
features/include/pcl/features/principal_curvatures.h	Renamed constructor and parameter, member renaming
features/include/pcl/features/normal_3d_omp.h	Renamed constructor and parameter, member renaming
features/include/pcl/features/intensity_gradient.h	Renamed and rewrote `setNumberOfThreads`, member renaming
features/include/pcl/features/impl/shot_omp.hpp	Updated calls and pragmas to use `num_threads_`
features/include/pcl/features/impl/shot_lrf_omp.hpp	Updated parameter logic and pragmas
features/include/pcl/features/impl/principal_curvatures.hpp	Updated `setNumberOfThreads` logic and pragmas
features/include/pcl/features/impl/normal_3d_omp.hpp	Updated `setNumberOfThreads` logic and pragmas
features/include/pcl/features/impl/intensity_gradient.hpp	Removed old detection logic, updated pragmas
features/include/pcl/features/impl/fpfh_omp.hpp	Updated `setNumberOfThreads` logic and pragmas
features/include/pcl/features/fpfh_omp.h	Renamed constructor and parameter, member renaming

Copilot · 2025-06-13T20:50:33Z

filters/include/pcl/filters/pyramid.h

+#ifdef _OPENMP
+          num_threads_ = num_threads != 0 ? num_threads : omp_get_num_procs();
+#else
+          if (num_threads_ != 1) {


The warning condition checks num_threads_ (the member) rather than the requested num_threads parameter. It should warn when the user-requested num_threads is not 1.

Suggested change

if (num_threads_ != 1) {

if (num_threads != 1) {

Copilot · 2025-06-13T20:50:33Z

filters/include/pcl/filters/pyramid.h

@@ -155,7 +165,7 @@ namespace pcl
        /// Threshold distance between adjacent points
        float threshold_{0.01f};
        /// \brief number of threads
-        unsigned int threads_{0};
+        unsigned int num_threads_{1};


Defaulting num_threads_ to 1 changes the previous auto-detection behavior when setNumberOfThreads(0) was never called. Consider initializing to 0 or calling setNumberOfThreads(0) in the constructor to preserve auto-detect.

Suggested change

unsigned int num_threads_{1};

unsigned int num_threads_{0};

Copilot · 2025-06-13T20:50:33Z

filters/include/pcl/filters/pyramid.h

          */
        inline void
-        setNumberOfThreads (unsigned int nr_threads = 0) { threads_ = nr_threads; }
+        setNumberOfThreads(unsigned int num_threads = 0)


[nitpick] The setNumberOfThreads logic is duplicated across many classes. Consider extracting it into a shared helper or macro to reduce code repetition and ease future updates.

mvieth · 2025-06-14T12:20:24Z

An idea: how about, instead of having unsigned int num_threads_{1}; in so many classes, we move that to PCLBase? That would have the additional benefit that there is no ABI break every time parallelization is added to a class (like in the two recent PRs).

As a second step, we could then discuss also moving setNumberOfThreads to PCLBase (to reduce code duplication), however that would make it less clear which classes actually use this setting.

larshg · 2025-06-16T18:33:41Z

An idea: how about, instead of having unsigned int num_threads_{1}; in so many classes, we move that to PCLBase? That would have the additional benefit that there is no ABI break every time parallelization is added to a class (like in the two recent PRs).

As a second step, we could then discuss also moving setNumberOfThreads to PCLBase (to reduce code duplication), however that would make it less clear which classes actually use this setting.

Yeah, It seems PCLBase is used for all algorithms, so could make sense. I'm not sure how we should do it with setNumberOfThreads. Not sure if it its enough, just documenting, that not all classes make use of the set value.

larshg · 2025-06-16T20:48:18Z

Should we default to using omp_get_num_procs() threads instead of 1 - I would expect code to be multithreaded by default 😄

mvieth · 2025-06-17T07:50:05Z

I'm not sure how we should do it with setNumberOfThreads. Not sure if it its enough, just documenting, that not all classes make use of the set value.

I am also fine with setNumberOfThreads staying in each parallelized class.

Should we default to using omp_get_num_procs() threads instead of 1 - I would expect code to be multithreaded by default 😄

Sounds good to me. How are you planning to do that? Calling setNumberOfThreads(0) in each constructor?

larshg · 2025-06-17T12:05:41Z

Should SampleConsensus inherit from PCLBase as well?

The I/O Imagegrabber also has parallelizing, but should they also inherit from PCLBase?
The Brieft from PCLBase:
PCL base class. Implements methods that are used by most PCL algorithms.

So I guess I/O shouldn't, but SampleConsesus could(Should)?

mvieth · 2025-06-17T14:59:38Z

Should SampleConsensus inherit from PCLBase as well?

The I/O Imagegrabber also has parallelizing, but should they also inherit from PCLBase? The Brieft from PCLBase: PCL base class. Implements methods that are used by most PCL algorithms.

So I guess I/O shouldn't, but SampleConsesus could(Should)?

I think I would not change any inheritance. SampleConsensus and the image grabber can then simply keep their own num_threads_ member.

Set number of threads using omp_get_num_procs in initCompute.

…gle thread for the test.

larshg requested a review from Copilot June 13, 2025 20:26

This comment was marked as outdated.

Sign in to view

larshg commented Jun 13, 2025

View reviewed changes

larshg requested a review from Copilot June 13, 2025 20:44

This comment was marked as outdated.

Sign in to view

larshg force-pushed the fixomp branch from 8eadf4d to 86ee90e Compare June 13, 2025 20:48

larshg requested a review from Copilot June 13, 2025 20:48

Copilot AI reviewed Jun 13, 2025

View reviewed changes

larshg added the changelog: behavior change Meta-information for changelog generation label Jun 13, 2025

larshg requested a review from mvieth June 13, 2025 21:33

larshg force-pushed the fixomp branch 4 times, most recently from b054dd8 to 808f253 Compare June 14, 2025 09:19

Make omp naming and functionality consistent.

1441409

larshg force-pushed the fixomp branch from 808f253 to 1441409 Compare June 14, 2025 11:18

larshg force-pushed the fixomp branch 4 times, most recently from 5a216b8 to 9679064 Compare June 17, 2025 14:58

larshg force-pushed the fixomp branch 4 times, most recently from c072a2f to fb4af32 Compare June 19, 2025 07:30

larshg added this to the pcl-1.16.0 milestone Jun 19, 2025

larshg force-pushed the fixomp branch 2 times, most recently from b225008 to 656b4eb Compare June 19, 2025 10:38

larshg added 2 commits June 19, 2025 16:29

Move num_threads_ to pcl_base.

bc839c3

Set number of threads using omp_get_num_procs in initCompute.

Convolution doens't inherit nothing?

8a92f5b

larshg force-pushed the fixomp branch 2 times, most recently from ad60cbf to d1f3bcf Compare June 19, 2025 18:57

larshg added 4 commits June 19, 2025 21:36

Add more usings.

7c6e3f1

Rename setThreads to setNumberOfThreads.

35e7430

Poisson4 returns different vertices when parallelized, so keep to sin…

7cfe6d7

…gle thread for the test.

Pyramid doesn't inherit PCLBase.

2b81291

larshg force-pushed the fixomp branch from d1f3bcf to 2b81291 Compare June 19, 2025 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Make omp naming and functionality consistent. #6293

Make omp naming and functionality consistent. #6293

Uh oh!

larshg commented Jun 13, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

larshg Jun 13, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jun 13, 2025

Uh oh!

Copilot AI Jun 13, 2025

Uh oh!

Copilot AI Jun 13, 2025

Uh oh!

mvieth commented Jun 14, 2025

Uh oh!

larshg commented Jun 16, 2025

Uh oh!

larshg commented Jun 16, 2025

Uh oh!

mvieth commented Jun 17, 2025

Uh oh!

larshg commented Jun 17, 2025

Uh oh!

mvieth commented Jun 17, 2025

Uh oh!

Uh oh!

Uh oh!

Make omp naming and functionality consistent. #6293

Are you sure you want to change the base?

Make omp naming and functionality consistent. #6293

Uh oh!

Conversation

larshg commented Jun 13, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

larshg Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

mvieth commented Jun 14, 2025

Uh oh!

larshg commented Jun 16, 2025

Uh oh!

larshg commented Jun 16, 2025

Uh oh!

mvieth commented Jun 17, 2025

Uh oh!

larshg commented Jun 17, 2025

Uh oh!

mvieth commented Jun 17, 2025

Uh oh!

Uh oh!