Handle solver failures like TrajOpt does #369
Conversation
rjoomen
force-pushed
the
maxsolverfailures
branch
from
0de20a6 to
0bc3609
Compare
|
I'm not entirely sure about removing the setVariables, what do you think, @Levi-Armstrong? The calls to updateBounds() seems necessary to me, as box size changes do modify the bounds here. |
Yea, the set variables need to stay because it need to set them back to the previous iterations values. |
| if (status_ != SQPStatus::RUNNING) | ||
| { | ||
| qp_problem->setVariables(results_.best_var_vals.data()); |
There was a problem hiding this comment.
This just needs to be added back.
There was a problem hiding this comment.
Why here exactly? Wouldn't adding it to solveQPProblem() upon failure (in the else block) be more logical?
There was a problem hiding this comment.
I don't think that would work since it would need to be set before convexify is called in stepSQPSolver. I think we might be able to move into that method but I would want to do a lot of testing to make sure everything aligns with the legacy version before making a change like this.
There was a problem hiding this comment.
Isn't this equivalent? In the previous version solveQPProblem() is called, and if qp_solver->solve() failed and hence solveQPProblem() failed setVariables is called from withing runTrustRegionLoop(). Now I moved this line to solveQPProblem(), still to be called if qp_solver->solve() failed. It's just clearer now, as solveQPProblem() either sets the variables to new_var_vals and back to best_var_vals if succeeded and immediately to best_var_vals if failed. See line 386 in my PR.
There was a problem hiding this comment.
I know the numerical ik has difference on Jammy because on one interaction it returns iteration limit instead of approximate solution but focal produces identical results. I am currently running focal so let me know if you want me to compare since I think you are running jammy.
There was a problem hiding this comment.
Aha, I'm running in that issue exactly! But I've also access to focal (I'm using WSL), so I'll run it there tomorrow.
There was a problem hiding this comment.
From what I could tell both hit the iteration limit but because of slight numerical differences focal picks approximate solution and jammy pick iteration limit.
There was a problem hiding this comment.
Mmm, I'm seeing other differences on Focal. And Focal and Jammy do not match. Would you mind taking a look?
rjoomen
force-pushed
the
maxsolverfailures
branch
from
f70c3d2 to
5b65d44
Compare
|
Unstable/jammy fails due to clang-tidy-14. It needs a more extensive clang-tidy config, I think. Ubuntu/jammy succeeds, as no clang-tidy is found. |
Lets try for CI to install the same version of clang-tidy that is used on focal for now until we have time to go through and upgrade to the latest version of clang-tidy. |
rjoomen
force-pushed
the
maxsolverfailures
branch
from
abce1c1 to
864e35a
Compare
…handle solver failures - Added updateBounds() call after each box size modification - Removed setVariables() call after a solver failure - Renamed SQPStatus::TIME_LIMIT to SQPStatus::OPT_TIME_LIMIT because of a name collision with OSQP (in tesseract_planning)
rjoomen
force-pushed
the
maxsolverfailures
branch
from
864e35a to
78d4c57
Compare
rjoomen
force-pushed
the
maxsolverfailures
branch
from
9f7812a to
2ee35aa
Compare
This reverts commit 4a16157.
|
I compared trajopt_sqp numerical ik output between this branch and master with no differences found. I think your changes are good. |
|
@rjoomen You good with me squash merging this? |
|
Go ahead! Thanks for checking |
Levi-Armstrong
merged commit 713cc9d
into
tesseract-robotics:master
Closes #368