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

Add kwarg fail_on_infeasable in model.solve #1139

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

esske
Copy link

@esske esske commented Nov 22, 2024

Added a new keyword to model.solve(): allow_nonoptimal, which is False on default.

If allow_nonoptimal is False an error message is raised if the model does return a nonoptimal soltion, such as infeasable or unbounded.

Implements #1056.

@pep8speaks
Copy link

pep8speaks commented Nov 22, 2024

Hello @esske! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2025-01-10 09:51:21 UTC

@3LL3RM4NN
Copy link

I like the idea of having this new keyword in the solve() function.
However, how do you define nonoptimal?
From this pyomo blogpost, I would say there are nonoptimal termination conditions and failed termination conditions like infeasible.
In my opinion there should be a differentiation.

Additionally, I dont get why solver status and termination condition are accessed like this:
status = solver_results["Solver"][0]["Status"] and termination_condition = solver_results["Solver"][0]["Termination condition"]
and not this:
status = solver_results.Solver.Status and termination_condition = solver_results.Solver.Termination_condition

@p-snft
Copy link
Member

p-snft commented Dec 3, 2024

I like the idea of having this new keyword in the solve() function. However, how do you define nonoptimal? From this pyomo blogpost, I would say there are nonoptimal termination conditions and failed termination conditions like infeasible. In my opinion there should be a differentiation.

Thanks for your feedback. The feature is targeted at novice users. We expect that they assume a result to be optimal by default. Thus, model.solve() would fail on everything else - which is unexpected behaviour in these cases. If you are fine with a different solution you get e.g. after a time limit is reached, you should also be able to handle the differentiation yourself.

Additionally, I dont get why solver status and termination condition are accessed like this: status = solver_results["Solver"][0]["Status"] and termination_condition = solver_results["Solver"][0]["Termination condition"] and not this: status = solver_results.Solver.Status and termination_condition = solver_results.Solver.Termination_condition

I like the API that works without the [...] operators. It has been like that since 2017 (see 4fefc4b). The message, however, does not explain why it is done this way and not differently. (Interestingly, the commit claims to fix #292, which it did not really. I guess, we would not have #1056 if it really introduced the promised error raising.) Long story short: It seems to make sense to change this.

@esske
Copy link
Author

esske commented Dec 20, 2024

I changed the access of solver status and termination condition as suggested and wrote test to check if infeasable and unbounded problems are caught.

I noticed that if the the 'tee=True' argument of pyomo is set. There is an additional warning from Pyomo in the form of

WARNING: Loading a SolverResults object with a warning status into
model.name="Model";
    - termination condition: infeasible
    - message from solver: Model was proven to be infeasible.

Seems a bit redundant, but I am not sure if we should supress this warning?

@p-snft
Copy link
Member

p-snft commented Dec 30, 2024

I noticed that if the the 'tee=True' argument of pyomo is set. There is an additional warning from Pyomo (...)
Seems a bit redundant, but I am not sure if we should supress this warning?

The redundant warning was there before. Now we fail after the Pyomo warning, if not set otherwise. I won't worry too much, as this is a clear improvement. (We could decide not to warn if both, tee and returning without an optimal solution, are set, but that's a separate topic from this PR.)

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.

4 participants