mechmotum / cyipopt

Cython interface for the interior point optimzer IPOPT
Eclipse Public License 2.0
231 stars 54 forks source link

Returning `False` from `intermediate` no longer halts ipopt #249

Closed jsiirola closed 6 months ago

jsiirola commented 6 months ago

Commit d4e986a (merged in #227, released in 1.4.0) changed the implementation of cdef Bool intermediate_cb() so that it no longer returns the result from the user-provided intermediate callback function. As a result, user-provided callbacks can no longer stop Ipopt by returning False.

moorepants commented 6 months ago

Would you have a simple test case for this?

jsiirola commented 6 months ago

Alas, not a simple test case - we saw it as a new failure in the Pyomo test suite (pyomo/contrib/pynumero/examples/tests/test_cyipopt_examples.py::TestExamples::test_cyipopt_callback_halt; e.g., https://github.com/Pyomo/pyomo/actions/runs/8514519228/job/23320399948?pr=3221).

I believe the fix is simply

diff --git a/cyipopt/cython/ipopt_wrapper.pyx b/cyipopt/cython/ipopt_wrapper.pyx
index 305b636..6b22953 100644
--- a/cyipopt/cython/ipopt_wrapper.pyx
+++ b/cyipopt/cython/ipopt_wrapper.pyx
@@ -1274,6 +1274,7 @@ cdef Bool intermediate_cb(Index alg_mod,

         if ret_val is None:
             return True
+        return ret_val
     except:
         self.__exception = sys.exc_info()
         return True
moorepants commented 6 months ago

I see the fix, but wanted to add a regression test.

jsiirola commented 6 months ago

Ahh - sorry - the test in question uses a model that is expected to take 8-10 iterations, adds an intermediate callback that is just:

def iteration_callback(
    nlp,
    alg_mod,
    iter_count,
    obj_value,
    inf_pr,
    inf_du,
    mu,
    d_norm,
    regularization_size,
    alpha_du,
    alpha_pr,
    ls_trials,
):
    if iter_count >= 4:
        return False
    return True

It then runs the solver and looks for the user interrupt return code. The problem is that it is all written in Pyomo (so leverages all the Pyomo modeling infrastructure and model conversions), so as written is probably a poor regression test for here.

moorepants commented 6 months ago

Thanks. I have a little test now.

moorepants commented 6 months ago

Fixed in version 1.4.1. Thanks for reporting.