Convert test to benchmark (radius search) #5851

Zekrom-7780 · 2023-10-19T21:51:58Z

feature: Added benchmark for OrganizedNeighborSearch

Converted the existing test code for OrganizedNeighborSearch into a proper benchmark.
#4718 (comment) This benchmark measures the time needed for setInputCloud and radiusSearch operations with an organized point cloud. The brute force search part has been removed as it is not needed.

Should solve Issue 4718

Soo Sorry that I wasn't able to rebase the multiple commits into a single one

…issue_4718

mvieth · 2023-10-21T16:18:12Z

Please remove the file .vscode/c_cpp_properties.json from this pull request
To fix the formatting problems, you can either click on the failing check to reach the log on Azure Pipelines (see "Show diff"), or use this script, or run make format on Linux (not sure whether this works on Windows too?)
Please remove the test in test_organized_index.cpp then
Please add it to benchmarks/CMakeLists.txt so that the benchmark is actually built
I would suggest to use benchmarks/search/radius_search.cpp instead of benchmarks/organized_index/organized_index.cpp

Zekrom-7780 · 2023-10-24T01:59:06Z

@mvieth, I've made the changes you suggested in the previous comments:

I removed the test in test_organized_index.cpp.
I changed the name of the benchmark to benchmark/search/radius_search.cpp.
I ran make format after navigating to the .dev directory (cd .dev).
I made the changes in the benchmarks/CMakeLists.txt

The strange thing is that build formatting ran successfully for c16fd5c, but the regular build failed because I hadn't pushed the change in the benchmark's name (benchmarks/organized_index/organized_index.cpp instead of benchmarks/search/radius_search.cpp). I understand the issue from looking at Azure Pipelines, as you suggested me to do.

However, in the subsequent commit (8834f04) and the latest one, the build formatting failed again.

I tried running different shell commands mentioned in the comments of this script like
sh ./.dev/format.sh /usr/bin/clang-format ./, but none of them resolved the issue.

I apologize for any inconvenience related to the multiple commits. I also apologize asking so many doubts in a
good-first-issue . Can you please provide some guidance or suggestions on how to resolve the formatting issue in the latest commits?

Thank you for your assistance.

mvieth · 2023-10-24T09:28:49Z

I am not sure why make format or running the formatting script does not work. Are there any error messages?
If nothing else works, you can always click on the failing Formatting Check code formatting, navigate to Azure Pipelines, and finally select the Show diff stage under Check code formatting. This is what it currently shows.
You also added .dev/format? I don't know whether you copied or renamed that locally, but you need to remove .dev/format again from this pull request. I strongly suggest to not add all local changes to git before you do a commit, but instead to add file by file so you can choose what you commit.

Zekrom-7780 · 2023-10-26T05:44:29Z

@mvieth , I RAN the make_format code PROPERLY NOW, ./format.sh /usr/bin/clang-format /home/user/pcl, in the /pcl/.dev directory, and the code ran for an infinite loop saying :-

Error reading /home/user/pcl/.clang-format: Invalid argument
YAML:47:22: error: unknown key 'CaseSensitive'
    CaseSensitive:   true
                     ^~~~

So, I commented this line, and ran this code, it modified multiple files EXCEPT THE CHANGED .LANG_FORMAT, and I pushed the modified files, but still, the formatting code did not run.

Could you suggest on how to fix this error, as by commenting it, the build_formatting didn't run

mvieth · 2023-10-26T12:17:38Z

Which version of clang-format are you using? You need at least version 12, because older versions do not understand CaseSensitive. In fact, I would recommend to use version 14 because that is what the formatting check is using, and older versions have made some formatting mistakes that are fixed in version 14.
If you look at the log of the failing formatting check, you will see that it complains about changes in some other files that you pushed (e.g. apps/in_hand_scanner/src/integration.cpp), but has no complains about benchmarks/search/radius_search.cpp.
Let me recommend again to check what you add/stage (and thus which changes you include in the commit). Adding all changes in all files is pretty much never a good idea. This article might be interesting for you: https://github.com/git-guides/git-add

mvieth · 2023-11-18T14:45:51Z

@Zekrom-7780 Hi, I just wanted to ask whether you are still working on this pull request? Was my last comment helpful?

Zekrom-7780 · 2023-11-18T15:24:11Z

@mvieth , I'm working on this issue, my college became pretty hectic for the last month
Installing clang-format-14 , and making the changes
Sorry for the delay.

Zekrom-7780 · 2023-11-18T15:38:55Z

@mvieth , the changes are made, updated clang-format, and right now the formatting check code has passed!

So Sorry for all the trouble!
Can I take another issue up as well?
Since I've understood how to do this?

mvieth

Can I take another issue up as well?
Since I've understood how to do this?

Sure, go ahead. If you would like my opinion on which issues are high priority, I am happy to suggest some 🙂

mvieth · 2023-11-20T13:03:40Z

benchmarks/CMakeLists.txt

@@ -25,3 +25,8 @@ PCL_ADD_BENCHMARK(filters_voxel_grid FILES filters/voxel_grid.cpp
                  ARGUMENTS "${PCL_SOURCE_DIR}/test/table_scene_mug_stereo_textured.pcd"
                            "${PCL_SOURCE_DIR}/test/milk_cartoon_all_small_clorox.pcd")

+PCL_ADD_BENCHMARK(search_radius_search FILES search/radius_search.cpp
+                  LINK_WITH pcl_io pcl_search pcl_features


Suggested change

LINK_WITH pcl_io pcl_search pcl_features

LINK_WITH pcl_io pcl_search

Linking the features lib should not be necessary here?

mvieth · 2023-11-20T13:22:14Z

benchmarks/search/radius_search.cpp

+    std::vector<int> k_indices;
+    std::vector<float> k_sqr_distances;
+
+    auto check_time = benchmark::Now();


The compiler doesn't know benchmark::Now(). Unless you have a better idea, I would suggest to simply use the default/automatic way and measure the duration of all functions in the for loop.

larshg · 2023-11-20T18:05:49Z

benchmarks/search/radius_search.cpp

+  for (auto _ : state) {
+    organizedNeighborSearch.setInputCloud(cloudIn);
+
+    int randomIdx = rand() % cloudIn->size();


Wouldn't it make sense to use a set of fixed indexes and radiies?
Not sure if these rands can make it difficult to compare etc?

Yeah, that is a good point. Random indices would at least add more noise in the time measurements, so that more iterations are necessary to reach a stable result. Alternatively, we could simply use every point from the input cloud once as the search point, and a fixed radius for all searches, e.g. 0.1 or 0.05?

Zekrom-7780 · 2023-11-21T18:18:54Z

Thanks a lot for the suggestions @mvieth @larshg !
Will do the implementations and send the changes in 3-4 hours

Also @mvieth , could you direct me to the issues with high priority ??

mvieth · 2023-11-22T09:53:26Z

Here are some issues that have, in my opinion, a nice and clear task description:

include header file qvtk_compatibility.h doesn't include protection #5752
Fix extra semicolon warnings #5640
ModelOutlierRemoval Documentation references non-existent member function setFilterLimitsNegative #2460
Replace header guards of type #ifndef with a #pragma once #3774
[Proposal] New tutorial for "How to install PCL for windows x64 Visual Studio 2019" #4462 (only if you have experience with Windows and the PCL all-in-one installer)

Zekrom-7780 · 2023-11-22T16:36:42Z

@mvieth @larshg , what should I do in this PR?
All the checks have passed here

mvieth

I am fine with the proposed changes. Thanks!

what should I do in this PR?
All the checks have passed here

If Lars has any additional suggestions or requests, please address them, otherwise you are done here 😄 We will merge this pull request when we both approved

Zekrom-7780 added 5 commits October 20, 2023 02:29

Initial_Commit_Creating_Benchmark_from_test

2b1cfb5

Merge branch 'PointCloudLibrary:master' into issue_4718

00bf3f6

Changes

2ef04e6

Merge branch 'issue_4718' of https://github.com/Zekrom-7780/pcl into …

9393bd8

…issue_4718

Changes_2

1a4c6c3

Zekrom-7780 mentioned this pull request Oct 19, 2023

[search] Convert "test" to benchmark #4718

Closed

mvieth added the module: benchmarks label Oct 21, 2023

Zekrom-7780 and others added 5 commits October 24, 2023 00:37

Merge branch 'PointCloudLibrary:master' into issue_4718

821edc0

Deleted_vscode

2b8b083

Ran_make_format

344cd98

Ran_make_format

c16fd5c

Pushing_changed_file_name

8834f04

Bhavesh Misra and others added 3 commits October 26, 2023 10:06

Deleted_format

5209c84

Merge branch 'PointCloudLibrary:master' into issue_4718

c8a8e8e

Ran_make_format

3a40dbc

Merge branch 'PointCloudLibrary:master' into issue_4718

514fc1d

Zekrom-7780 added 2 commits November 18, 2023 21:05

Updated clang_format_14, and unmade_the_changes

c63d620

Merge branch 'PointCloudLibrary:master' into issue_4718

55fb003

mvieth reviewed Nov 20, 2023

View reviewed changes

larshg reviewed Nov 20, 2023

View reviewed changes

Merge branch 'PointCloudLibrary:master' into issue_4718

62121b5

Zekrom-7780 added 2 commits November 22, 2023 03:28

Using_the_chrono_now_way

c34a277

Ran_format.sh

7b4926d

mvieth approved these changes Nov 24, 2023

View reviewed changes

larshg approved these changes Nov 25, 2023

View reviewed changes

larshg merged commit 50c4064 into PointCloudLibrary:master Nov 25, 2023
13 checks passed

mvieth changed the title ~~Issue 4718~~ Convert test to benchmark (radius search) Nov 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert test to benchmark (radius search) #5851

Convert test to benchmark (radius search) #5851

Zekrom-7780 commented Oct 19, 2023

mvieth commented Oct 21, 2023

Zekrom-7780 commented Oct 24, 2023

mvieth commented Oct 24, 2023 •

edited

Loading

Zekrom-7780 commented Oct 26, 2023

mvieth commented Oct 26, 2023

mvieth commented Nov 18, 2023

Zekrom-7780 commented Nov 18, 2023

Zekrom-7780 commented Nov 18, 2023

mvieth left a comment

mvieth Nov 20, 2023

mvieth Nov 20, 2023

larshg Nov 20, 2023

mvieth Nov 21, 2023

Zekrom-7780 commented Nov 21, 2023

mvieth commented Nov 22, 2023

Zekrom-7780 commented Nov 22, 2023

mvieth left a comment

	LINK_WITH pcl_io pcl_search pcl_features
	LINK_WITH pcl_io pcl_search

Convert test to benchmark (radius search) #5851

Convert test to benchmark (radius search) #5851

Conversation

Zekrom-7780 commented Oct 19, 2023

mvieth commented Oct 21, 2023

Zekrom-7780 commented Oct 24, 2023

mvieth commented Oct 24, 2023 • edited Loading

Zekrom-7780 commented Oct 26, 2023

mvieth commented Oct 26, 2023

mvieth commented Nov 18, 2023

Zekrom-7780 commented Nov 18, 2023

Zekrom-7780 commented Nov 18, 2023

mvieth left a comment

Choose a reason for hiding this comment

mvieth Nov 20, 2023

Choose a reason for hiding this comment

mvieth Nov 20, 2023

Choose a reason for hiding this comment

larshg Nov 20, 2023

Choose a reason for hiding this comment

mvieth Nov 21, 2023

Choose a reason for hiding this comment

Zekrom-7780 commented Nov 21, 2023

mvieth commented Nov 22, 2023

Zekrom-7780 commented Nov 22, 2023

mvieth left a comment

Choose a reason for hiding this comment

mvieth commented Oct 24, 2023 •

edited

Loading