Adds osqp and qpOASES solver interfaces #40

arocchi · 2018-11-09T02:28:54Z

Adds osqp and qpOASES in the list of available solvers.
Automatically runs all tests that use a convex solver with all the list of solvers detected as available on the machine.
Disabled test calling solver with a non positive semi-definite matrix.
Please notice this needs a good refactor to simplify code in common between osqp and qpOASES solver, but at the moment there are issues with memory alignment that can cause weird errors whenever a serious refactor is attempted. All Eigen instances possibly causing alignment issues in the tesseract/trajopt stack should be reviewed.

Please review @Levi-Armstrong.

… matrix

Notice that while this change seems trivial, it actually causes tests to pass. Since bpmpd interface is quite brittle, it was the case that using it as a third option in tests caused some of them to fail. This means the order of execution of tests has an influence on the solver, which is a bad sign.

Levi-Armstrong · 2018-11-10T04:44:11Z

@arocchi This is awesome and thank you for the contribution. I am on vacation next week and will try to get it reviewed this weekend.

Levi-Armstrong · 2018-11-10T04:47:28Z

Also I submitted PR #42 that should fix the eigen aligned issues. For some reason I thought this was no longer need. It is possible that I did not catch all the places where it is need. I am unable to recreate the issue @mpowelson was having with the examples.

Levi-Armstrong · 2018-11-10T05:01:06Z

I gave it a brief glance. I always hate asking because it seems nitpicky, but I plan to eventually migrate the style to using the ros cpp sytle guide so with new addition I would like to follow the ros style guide if that is not to painful. To help with formatting the repository has s a clang file.

Also I notice the use of raw pointers a few places would it be good to replace them with shared pointers?

arocchi · 2018-11-10T22:40:03Z

perfect, I'm on it!

Notice this should be reviewed after bpmpd is removed and all memory alignment problems are resolved. In fact, right now the solver is occasionally instantiated twice in each solve cycle: this makes test pass.

arocchi · 2018-11-11T02:49:41Z

refactored to take into account clang & roscpp guidelines. Doxygen is still lacking, but after we fix memory alignment issues and the solvers are refactored, cleanup + doxygen will follow

Levi-Armstrong · 2018-11-16T16:48:18Z

@arocchi In search for the Eigen Issues I did some cleaning in trajopt packages and found one more structure without the Eigen Macro. I am unable to recreate the issue on my machine but let me know if you still have the issue if you test with PR #43.

arocchi · 2018-11-16T18:51:50Z

Thanks @Levi-Armstrong - I will check it later today and get back to you.

…vers_upstream

…rs_upstream

…e, qpoases_interface

arocchi · 2018-11-20T21:47:49Z

Tried code on new tesseract and trajopt, and find the issue (on my side that was causing the problem)
The new solver_utils work and I am in the process of refactoring the osqp and qpoases solver interfaces using them

arocchi · 2018-11-22T00:55:16Z

@Levi-Armstrong @bhaskara @mpowelson please review. This refactors the new solvers using a shared set of solver_utils which is fairly clean, tested and allows to quickly add new solvers beyond qpOASES and osqp. Notice it contains #43.

Levi-Armstrong · 2018-11-27T22:01:46Z

@arocchi Do you mind re-basing on the latest and I will review it tomorrow?

arocchi · 2018-11-28T04:47:02Z

will do!

arocchi · 2018-11-28T17:16:43Z

@Levi-Armstrong I merged instead of rebasing, hope this is ok. Let me know otherwise! It is good to review. I added a new functionality to specify a solver in JSONs / ProblemConstructionInfo, I think this should need more testing. Please go ahead with the review, I will address comments and come up with more unit testing for this feature as I receive the review.

Levi-Armstrong · 2018-11-28T23:59:58Z

There still appears to be conflicts.

…vers_upstream

arocchi · 2018-11-29T00:54:58Z

As new branches are merged, conflicts appear. In particular, the last one was over new-lines. Fixed now.

trajopt_sco/include/trajopt_sco/expr_ops.hpp

trajopt_sco/src/osqp_interface.cpp

trajopt_sco/src/solver_interface.cpp

trajopt_utils/src/logging.cpp

trajopt_sco/include/trajopt_sco/osqp_interface.hpp

trajopt_sco/include/trajopt_sco/qpoases_interface.hpp

Levi-Armstrong · 2018-11-29T02:05:28Z

I am also looking into if we should use Factory style approach for the solvers.

Levi-Armstrong · 2018-11-29T02:06:10Z

Overall looks great just a few minor things.

bhaskara

A couple of high level comments, which feel free to ignore as I'm not very familiar with this codebase.

trajopt_sco/include/trajopt_sco/solver_interface.hpp

trajopt_sco/src/osqp_interface.cpp

bhaskara · 2018-11-29T23:18:43Z

trajopt_sco/include/trajopt_sco/solver_interface.hpp

@@ -148,5 +149,53 @@ std::ostream& operator<<(std::ostream&, const Cnt&);
 std::ostream& operator<<(std::ostream&, const AffExpr&);
 std::ostream& operator<<(std::ostream&, const QuadExpr&);

-ModelPtr createModel();
+class ConvexSolver


Do we actually need a whole class for this given that it seems to just wrap an enum?

This was a solution I came up to have a place to easily manage conversions from string and to string, for logging and loading of the enum from json (strings are more readable than ints!).
I am open for suggestions in case we can make it more elegant!

I plan to look at something similar to what is shown here but this can be done in a separate PR.

arocchi · 2018-12-06T01:12:33Z

@Levi-Armstrong bump

Levi-Armstrong · 2018-12-06T21:23:41Z

@arocchi Then only think left is the discussion around naming of enum to ModelType.

arocchi · 2018-12-06T23:01:07Z

@Levi-Armstrong I renamed ConvexSolver into ModelType, let me know your thoughts about it so that we can proceed with the PR

Levi-Armstrong · 2018-12-07T15:13:11Z

@arocchi It is saying there are conflicts with the base branch. Do yo mind rebasing and then I will merge?

arocchi · 2018-12-07T18:25:37Z

@Levi-Armstrong latest commit from kinetic-devel has been merged in e369b4b - and on my side github does not mention conflicts, but it still mentions a "Changes requested" from your review...

Levi-Armstrong · 2018-12-07T20:31:08Z

@arocchi It will not let me merge it. It says This branch cannot be rebased due to conflicts but it does not give me any indication what the conflict is.

arocchi added 7 commits November 8, 2018 18:22

Added osqp solver and changed default logging level.

292c128

Added qpOASES interface and tests

f603177

Disabled test with negative matrix, tuned osqp to work with nilpotent…

bfc1340

… matrix

Tests that use optimize() will now run for all solvers automatically

0856bb6

Tests that use optimize() now run for all available solvers

2d30d70

Fixed availableSolvers()

ae6bf84

arocchi added 3 commits November 10, 2018 18:26

Made qpOASES solver more robust.

011d5a9

Notice this should be reviewed after bpmpd is removed and all memory alignment problems are resolved. In fact, right now the solver is occasionally instantiated twice in each solve cycle: this makes test pass.

Fixed memory leaks in osqp solver

90afbbb

small refactor towards clang / roscpp guidelines

c3c6ae3

Remove the use of 'using namespace'

43f0850

Alessio Rocchi added 7 commits November 19, 2018 16:27

Merge remote-tracking branch 'rosind/kinetic-devel' into add_free_sol…

dd30768

…vers_upstream

Merge remote-tracking branch 'levi/kinetic-devel' into add_free_solve…

2a44cc1

…rs_upstream

Using typedefs instead of std::vector for common types: osqp_interfac…

a7c4e81

…e, qpoases_interface

Removed evil cleanupQuad from trajopt_sco/expr_ops.*

36a1fb0

Added solver_utils

dceb015

Added #pragma once for all solvers interfaces

ab39ee0

solver_utils tests are passing

a905f25

Alessio Rocchi added 2 commits November 21, 2018 16:45

Refactored qpOASES, osqp, solver_utils

4515896

clang-format

30229ec

ProblemConstructionInfo now contains info on which convex solver to use

54d63a5

Added missing JSONCPP from trajopt_sco/CMakeLists.txt

2f4515d

Alessio Rocchi added 2 commits November 28, 2018 16:33

Cleanup ConvexSolver to string and back

6880ba6

Merge remote-tracking branch 'rosind/kinetic-devel' into add_free_sol…

e369b4b

…vers_upstream

Levi-Armstrong requested changes Nov 29, 2018

View reviewed changes

bhaskara reviewed Nov 29, 2018

View reviewed changes

Addressed most comments in first round of review

3569135

Renamed ConvexSolver into ModelType

ef3ac00

Levi-Armstrong approved these changes Dec 7, 2018

View reviewed changes

Levi-Armstrong merged commit e96e303 into tesseract-robotics:kinetic-devel Dec 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds osqp and qpOASES solver interfaces #40

Adds osqp and qpOASES solver interfaces #40

arocchi commented Nov 9, 2018

Levi-Armstrong commented Nov 10, 2018

Levi-Armstrong commented Nov 10, 2018

Levi-Armstrong commented Nov 10, 2018

arocchi commented Nov 10, 2018

arocchi commented Nov 11, 2018

Levi-Armstrong commented Nov 16, 2018

arocchi commented Nov 16, 2018

arocchi commented Nov 20, 2018

arocchi commented Nov 22, 2018 •

edited

Levi-Armstrong commented Nov 27, 2018

arocchi commented Nov 28, 2018

arocchi commented Nov 28, 2018

Levi-Armstrong commented Nov 28, 2018

arocchi commented Nov 29, 2018

Levi-Armstrong commented Nov 29, 2018

Levi-Armstrong commented Nov 29, 2018

bhaskara left a comment

bhaskara Nov 29, 2018

arocchi Nov 30, 2018

Levi-Armstrong Dec 6, 2018

arocchi commented Dec 6, 2018

Levi-Armstrong commented Dec 6, 2018

arocchi commented Dec 6, 2018

Levi-Armstrong commented Dec 7, 2018

arocchi commented Dec 7, 2018

Levi-Armstrong commented Dec 7, 2018

Adds osqp and qpOASES solver interfaces #40

Adds osqp and qpOASES solver interfaces #40

Conversation

arocchi commented Nov 9, 2018

Levi-Armstrong commented Nov 10, 2018

Levi-Armstrong commented Nov 10, 2018

Levi-Armstrong commented Nov 10, 2018

arocchi commented Nov 10, 2018

arocchi commented Nov 11, 2018

Levi-Armstrong commented Nov 16, 2018

arocchi commented Nov 16, 2018

arocchi commented Nov 20, 2018

arocchi commented Nov 22, 2018 • edited

Levi-Armstrong commented Nov 27, 2018

arocchi commented Nov 28, 2018

arocchi commented Nov 28, 2018

Levi-Armstrong commented Nov 28, 2018

arocchi commented Nov 29, 2018

Levi-Armstrong commented Nov 29, 2018

Levi-Armstrong commented Nov 29, 2018

bhaskara left a comment

Choose a reason for hiding this comment

bhaskara Nov 29, 2018

Choose a reason for hiding this comment

arocchi Nov 30, 2018

Choose a reason for hiding this comment

Levi-Armstrong Dec 6, 2018

Choose a reason for hiding this comment

arocchi commented Dec 6, 2018

Levi-Armstrong commented Dec 6, 2018

arocchi commented Dec 6, 2018

Levi-Armstrong commented Dec 7, 2018

arocchi commented Dec 7, 2018

Levi-Armstrong commented Dec 7, 2018

arocchi commented Nov 22, 2018 •

edited