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 warm start, gap retrieval and better solution statuses for SCIP_PY (pyscipopt) #624

Merged
merged 6 commits into from
Jan 12, 2024

Conversation

supremeBaboon
Copy link
Contributor

@supremeBaboon supremeBaboon commented Jan 17, 2023

This PR aims to improve the SCIP_PY solver by :

  1. Adding a boolean warmStart parameter. When True, variables set to an initial value will be used by SCIP to fasten the optimization. In particular, one can warm start only a few variables, as SCIP does not need a complete solution to start from.
  2. Improving the solution statuses when a MIP reaches time or gap limit, as such cases are currently returned as LpStatusNotSolved, even when a feasible (but non optimal solution) exists. This correction is pretty similar to that of Correcting the solution status for SCIP in order to read integer feasible solutions #473 for SCIP_CMD*.
  3. Adding a getGap method to retrieve SCIP's gap after an optimization. As SCIP is particularly verbose, I usually disable its logs, then using this method to still get a (concise) information about the optimization.

I have not made any unit test, but I have (sucessfully) tested those additions on a custom MIP on which I am working.

*Does anyone have any news about that PR ? It has been open for ~1 year and a half now, and looks almost complete...

@CLAassistant
Copy link

CLAassistant commented Jan 17, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ supremeBaboon
❌ agautier


agautier seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@supremeBaboon
Copy link
Contributor Author

I was initially pretty unsure about pyscipopt's warm start, since I've only found 2 related links : this one (whose formulation is used in this PR) and this one.
Yet, after multiple tests, it seems to work just fine. The only downside I found lies in SCIP itself, as feeding it with only a few variables to warm start can 'freeze' it in finding a first solution from these initial values. In particular, SCIP can find a first solution faster without that partial initial solution (but its cost might be worse).

@supremeBaboon
Copy link
Contributor Author

@pchtsp could you review this PR please ?

@sam-slrx
Copy link

sam-slrx commented Mar 9, 2023

@supremeBaboon have you run into any issues with this approach (specifically the solution statuses fix)? I'm planning on forking your fork :)

Copy link
Collaborator

@pchtsp pchtsp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I'm not 100% sure on the method to get the gap. It may be a good idea but I can also see it being a bit confusing. Can we split it into two PRs and that way I can approve the rest? thanks!

I left a small change on the if clause and guaranteeing the code still works if the warmStart is not present.

##################################################
# add warm start
##################################################
if self.optionsDict["warmStart"]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that here you'll run into problems if warmStart is not given.
Check what we do for cplex:

if self.optionsDict.get("warmStart", False):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected, but since warmStart was instantiated to False this shouldn't had thrown an error

@@ -680,3 +711,12 @@ def actualResolve(self, lp):
raise PulpSolverError(
f"The {self.name} solver does not implement resolving"
)

def getGap(self, lp):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the need for this method? I'd take it out as it will be a bit confusing: we don't usually offer the gap for any solver so far.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this method, it was mostly for my personal use but I understand your point. Tell me if you think it is still valuable to put in another pull request ;)

@supremeBaboon
Copy link
Contributor Author

@sam-slrx Sorry for late answer, I've been using these changes extensively for the past 2 months and I've never encountered any issue, both for the warm-start and the solution statuses, so I assume it's safe to say they work as expected. Note however that I only tested those on MIP, not on LP.

@pchtsp
Copy link
Collaborator

pchtsp commented Apr 1, 2023

I think you need to apply the black linter to the project, as explained here: https://coin-or.github.io/pulp/develop/contribute.html#applying-the-black-linter-formatter

@supremeBaboon
Copy link
Contributor Author

@pchtsp done, is there anything more to do ?

@pchtsp
Copy link
Collaborator

pchtsp commented Jul 15, 2023

looks good! It seems one contributor (agautier) has not signed the CLA. Once that's done, I can merge.

@supremeBaboon
Copy link
Contributor Author

@pchtsp is it signed now ?

@pchtsp
Copy link
Collaborator

pchtsp commented Sep 24, 2023

No, it's still pending.

image

@pchtsp
Copy link
Collaborator

pchtsp commented Sep 24, 2023

image

@supremeBaboon
Copy link
Contributor Author

supremeBaboon commented Sep 24, 2023

But it keeps telling me it is signed

image

@tmaarse
Copy link
Contributor

tmaarse commented Sep 24, 2023

@supremeBaboon The last commit was made by agautier adrien.gautier@soprasteria.com, who hasn't signed the CLA yet. You might be able to replace their commit with one where you ran Black, or get them to sign it as well.

@pchtsp pchtsp merged commit 5769e27 into coin-or:master Jan 12, 2024
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.

5 participants