Qpsol support - [merged] #1253

Closed SGeeversAtVortech closed 1 week ago

In GitLab by @vreeken on May 15, 2018, 21:52

Merges qpsol-support -> master

Closes #1006

In GitLab by @vreeken on May 15, 2018, 21:54

@baayen

There was a special case for "too many degrees of freedom". Any idea on why that was?
There was a comment in the old parsing that stated "# In this case we expect some higher level process to deal with the solver failure, so we only log it as info here.". Yet it still returned success = False. The only difference was whether it was logged as info or error. Why?

In GitLab by @baayen on May 16, 2018, 12:49

Ad 1: Yes, if too many goals are specified, the optimization problem will at some point run out of degrees of freedom to improve anything. It'll then fail with said error message, and the solution of the previous goal should be returned.

Ad 2: A homotopy suboptimization run could fail. HomotopyMixin will then backtrack and return the deformation parameter step size. The error is this case internal, and should not be flagged up as a user-facing error in FEWS.

In GitLab by @vreeken on May 16, 2018, 13:02

Ok, but the only difference was warning instead of error (both are success=False). Do you want to support this nuance in logging? I think that users that know/pay attention to the difference between error and warning are also aware of what Not_Enough_Degrees_Of_Freedom means. If you want it, I'll special case ipopt in solver_success for backwards compatibility in logger warning/error levels.
OK. I can support that still.

In GitLab by @vreeken on May 16, 2018, 13:07

added 1 commit

aa9d7571 - Readd support for logging solver errors with info not error

Compare with previous version

In GitLab by @baayen on May 16, 2018, 13:24

Ad 1: Yes, the nuance is important. If any error is logged, any error at all, FEWS will flag the entire run as having failed.

In GitLab by @vreeken on May 16, 2018, 13:34

added 1 commit

77d7297f - Fix mistakes

Compare with previous version

In GitLab by @vreeken on May 16, 2018, 13:35

added 5 commits

b041d936 - 1 commit from branch master
dda20788 - Add support for CasADi's qpsol
5b3e7c80 - Handle solver status for solvers other than ipopt
93036571 - Readd support for logging solver errors with info not error
b7575b60 - Fix mistakes

Compare with previous version

In GitLab by @vreeken on May 16, 2018, 14:00

added 1 commit

bf49d68a - Reimplement Not_Enough_Degrees_Of_Freedom exception for IPOPT

Compare with previous version

In GitLab by @vreeken on May 16, 2018, 16:49

added 3 commits

bf49d68a...44522cf1 - 2 commits from branch master
078b19ea - Add support for qpsol and solvers other than IPOPT

Compare with previous version

In GitLab by @vreeken on May 16, 2018, 17:19

added 1 commit

d2d0e250 - Break the old API

Compare with previous version

In GitLab by @vreeken on May 16, 2018, 17:28

Unfortunately it's not possible to (1) provide generality and (2) sane defaults, without breaking the API. The old code was too focused on just IPOPT and BONMIN, and CasADi is not very consistent. I'd rather not try to be smart to work around CasADi's inconsistencies, and just let users "talk directly" to CasADi.

I tried to just not use super() in solver_options, but it turns out it's also used for other magic stuff like optimized_num_dir, which are very much needed but then remain unset. And it's not possible for e.g. GoalProgrammingMixin or CollocatedIntegratedOptimizationProblem to know whether the user or not will use IPOPT or not with the current way of travelling the hierarchy.

Anyway, to fix it we need to:

No longer request flattened solver options, and then at the last moment "indent" it with the solver name. CasADi only allows this for a few solvers (e.g. ipopt, bonmin, gurobi), but not for CLP and QPOases. For example, for QPOases all options are specified at the same level as CasADi, and for CLP it's not allowed to specify any options at all (not even passing {'clp': {}}). Not sure about others.
~~Remove IPOPT specific code elsewhere in the code.~~ Not needed; just add it to the 'ipopt' or 'bonmin' solver options.

In GitLab by @vreeken on May 16, 2018, 17:37

added 1 commit

858440bf - Remove ipopt specific stuff from code

Compare with previous version

In GitLab by @baayen on May 17, 2018, 12:33

Commented on src/rtctools/optimization/goal_programming_mixin.py line 397

Some of these options are crucial for some applications.

I suggest to leave them in. When using a different solver than IPOPT, they can be deleted by the modeller. Previously, we had an if solver == 'ipopt' block in, but that then broke things for bonmin, and so rather than checking for 'ipopt' or 'bonmin', I at the time decided to remove the check.

Note that if we go ahead with the MI+homotopy solver, the need for switching solver backends will be much reduced or even vanish, with perhaps CLP for NHI as an exception.

In GitLab by @baayen on May 17, 2018, 12:33

Commented on src/rtctools/optimization/goal_programming_mixin.py line 426

Dropping this is not a good idea. Extremely important for debugging IP methods i.c.m. goal programming.

In GitLab by @vreeken on May 17, 2018, 15:47

Commented on src/rtctools/optimization/goal_programming_mixin.py line 397

I think there will always be a need for switching solver backend, and at the very least we should provide the possibility. Sane default however do make sense.

I'll add a check here to see if the solver is ipopt/bonmin, and then set it accordingly.

In GitLab by @vreeken on May 17, 2018, 15:47

Commented on src/rtctools/optimization/goal_programming_mixin.py line 426

OK, will add that back in

In GitLab by @vreeken on May 17, 2018, 15:59

Commented on src/rtctools/optimization/goal_programming_mixin.py line 397

changed this line in version 9 of the diff

In GitLab by @vreeken on May 17, 2018, 15:59

added 1 commit

d73f58b5 - Break the old API

Compare with previous version

In GitLab by @vreeken on May 20, 2018, 24:46

Commented on src/rtctools/optimization/goal_programming_mixin.py line 426

changed this line in version 10 of the diff

In GitLab by @vreeken on May 20, 2018, 24:46

added 9 commits

d73f58b5...ae399329 - 6 commits from branch master
2df96f96 - Add support for qpsol and solvers other than IPOPT
5a277b1b - Break the old API
6b629c90 - Add CLP test case

Compare with previous version

In GitLab by @vreeken on May 20, 2018, 24:50

added 1 commit

ee77880c - Small pep8 fix

Compare with previous version

In GitLab by @codecov on May 20, 2018, 24:56

Codecov Report

Merging #112 into master will increase coverage by 0.19%. The diff coverage is 91.89%.

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
+ Coverage   73.08%   73.28%   +0.19%     
==========================================
  Files          23       23              
  Lines        4314     4327      +13     
  Branches      961      963       +2     
==========================================
+ Hits         3153     3171      +18     
+ Misses        872      868       -4     
+ Partials      289      288       -1

Impacted Files	Coverage Δ
...tion/collocated_integrated_optimization_problem.py	`77.6% <100%> (+0.04%)`	:arrow_up:
src/rtctools/optimization/optimization_problem.py	`77.17% <100%> (+2.54%)`	:arrow_up:
...rc/rtctools/optimization/goal_programming_mixin.py	`74.72% <57.14%> (+0.16%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ae39932...ee77880. Read the comment docs.

In GitLab by @vreeken on May 22, 2018, 17:08

@jvande42b

solver_success an OK name?
OK with the API change for solver options?

In GitLab by @vreeken on May 22, 2018, 17:16

resolved all discussions

In GitLab by @vreeken on May 22, 2018, 17:25

added 1 commit

342219dc - Fix solver options in examples

Compare with previous version

In GitLab by @jvande42b on May 23, 2018, 20:32

The name is ok.
I agree, I think it is important to make sure we expose the full casadi options to the user.

Is solver_stats['success'] new? why didn't we use it before?

In GitLab by @vreeken on May 23, 2018, 20:44

I think it's somewhat new yes. I think Joris implemented it when I expressed my desire to have CLP return some kind of status, and that it would be nice that all solvers would do so in a consistent manner. I did not actually know he implemented this for all (?) solvers until I started working on this issue though.

In GitLab by @vreeken on May 26, 2018, 12:02

added 1 commit

bfe805f0 - Add support for qpsol and solvers other than IPOPT

Compare with previous version

In GitLab by @vreeken on May 26, 2018, 12:03

added 7 commits

bfe805f0...d81c9113 - 6 commits from branch master
b2dc65d6 - Add support for qpsol and solvers other than IPOPT

Compare with previous version

In GitLab by @vreeken on May 26, 2018, 12:11

merged

Deltares / rtc-tools

Qpsol support - [merged] #1253

Codecov Report