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

Plannerarena benchmarking #228

Merged
merged 3 commits into from Sep 22, 2016

Conversation

Projects
None yet
4 participants
@davetcoleman
Member

davetcoleman commented Sep 19, 2016

Supersedes #175 and addresses my code review in that PR

Be sure to merge (not squash merge) so that the three authors of this PR get credit. Also note that @ryanluna was the original author but is no longer involved with moveit.

I've tested the code locally and fixed up the documentation here: ros-planning/moveit_tutorials#14

@mamoll I tested uploading to your website, very cool!

@kirstyellis thanks for all your work in porting this to the new repo!

Should not be cherry-picked to indigo/jade because this is a total refactor to a package and removes another package (moveit_ros_benchmark_gui)

This was referenced Sep 19, 2016

@davetcoleman davetcoleman added this to the Kinetic Release milestone Sep 19, 2016

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 21, 2016

Member

note this has been reviewed by myself, @kirstyellis, and @sachinchitta and is very ready to be merged by a maintainer other than me

Member

davetcoleman commented Sep 21, 2016

note this has been reviewed by myself, @kirstyellis, and @sachinchitta and is very ready to be merged by a maintainer other than me

@v4hn

otherwise looks good to me

@@ -1,121 +0,0 @@
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This comment has been minimized.

@v4hn

v4hn Sep 22, 2016

Member

does this request intentionally not provide a CHANGELOG.rst?

@v4hn

v4hn Sep 22, 2016

Member

does this request intentionally not provide a CHANGELOG.rst?

This comment has been minimized.

@davetcoleman

davetcoleman Sep 22, 2016

Member

We have replaced the old benchmarks package with an entirely new one (code/method wise). I'm not sure if we would want to carry over the old CHANGELOG to the new package. I would vote no since the old CHANGELOG wouldn't have anything relevant to say about the new package and would probably just confuse users if it promises some new feature, etc

@davetcoleman

davetcoleman Sep 22, 2016

Member

We have replaced the old benchmarks package with an entirely new one (code/method wise). I'm not sure if we would want to carry over the old CHANGELOG to the new package. I would vote no since the old CHANGELOG wouldn't have anything relevant to say about the new package and would probably just confuse users if it promises some new feature, etc

Show outdated Hide outdated moveit_ros/benchmarks/CMakeLists.txt
roscpp
rosconsole
REQUIRED

This comment has been minimized.

@v4hn

v4hn Sep 22, 2016

Member

according to https://cmake.org/cmake/help/v3.6/command/find_package.html the REQUIRED should remain before the COMPONENTS tag.

@v4hn

v4hn Sep 22, 2016

Member

according to https://cmake.org/cmake/help/v3.6/command/find_package.html the REQUIRED should remain before the COMPONENTS tag.

This comment has been minimized.

@davetcoleman

davetcoleman Sep 22, 2016

Member

fixed

Show outdated Hide outdated moveit_ros/benchmarks/CMakeLists.txt
add_subdirectory(benchmarks)
install(DIRECTORY include/ DESTINATION ${CATKIN_PACKAGE_INCLUDE_DESTINATION})

This comment has been minimized.

@v4hn

v4hn Sep 22, 2016

Member

please respect http://moveit.ros.org/documentation/contributing/code.html .
This probably installs the headers to the wrong folder.

@v4hn

v4hn Sep 22, 2016

Member

please respect http://moveit.ros.org/documentation/contributing/code.html .
This probably installs the headers to the wrong folder.

This comment has been minimized.

@davetcoleman

davetcoleman Sep 22, 2016

Member

fixed

Code review fixup
Remove package benchmark_gui
clang-format Benchmarks package
@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 22, 2016

Member

Feedback addressed.

Member

davetcoleman commented Sep 22, 2016

Feedback addressed.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 22, 2016

Member

On Thu, Sep 22, 2016 at 08:40:28AM -0700, Dave Coleman wrote:

We have replaced the old benchmarks package with an entirely new one (code/method wise).
I'm not sure if we would want to carry over the old CHANGELOG to the new package.

I agree, it does not make sense to carry over the old file.
I was just wondering why there is no file at all anymore.
Will this be created with the next release?

Member

v4hn commented Sep 22, 2016

On Thu, Sep 22, 2016 at 08:40:28AM -0700, Dave Coleman wrote:

We have replaced the old benchmarks package with an entirely new one (code/method wise).
I'm not sure if we would want to carry over the old CHANGELOG to the new package.

I agree, it does not make sense to carry over the old file.
I was just wondering why there is no file at all anymore.
Will this be created with the next release?

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 22, 2016

Member

Yes, it is typically created during the release process.

Member

davetcoleman commented Sep 22, 2016

Yes, it is typically created during the release process.

@v4hn

v4hn approved these changes Sep 22, 2016

@v4hn v4hn merged commit 35d2fd3 into ros-planning:kinetic-devel Sep 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Sep 22, 2016

Member

ok, merged.

Member

v4hn commented Sep 22, 2016

ok, merged.

@davetcoleman davetcoleman deleted the davetcoleman:plannerarena_benchmarking branch Sep 22, 2016

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Sep 22, 2016

Member

Woohoo one year coming!

Member

davetcoleman commented Sep 22, 2016

Woohoo one year coming!

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