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

Avoid KeyError upon unknown IPOPT return statuses. #134

Conversation

tobias-kies-artemis
Copy link
Contributor

Follow-up PR as discussed in this thread. It's basically just a one-line change to avoid KeyErrors in case IPOPT returns a status that cyipopt cannot (yet) deal with.

Side-note: I considered adding a test for this, somewhat along the lines of

import ipopt_wrapper


def test_unknown_ipopt_status(
    monkeypatch, hs071_initial_guess_fixture, hs071_problem_instance_fixture
):
    """Check if cyipopt is able to gracefully handle unknown IPOPT
    statuses.
    """
    # Set some arbitrary "unknown" status code and make sure that it
    # really is unknown to cyipopt.
    unknown_status = -42
    assert unknown_status not in ipopt_wrapper.STATUS_MESSAGES

    # Monkey patch the call to the IPOPT solver to just instantly return
    # the unknown status code.
    def mock_solve(*args, **kwargs):
        return unknown_status

    # WILL NOT WORK -> IpoptSolve is "hidden".
    monkeypatch.setattr(ipopt_wrapper, "IpoptSolve", mock_solve)

    # Solve a sample problem and check for the final status message.
    x0 = hs071_initial_guess_fixture
    nlp = hs071_problem_instance_fixture
    _, info = nlp.solve(x0)
    assert info["status_msg"] == b"Unknown status"

but it would not work, because it appears you cannot monkey patch a "cimported" function and I do not see any way around this. We could probably get it to work by restructuring the code in ipopt_wrapper.pyx a bit further, but maybe that's a bit much of a hassle for a one-line change. What do you think?

@tobias-kies-artemis
Copy link
Contributor Author

(realized that I forgot to ping @moorepants - sorry in case this is spam and the ping got out already via mail notification earlier)

@moorepants moorepants merged commit 93d712e into mechmotum:master Nov 2, 2021
@moorepants
Copy link
Collaborator

Thanks!

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.

2 participants