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

PCA, build and namespace Fixes #32

Merged
merged 8 commits into from
Dec 9, 2016
Merged

PCA, build and namespace Fixes #32

merged 8 commits into from
Dec 9, 2016

Conversation

geoffreychiou
Copy link
Contributor

@geoffreychiou geoffreychiou commented Nov 14, 2016

@akgoins Please review.

  1. Fixed some problems to allow for building with catkin tools.
  2. @akgoins Fixed some bugs with PCA and added unit tests in sphere_discretization.cpp
  3. Removed the use of using namespace to make the code easier to debug.

@jontromanab
Copy link
Contributor

@akgoins and @geoffreychiou I am working on the #11
Is anybody else is working on that? Should I continue or wait for your updates?

@geoffreychiou
Copy link
Contributor Author

@jontromanab I have not started working on it yet, if you have already started please continue, I can find something else.

  • Geoffrey

@jontromanab
Copy link
Contributor

@geoffreychiou I am currently working on #11 #30 and #16
Hope you understand that the time limit was really small. That's why the codes are all messed up. Now its time to make them better.
You are doing an wonderful job. Keep up the good work.

@akgoins
Copy link
Contributor

akgoins commented Nov 22, 2016

@geoffreychiou, Sorry, but merging the other PR has broken this one. Please resolve conflict and I will review as soon as possible.

@geoffreychiou
Copy link
Contributor Author

@akgoins I have resolved the conflict, please check whenever you have some free time.

  • Geoff



catkin_add_gtest(utest test/utest.cpp)
target_link_libraries(utest sphere_discretization ${catkin_LIBRARIES})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the catkin tests should be enclosed with the if statement below so the the gtest does not build every time.

if(CATKIN_ENABLE_TESTING)
  catkin_add_gtest(utest test/utest.cpp)
  target_link_libraries(utest sphere_discretization ${catkin_LIBRARIES})
endif()

kinematics::Kinematics k;
std::string ext = ".h5";
std::string filename =
std::string(k.getRobotName()) + "_" + "r" + str(boost::format("%d") % resolution) + "_" + "reachability" + "." + "h5";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest using boost::format for everything. I have not tested it, but it should work.

str(boost::format("%s_r%d_reachability.h5" % k.getRobotName() % resolution));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to move the ')' inside to get it to build.

str(boost::format("%s_r%d_reachability.h5") % k.getRobotName() % resolution);

Thanks for the suggestions.

@akgoins
Copy link
Contributor

akgoins commented Dec 9, 2016

@geoffreychiou, if you could make the two changes that Levi mentioned, I will merge this PR.

@geoffreychiou
Copy link
Contributor Author

@akgoins, I have added the changes Levi suggested.

@akgoins
Copy link
Contributor

akgoins commented Dec 9, 2016

+1

@akgoins akgoins merged commit 140a3e5 into ros-industrial-attic:master Dec 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants