Skip to content
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

[search] Convert "test" to benchmark #4718

Closed
kunaltyagi opened this issue Apr 22, 2021 · 10 comments
Closed

[search] Convert "test" to benchmark #4718

kunaltyagi opened this issue Apr 22, 2021 · 10 comments
Labels
good first issue Skills/areas of expertise needed to tackle the issue kind: todo Type of issue module: benchmarks

Comments

@kunaltyagi
Copy link
Member

This "test" doesn't really belong here?

It seems to capture processing times, but don't print or use them / compare them?

Originally posted by @larshg in #4712 (comment)

@kunaltyagi kunaltyagi added effort: high Rough estimate of time needed to fix/implement/solve kind: todo Type of issue labels Apr 22, 2021
@mvieth mvieth changed the title Convert "test" to benchmark (after benchmarks are added) [search] Convert "test" to benchmark Jun 30, 2021
@mvieth
Copy link
Member

mvieth commented Jun 30, 2021

For anyone interested: benchmarks are added now, you can orient yourself at the existing benchmark(s).

@kunaltyagi kunaltyagi added module: benchmarks good first issue Skills/areas of expertise needed to tackle the issue and removed effort: high Rough estimate of time needed to fix/implement/solve labels Jul 1, 2021
@kunaltyagi
Copy link
Member Author

Added a new label for the new module. Should have done that before :)

@Zekrom-7780
Copy link
Contributor

Zekrom-7780 commented Jan 17, 2022

Hey, @mvieth can I work on this issue?
Or is this like also closed?

@mvieth
Copy link
Member

mvieth commented Jan 17, 2022

@Zekrom-7780 Yes, you can work on this issue. Thank you!

@Zekrom-7780
Copy link
Contributor

@mvieth @kunaltyagi I AM SOO SORRY, for reaching THIS late, but is this Still open?
If yes, then I would still like to work on it

@mvieth
Copy link
Member

mvieth commented Oct 17, 2023

@Zekrom-7780 Sure, go ahead.

@Zekrom-7780
Copy link
Contributor

@mvieth , I finished setting up the repo, and I saw like an entire test folder, and a benchmarks folder. Could you tell me on how to proceed with this?

So sorry if this is a dumb question

@mvieth
Copy link
Member

mvieth commented Oct 19, 2023

The goal is to convert this test:

TEST (PCL, Organized_Neighbor_Search_Pointcloud_Neighbours_Within_Radius_Search_Benchmark_Test)

to a proper benchmark (like the other benchmarks in the benchmarks folder). Notice how the test measures the time needed by setInputCloud and radiusSearch. The brute force search that is also performed in the test is pretty much useless and can be discarded.

@Zekrom-7780
Copy link
Contributor

Thanks for the explanation @mvieth , I have made a pull request, could you please look into it?

Also, please could you tell me on how to get the build checks to pass, like which script do I have to run?

Also, as mentioned in the pull request, Soo Sorry that I wasn't able to rebase the multiple commits into a single one

@larshg
Copy link
Contributor

larshg commented Nov 25, 2023

Fixed by #5851

@larshg larshg closed this as completed Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Skills/areas of expertise needed to tackle the issue kind: todo Type of issue module: benchmarks
Projects
None yet
Development

No branches or pull requests

4 participants