search

coin-or / pulp

A python Linear Programming API
http://coin-or.github.io/pulp/
Other
2.04k stars 381 forks source link

Change in highs api and status handling in both cases #682

Closed ggsdc closed 7 months ago

ggsdc commented 11 months ago

The status while solving with HiGHS was not getting updated properly and It would give back a wrong solution code on time limit but with a solution found.

This PR should fix those issues.

Should close #661 and close #633

CLAassistant commented 11 months ago

CLA assistant check
All committers have signed the CLA.

CLAassistant commented 11 months ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

ggsdc commented 11 months ago

Thanks Guille. I like the mapping dictionary, because it's easy to read. Could it be kept while improving the precision that these changes bring?

I've added a status mapping dict with two codes but I still have to leave an exception to the dictionary for the kTimeLimit and kIterationLimit statuses as are the only two where the actual status to give back can either be optimal + integer feasible or not solve + no solution.

ggsdc commented 11 months ago

The checks seem to be failing due to the issue reported in #683 and #684

pchtsp commented 11 months ago

can you please add a unit test that proves that there was something wrong and it was fixed? thanks

ggsdc commented 11 months ago

can you please add a unit test that proves that there was something wrong and it was fixed? thanks

I think the new test should take into account the main issue that was solved that the solver as returning status 0 on time limit even if a solution was founded.

The new test should be able to be extended to other solvers to check correct status handling on time limit without a solution.

I will try to add (in a new PR soon) the ability for the checks, on the pulpTestCheck method, to also check for the solution status if given. This should allow to increase the coverage to these statuses as well.

pchtsp commented 11 months ago

also, is this related to https://github.com/coin-or/pulp/pull/624?

ggsdc commented 11 months ago

also, is this related to #624?

As far as I can see point 2 of the ones mentioned on the PR should be on par with the changes done here to HiGHS. Once we modify the tests to take into account the solution status we may find that there are instances in which the solution status that PuLP is giving back may not be the correct one.