Add single machine scheduling separator - #2990
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## latest #2990 +/- ##
==========================================
- Coverage 72.92% 72.89% -0.04%
==========================================
Files 436 438 +2
Lines 106038 106283 +245
Branches 17071 17118 +47
==========================================
+ Hits 77329 77470 +141
- Misses 28433 28536 +103
- Partials 276 277 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I've added some basic clique extraction (was the largest TODO). It now solves the benchmark instance From this you can conclude that |
BenChampion
left a comment
There was a problem hiding this comment.
Thanks for asking me to review this change! It was very educational. (The quantity of comments reflects that: I added the comments as I worked through it.)
To summarize my feedback, some parts of this were much harder to understand than others. It might help to delineate as much as possible
- code that identifies the single machine scheduling structure, ensuring we don't try to build the cuts where they won't be helpful
- code that computes the data we need once we've identified the structure
- the complete preconditions for the valid inequalities
- ideas for each step, intents behind data structures, and your algorithmic strategy
In due course, testing results would also be good to see.
Once you've had a chance to digest my feedback I'd be happy to chat. (It would certainly help me to fill any gaps in my understanding.)
| HPRESOLVE_CHECKED_CALL(static_cast<Result>(convertImpliedInteger(col))); | ||
|
|
||
| // shift integral variables to have a lower bound of zero | ||
| // shift "binary" variables to have a lower bound of zero |
There was a problem hiding this comment.
I see you have an outstanding comment to @fwesselm about this; I don't think I know enough yet to evaluate the implications of changing this.
There was a problem hiding this comment.
It is in general nice to have variables have a lower bound of zero. I don't see this making any mathematical difference though, and it completely changes the structure. I don't believe it's done in other solvers. The advantage would be that there's less changes needed to be done in stuff like cut transformation as they already have a lb of 0.
Consider that HiGHS has proven that one job has to start at at-least time 5. It will shift this 0. The job variable no longer represents the start time though, it now represents the start time + 5, which is impossible to recover easily. It can no longer be used to make the inequality.
| std::vector<double> processingTimes; | ||
| processingTimes.resize(largestDegree + 1, kHighsInf); | ||
| // Iterate over potential neighbours and check validity | ||
| for (HighsInt col : potentialNeighbours) { |
There was a problem hiding this comment.
Worth explaining in more detail your strategy here? (In particular, you are enlarging neighbors with every iteration.) I had to backtrack multiple times to figure out what this loop was doing.
There was a problem hiding this comment.
I've added a small comment
There was a problem hiding this comment.
Thanks for addressing my comments. This looks good to me now modulo some new comments/continuing discussions, and proper testing.
That just leaves the clique size at least three thing: why can't we form the inequality for two jobs? Is it just not useful? Or is it something else? (EDIT: this is only for my understanding)
| // 1 -> (i,j) y = 0, 2 -> (i,j) y = 1, 4 -> (j,i) y = 0, 8 -> (j,i) y = 1 | ||
| HighsHashTable<std::tuple<HighsInt, HighsInt, HighsInt>, uint8_t> jobOrder; | ||
|
|
||
| auto addEntry = [&](HighsInt posCol, HighsInt negCol, HighsInt binCol, |
There was a problem hiding this comment.
It's much harder to reason about what a function is doing if it can modify any value in scope rather than just three specific things.
| continue; | ||
| } | ||
| // Add cliques x1 + ~x2 <= 1 and ~x1 + x2 <= 1 which together imply x1 | ||
| // == x2 (depending on signs may also have x1 == ~x2) |
There was a problem hiding this comment.
This last addition is a little confusing but I think I know what you mean (that the original variables may already be complemented with respect to each other)
| // My_ji + s_i - s_j >= p_ji, p_ji >= 0 | ||
| // y_ji = 1 -> s_i >= s_j + p_jj - M |
There was a problem hiding this comment.
| // My_ji + s_i - s_j >= p_ji, p_ji >= 0 | |
| // y_ji = 1 -> s_i >= s_j + p_jj - M | |
| // My_ji + s_i - s_j >= p_ji, p_ji >= 0 | |
| // y_ji = 1 -> s_i >= s_j + p_ji - M |
Also, don't you want M(1-y_ji)?
There was a problem hiding this comment.
It's just re-arranging and setting y_ji = 1 so that it becomes M. Edit: Fixed the p_jj error
…HiGHS into machine-schedule-sepa
…ine-schedule-sepa
fwesselm
left a comment
There was a problem hiding this comment.
Looks good, @Opt-Mucca!
|
This improves performance by 1% or so on the test set of @fwesselm I'm now going to merge it |
This PR implements identification of natural-date single-machine scheduling subproblems and derives valid inequalities. See the description of
highs/mip/HighsMachineSchedSeparator.hfor the exact description.Motivation: The instance
neos-3046615-murgfrom MIPLIB 2017 benchmark now solves well within time limit, andneos-3046601-motufrom MIPLIB 2017 collection now solves in a second or two (both were previously unsolvable within a few hours). This change should introduce some tinyepsilonand very positively affect some instances.Changes:
highs/mip/HighsMachineSchedSeparator.handhighs/mip/HighsMachineSchedSeparator.cpp. Currently the separator is only called once, and the cuts are only added to the pool. This is efficient, and I noticed no real change by trying to separate more frequently.Potential improvements that I don't think need to be made:
[-inf, 0]instead of[0, inf]. I'd currently not spot these. I'd also not spot a row where both binary columns share the same coefficient value (different signs), although I think presolve is going to scale these rows most of the time already.HighsCliqueTable, which I don't think was efficient to use in this case (could be wrong). There's likely going to be some instances that this code could improve but fails to because of this naive approach, however it ensures near-zero overhead is added to all other instances. An example: I suspectsupportcase26could be improved with some better detection.This should not be merged until a more thorough performance check has been made! @fwesselm This is a way lower priority than the MIP Worker Refactor to review / test.