Fix: handle HighsModelStatus.kSolutionLimit like kIterationLimit

[rschwarz]

Was previously not handled at all, resulting in termination_condition of unknown, and no further information about whether a solution was found.

kIterationLimit may not be exact, but it's the best fit among the current enum values, I guess.

Fixes #3632 .

Summary/Motivation

handle HighsModelStatus.kSolutionLimit like kIterationLimit

Changes proposed in this PR:

• add case for kSolutionLimit
• add warning for the else branch

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:

1. I agree my contributions are submitted under the BSD license.
2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

[mrmundt]

So I understand - does kSolutionLimit exist for all versions of highspy, or is it only present in 1.5?

[rschwarz]

My understanding is that it was added to HiGHS v1.5, and has remained to the present day.

EDIT: See this HiGHS test which is still on the main branch:

https://github.com/ERGO-Code/HiGHS/blob/364c83a51e44ba6c27def9c8fc1a49b1daf5ad5c/check/TestMipSolver.cpp#L62-L67

[mrmundt]

Okay, then this is going to need a bit of work. This change would effectively pin us to a minimum version of highspy, and we try our best to maintain backwards compatibility as much as possible. Additionally, there are actually two different HiGHS interfaces - contrib/appsi and contrib/solver. We have a dev call tomorrow and will talk about it then. Thanks for submitting the PR! I'll let you know soon what we decide.

[rschwarz]

If it helps, I could add in an additional version check for HiGHS in the code.

[jsiirola]

A version check would be OK, but something like

       elif status == getattr(highspy.HighsModelStatus, "kSolutionLimit", NOTSET):
            results.termination_condition = TerminationCondition.maxIterations

might be more performant and simpler.

[rschwarz]

A version check would be OK, but something like

       elif status == getattr(highspy.HighsModelStatus, "kSolutionLimit", NOTSET):
            results.termination_condition = TerminationCondition.maxIterations

might be more performant and simpler.

This looks good to me, but I'm not sure if this might cause some type problems, as highspy.HighsModelStatus is actually an enum at the C++ level.

Please let me know if which changes I should apply, if any.

PS: I'm now aware of the non-APPSI interface to HiGHS, which is affected by the same problem.

[mrmundt]

Okay, we talked about this today, and we think using @jsiirola 's proposed solution above on both files should be good. Please make those changes and we'll happily move forward!

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.94%. Comparing base (dc503c2) to head (55b3300). Report is 40 commits behind head on main.

Files with missing lines	Patch %	Lines
pyomo/contrib/appsi/solvers/highs.py	25.00%	3 Missing :warning:
pyomo/contrib/solver/solvers/highs.py	25.00%	3 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3634      +/-   ##
==========================================
+ Coverage   88.92%   88.94%   +0.02%     
==========================================
  Files         888      888              
  Lines      102347   102709     +362     
==========================================
+ Hits        91009    91356     +347     
- Misses      11338    11353      +15     

Flag	Coverage Δ
builders	26.68% <25.00%> (-0.01%)	:arrow_down:
default	85.50% <25.00%> (?)
expensive	34.05% <25.00%> (?)
linux	86.74% <25.00%> (-1.94%)	:arrow_down:
linux_other	86.74% <25.00%> (+0.01%)	:arrow_up:
osx	83.04% <25.00%> (+0.01%)	:arrow_up:
win	84.91% <25.00%> (-0.04%)	:arrow_down:
win_other	84.96% <25.00%> (+0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rschwarz]

I see that the newly added lines are not covered by the existing tests. Should I attempt to set up a MIP problem of moderate size where this status would result after a node limit of 1, or would that be overkill?

[jsiirola]

That would be fantastic. We (i.e., @mrmundt) are working on building a new set of "standard solver tests" that we can use to exercise all the solver interfaces in a semi-consistent manner (in particular to exercise the expected solver return status flags), and it would be great to include this in the suite. Testing it for HiGHS specifically is fine for now -- at least it will be in the repo and can be pulled into the new test suite as it gets assembled.

[rschwarz]

It's good that I tried to add tests, and for both interfaces of HiGHS: I was unaware that multiple versions of the TerminationCondition class exist, and that the field is sometimes called maxIterations, and sometimes iterationLimit.

I'm using a Knapsack instance based on random data, which is not solved to optimality in the root with my version of HiGHS and on my machine, so I hope this will also be the case for GitHub.