Fix: handle HighsModelStatus.kSolutionLimit like kIterationLimit #3634
Conversation
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.
|
So I understand - does |
|
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: |
|
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. |
|
If it helps, I could add in an additional version check for HiGHS in the code. |
|
A version check would be OK, but something like elif status == getattr(highspy.HighsModelStatus, "kSolutionLimit", NOTSET):
results.termination_condition = TerminationCondition.maxIterationsmight be more performant and simpler. |
This looks good to me, but I'm not sure if this might cause some type problems, as 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. |
|
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 ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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? |
|
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. |
…hs.py The enum value is called iterationLimit in this context. Turns out that there are multiple classes TerminationCondition with different names for the cases.
|
It's good that I tried to add tests, and for both interfaces of HiGHS: I was unaware that multiple versions of the 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. |
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:
kSolutionLimitelsebranchLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: