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

SLSQP search stopped too early on tutorial constraints #514

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

edwintorok
Copy link

I noticed that the python and C++ tutorials gave different answers when used with SLSQP:
SLSQP was giving the wrong answer on the tutorial in 't_python.py':
optimum at [0.50777849 4.76252907] minimum value = 2.182321944309753
instead of:
optimum at [0.33333333 0.29629629] minimum value = 0.5443310523133087

They are implemented slightly differently, using float multiplication vs ** 2 and ** 3, which result in slightly different floating point values, however that shouldn't cause SLSQP to fail this badly.
The problem isn't with the Python wrapper, because if I reimplement the same function for 'SciPy' it gives the correct answer.

After printing the individual 'f' evaluations I noticed that it stopped the search due to hitting 'xtol_rel', but the function was infeasible at that point (so it returned the last feasible one, which is way off the optimal value).
If I instead let the search continue until it finds a feasible point again then it finds the correct answer.

The problem was hinted to at #368 (comment), which mentions that perhaps feasible_cur should be used, and I think as this example shows that would be the correct approach
(the original SLSQP fortran function also had a check for 'h3', which looks similar to the feasibility check done here).

While fixing the bug I also tweaked the tutorial function to accept the algorithm as parameter and run this during make test to check a few algorithms (those that accept inequality constraints and infinite bounds).

SLSQP was giving the wrong answer on the tutorial in 't_python.py':
```
optimum at  [0.50777849 4.76252907]
minimum value =  2.182321944309753
```
instead of:
```
optimum at  [0.33333333 0.29629629]
minimum value =  0.5443310523133087
```

Slightly tweaking the function (e.g. to use repeated multiplication
instead of pow), or increasing xtol_rel to 1e-9 made it find the answer.
But the bogus optimum and real optimum are different by more than
xtol_rel.

Printing the function evaluations show that it reaches xtol_rel at an
infeasible point, and then returns the last feasible point:
```
f(0.337085195789626, 0.287031459045266)=0.535753169888211
c(0.337085195789626, 0.287031459045266)=0.019382838080164
c(0.337085195789626, 0.287031459045266)=0.004290454106768
f(0.337085195789626, 0.287031459045266)=0.535753169888211
c(0.337085195789626, 0.287031459045266)=0.019382838080164
c(0.337085195789626, 0.287031459045266)=0.004290454106768
optimum at  [0.50777849 4.76252907]
minimum value =  2.182321944309753
result code =  4
nevals =  22
```

Fix this by checking xtol_rel on mode==-1 only if the current point is
feasible, not when any previous point was feasible
(which matches how the original SLSQP's stopping condition was, and how
the other xtol_rel check is done in this same function).

Now the search continues and finds the correct answer:

```
f(0.337085195789626, 0.287031459045266)=0.535753169888211
c(0.337085195789626, 0.287031459045266)=0.019382838080164
c(0.337085195789626, 0.287031459045266)=0.004290454106768
f(0.298264335607084, 0.000001000000000)=0.001000000000000
c(0.298264335607084, 0.000001000000000)=0.212271613303731
c(0.298264335607084, 0.000001000000000)=0.345556758201188
[...]
f(0.333333334001826, 0.296296294513450)=0.544331052314168
c(0.333333334001826, 0.296296294513450)=0.000000003565495
c(0.333333334001826, 0.296296294513450)=0.000000000891522
optimum at  [0.33333333 0.29629629]
minimum value =  0.5443310523133087
result code =  4
nevals =  48
```

Fixes: 42c43f3 ("Only exit SLSQP successfully if solution is feasible (stevengj#465)")

Signed-off-by: Edwin Török <edwin@etorok.net>
Only disable tests on those that require subalgorithms.

Signed-off-by: Edwin Török <edwin@etorok.net>
Signed-off-by: Edwin Török <edwin@etorok.net>
Signed-off-by: Edwin Török <edwin@etorok.net>
@stevengj
Copy link
Owner

stevengj commented Jun 7, 2023

LGTM.

@jschueller
Copy link
Collaborator

some tests fail

@edwintorok
Copy link
Author

Interesting it fails only on windows but not Linux. I'll take a look, but perhaps for now I'll mark those tests as skipped and fix the problems in another PR (those algorithms were not being tested at all previously I think).

@edwintorok
Copy link
Author

edwintorok commented Jun 8, 2023

I can reproduce one of the failures on Linux: the starting point is randomly generated and some starting points cause the algorithm to fail with NLOPT_ROUNDOFF_LIMITED.
This will eventually fail: while ./testopt -a 30 -o 1; do echo -n ;done.

I'll remove algorithms 30 and 31 from the tests so far, it looks like their algorithms aren't quite as numerically stable as the other ones.

Note that before the following commit these were disabled:
a6ceab9

Sometimes these algorithms fail depending on the (random) starting point
with NLOPT_ROUNDOFF_LIMITED.

Signed-off-by: Edwin Török <edwin@etorok.net>
@stevengj
Copy link
Owner

stevengj commented Jun 8, 2023

This will eventually fail: while ./testopt -a 30 -o 1; do echo -n ;done.

You aren't passing any stopping tolerance to testopt? Probably should be passing e.g. -m 1e-3 so that it stops when it gets close to the (known) minimum.

That being said, the tests should probably also pass -r 0 to use a fixed random seed, in order to lessen the chance of a weird random event biting us on repeated test runs.

test/CMakeLists.txt Outdated Show resolved Hide resolved
@stevengj
Copy link
Owner

stevengj commented Aug 9, 2024

Still failing on Windows 😢

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.

3 participants