Conversation
…resolve-time unit test as it's not silent
…lve rule logging now correct for initial sweep
…r removed columns and rows
… sweep is performed
|
This closes #3040 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## latest #3167 +/- ##
==========================================
+ Coverage 72.72% 72.77% +0.05%
==========================================
Files 442 444 +2
Lines 106631 107416 +785
Branches 17173 17283 +110
==========================================
+ Hits 77544 78172 +628
- Misses 28810 28967 +157
Partials 277 277 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fwesselm
left a comment
fwesselm
left a comment
There was a problem hiding this comment.
I can't really comment on the changes to HFactor etc.
The presolve changes look reasonable, although I am wondering whether it would be better to have a member variable (like bool in_initial_sweep_), thereby avoiding changes to function signatures.
| std::copy(valuesNew.cbegin(), valuesNew.cend(), values.begin()); | ||
| #else | ||
| for (size_t i = index.size(); i < static_cast<size_t>(origSize); i++) | ||
| values[i] = zero; |
There was a problem hiding this comment.
values[i] = T{};
But, this would initialise the basis with HighsBasisStatus::kLower. I am not sure if this is problematic.
There was a problem hiding this comment.
It's not problematic.
The method scatters "values" for the presolved problem into their position in the original problem. The values not assigned in this process are for the rows and columns removed. Their values are assigned during postsolve, without reference to what was stored in the particular location - so it doesn't matter what was stored in that location. Indeed, previously, what was in the particular location was the value for a row or column in the presolved problem. I found this very confusing when developing.
The "it doesn't matter" argument is also reflected in the use of std::numeric_limits<T>::signaling_NaN() to fill these locations when DEBUG_EXTRA is assigned. This NaN assignment ensures that a check can be made (with bool containsNanOrInf) that a value is assigned in postsolve for all the (double) locations corresponding to rows and columns removed.
In the case of T = HighsBasisStatus, ideally zero would yield HighsBasisStatus::kNonbasic as that's never assigned by the simplex solver or crossover, so would be clearly "not assigned". However, this would require the definition of HighsBasisStatus to be changed so kNonbasic = 1 and, even if the values of kHighsBasisStatus* were changed in interfaces/highs_c_api.h there are sure to be users who are assuming that HighsBasisStatus::kLower casts to 0, who would then be messed up, so I'll live with initialising to HighsBasisStatus::kLower.
|
|
||
| double HPresolve::getMaxAbsColVal(HighsInt col) const { | ||
| double HPresolve::getMaxAbsColVal(HighsInt col, | ||
| const bool initial_sweep) const { |
There was a problem hiding this comment.
Maybe add a member variable bool bool in_initial_sweep_?
Thereby, the signatures of HPresolve::getMaxAbsColVal etc. would not need to be changed.
There was a problem hiding this comment.
Yes, this is more elegant. The explicit change was helpful in development
| --numCol; | ||
| void HighsPostsolveStack::compressIndexMap( | ||
| const std::vector<HighsInt>& newIndex, std::vector<HighsInt>& origIndex) { | ||
| // loop over entries, decrease entry counter for deleted entriess |
| size_t check_reduction = -35044; | ||
|
|
||
| auto solutionLogging = [&](const std::string& message) { | ||
| return; |
There was a problem hiding this comment.
The lambda directly returns?
There was a problem hiding this comment.
Yes, I see that it's unnecessary. That said, I've commented out the debugging lambdas and the calls to them to improve code coverage! I think they are worth keeping for future debugging
|
One consequence of this PR (@fwesselm and @Opt-Mucca) is that the initial sweep is always performed, so the behaviour of any solve involving presolve (for a problem with empty columns, fixed columns, empty rows, singleton rows or redundant rows) will change. Clearly this will change MIP performance (in particular) but the initial sweep is of very low cost if it doesn't yield reductions, and it's more efficient than achieving these reductions in "standard" presolve. Hence I think that
|
3 participants
Presolve "light" performs an initial sweep removing empty and fixed columns, and empty, singleton and redundant rows. For some practical problems, this removes significant numbers of rows and columns, and avoids the overhead of forming the general presolve data structure for the whole original model.
After the initial sweep, presolve light avoids applying the following expensive presolve rules