GF2 presolve rank-deficiency fix - #1660
Conversation
|
/ok to test 1e6415d |
📝 WalkthroughWalkthroughThe PR replaces the square GF(2) solver with a rectangular rank-aware implementation, adds validated presolve fixings and extensive coverage, and introduces optional Papilo reduction-method allowlists for main and simplex-subproblem presolve paths. ChangesGF(2) presolve
Papilo reduction allowlist
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tests/mip/gf2_presolve_test.cpp (1)
523-542: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the test name and comment with the asserted status.
The test is named
more_rows_than_bins_reducesand the comment says the third row is redundant, but the assertion expectsOPTIMAL, notREDUCED. The presolver fully solves this instance, soOPTIMALis the real outcome. Rename the test to reflect that, or state in the comment why presolve reachesOPTIMALhere. This keeps a future reader from treating anOPTIMAL-to-REDUCEDchange as a test bug.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/mip/gf2_presolve_test.cpp` around lines 523 - 542, Rename the test `more_rows_than_bins_reduces` to reflect that presolve fully solves the instance with `OPTIMAL`, and update the comment so it no longer describes only a redundant row without explaining the optimal outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/tests/mip/gf2_presolve_test.cpp`:
- Around line 523-542: Rename the test `more_rows_than_bins_reduces` to reflect
that presolve fully solves the instance with `OPTIMAL`, and update the comment
so it no longer describes only a redundant row without explaining the optimal
outcome.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0a583e30-9d20-488e-b0ca-9142b3365d5c
📒 Files selected for processing (7)
cpp/src/mip_heuristics/presolve/gf2_presolve.cppcpp/src/mip_heuristics/presolve/gf2_presolve.hppcpp/src/mip_heuristics/presolve/third_party_presolve.cppcpp/src/mip_heuristics/presolve/third_party_presolve.hppcpp/tests/internal/CMakeLists.txtcpp/tests/mip/gf2_presolve_test.cppcpp/tests/mip/presolve_test.cu
💤 Files with no reviewable changes (1)
- cpp/tests/mip/presolve_test.cu
CI Test Summary✅ All 31 test job(s) passed. |
| papilo::Presolve<f_t>& presolver, | ||
| problem_category_t category, | ||
| bool dual_postsolve, | ||
| std::optional<std::unordered_set<std::string>> const& method_allowlist = std::nullopt) |
There was a problem hiding this comment.
Are you using the allowlist for anything? Maybe a better approach is pass all the desired reductions as a list (empty = all) and set the presolve methods based on that using a loop + switch case
There was a problem hiding this comment.
I went with this path since it allows expressing the whitelist as strings, without needing to pull any Papilo headers or machinery on the calling side
There was a problem hiding this comment.
My suggestion is to use just a vector of string (or enums) and then translate from there, e.g.,
std::vector<std::string> reductions = {"SingletonCols", "GF2Presolve"};
set_presolve_methods(reductions);and
void set_presolve_methods(reductions) {
for(auto r : reductions) {
if (r == "SingletonCols") presolver.addPresolveMethod(uptr(new papilo::SingletonCols<f_t>()));
else if ...
}
}There was a problem hiding this comment.
The idea here being that we can just crosscheck against the presolver names reported by Papilo reductions, so that we don't have to manually if-else over all possible options
| i_t num_cpu_threads = 0); | ||
|
|
||
| // If set, only Papilo methods whose getName() is listed are registered | ||
| void set_reduction_allowlist(std::optional<std::unordered_set<std::string>> allowlist) |
There was a problem hiding this comment.
It this just for the tests or are we planning to expose it to user or developers with some config?
There was a problem hiding this comment.
Just for testing convenience, I wasn't too sure how to do this best to be fair :/ Seemed like the least bad option. But it will be useful for later presolver pass PRs
|
/merge |
4 participants
The GF(2) presolver treated a missing pivot column as proof of infeasibility, so rank-deficient but consistent GF2 blocks were wrongly reported infeasible. It now runs a full RREF that separates inconsistency from underdetermination and fixes only variables whose value is identical in every solution.
The GF2 presolver also now supports rectangular matrices, and test coverage has been expanded.
Description
Issue
Checklist