-
Notifications
You must be signed in to change notification settings - Fork 68
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
PrimalSearch environment #183
Conversation
libecole/src/scip/scimpl.cpp
Outdated
// if action is empty, do nothing | ||
if (varvals.empty()) { | ||
*result = SCIP_DIDNOTFIND; | ||
return SCIP_OKAY; | ||
} |
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.
Will SCIP run the other (default) scip::ObjHeur
callbacks, and is this what we want?
Just asking, I'm not sure what the desired behavior is here.
Perhaps change the comment do nothing
to what will happen globally.
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.
Those lines were simply about removing some computational overhead in the specific case where the action is {}, by simply doing nothing. The environment should behave in the same way if we remove those lines. If no variable fixings are provided, it will simply re-solve the node's LP. If the LP solution was integer-feasible, then it should already have been found by SCIP when it solved the LP in the first place.
Note: I actually ran some tests with and without those lines, and it appears this is not true. While adding those lines indeed reduces (slightly) the number of LP iterations, it also increase the number of nodes...
Solving 10 instances, with 500 empty actions at each node, without those lines:
nnodes 219
lpiters 3553
time 16.094735185
Same instances, same seed, with those lines:
nnodes 223
lpiters 3466
time 13.932313034
Users providing an empty action should be a rare situation I believe, especially 500 times in a row for each node. Also it seems SCIP handles nicely the re-solving of the same LP several times in a row. Should I remove those lines then, to have the code simpler and the environment's behavior more consistent (always solving the LP) ?
A very general, low priority, comment (but A better split would be that |
Co-authored-by: Antoine Prouvost <[email protected]>
Co-authored-by: Antoine Prouvost <[email protected]>
Co-authored-by: Antoine Prouvost <[email protected]>
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.
LGTM. Thanks for passing over the other docstrings!
Documentation tests are failing because https://www.scipopt.org/ is down (hence making broken link) but I think we can merge. |
Closes #182 |
Pull request checklist
Proposed implementation
The environment is implemented though a primal heuristic plugin. The environment deviates little from the behavior of a primal heuristic, except that it offers an option for the primal search to be called several times (users can be allowed several trials, i.e., several actions, at each node). Also, users have the option to provide only a partial solution, which SCIP will try to complete if possible by fixing the variables in the partial solution and then solving an LP.