-
Notifications
You must be signed in to change notification settings - Fork 530
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 warmstart for appsi_highs (issue #3450) #3494
base: main
Are you sure you want to change the base?
Conversation
@@ -155,3 +155,31 @@ def test_capture_highs_output(self): | |||
self.assertIn("HiGHS run time", OUT.getvalue()) | |||
ref = "10.0 5.0\n" | |||
self.assertEqual(ref, OUT.getvalue()[-len(ref) :]) | |||
|
|||
def test_warm_start(self): | |||
m = pe.ConcreteModel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stole this model from issue #3443, I think it's a good test case for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! I have one minor comment/question below.
with Highs() as opt, capture_output() as output: | ||
opt.solve(m, tee=True, warmstart=True) | ||
log = output.getvalue() | ||
self.assertIn("MIP start solution is feasible, objective value is 25", log) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to test this without parsing the log? That tends to be fragile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also prefer to use another way to test it, I only went with this as this is what I saw in test_cbc.py. If somebody can suggest a better way, I will change it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I know of a better way. Everything I can think of is equally fragile (if not moreso). I just thought I'd ask.
if has_values: | ||
solution = highspy.HighsSolution() | ||
solution.col_value = col_value | ||
solution.value_valid = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does value_valid
mean? Does that indicate the solution is feasible? What happens if the solution provided is not feasible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution.value_valid flag doesn't assert that the provided solution is feasible - it only indicates that the primal values are valid inputs that should be considered by the solver.
If you pass an infeasible starting point to HiGHS with value_valid = True:
- HiGHS will accept the values as a starting point
- The solver will still verify feasibility and will not treat your solution as feasible until it confirms this
- If your solution is infeasible, it seems HiGHS will work to repair it or use parts of it to guide the search
For example, here is the log excerpt after setting x1 = 10 in the test case I have just added:
Assessing feasibility of MIP using primal feasibility and integrality tolerance of 1e-06
WARNING: Row 0 has infeasibility of 5.5 from [lower, value, upper] = [ -inf; 14.5; 9]
WARNING: Row 1 has infeasibility of 16.5 from [lower, value, upper] = [ -inf; 34.5; 18]
Solution has num max sum
Col infeasibilities 0 0 0
Integer infeasibilities 0 0 0
Row infeasibilities 3 16.5 25
Row residuals 0 0 0
Attempting to find feasible solution by solving LP for user-supplied values of discrete variables
Coefficient ranges:
Matrix [1e+00, 3e+00]
Cost [2e+00, 4e+00]
Bound [1e+00, 1e+01]
RHS [6e+00, 2e+01]
Presolving model
Problem status detected on presolve: Infeasible
Model status : Infeasible
Objective value : 0.0000000000e+00
HiGHS run time : 0.00
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy to add the MIP start with x1 = 10 as another test case but that would mean more log parsing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Good to know. No need for a new test (in my opinion).
Once tests pass, I will approve. |
I will dig in more later today but I'm getting this error:
|
@@ -221,6 +221,29 @@ def symbol_map(self): | |||
return SymbolMap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want this to be able to be used as a context manager, make sure to add an __enter__
and __exit__
, as in here: https://github.com/Pyomo/pyomo/blob/main/pyomo/contrib/solver/base.py#L73
This is not inherently supported in the APPSI base class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, thank you. I just saw that you have a PR up that revamps a lot of the solver interfaces so I will do the least amount of changes and do not use it as a context manager in my test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solver refactor PR is the next in what is probably going to be a fairly long series of changes, so good call 👍 That being said, the refactor is based on some fundamental design choices from APPSI so anything you do here should be portable without too much extra effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I missed this conversation. Sorry.
hi @kutlay, thank you for testing it. I think this is the expected behavior: |
I have fixed the tests error from this morning. The pipeline is failling for windows at the moment; |
Correct, not your fault. We have been seeing a lot of issues recently with windows + conda builds that are outside of anything directly in pyomo. |
@quantresearch1, @kutlay - we are working on a complete refactor of the solver interfaces (see #1030, #3476, and https://pyomo.readthedocs.io/en/latest/explanation/experimental/solvers.html). As part of the refactor, things like config parameters and keyword arguments to the solve method will be far more consistent. |
Fixes #3450 .
Summary/Motivation:
I tried using warmstart=True with 'appsi_highs' and it was crashing. By looking online, I saw there were other people in need of the feature and decided it was not too much work to implement it.
Changes proposed in this PR:
-add warmstart/mip start support for appsi_highs
Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: