You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Patch Papilo to skip unchanged coefficients in sparsify - #2267
SparseStorage::changeRow in Papilo called the coefficient-changed callback even when the merged value equalled the old one. The Sparsify activity update asserts the two differ, so a sub-MIP presolve aborted in builds with assertions on (the INCUMBENT_CALLBACK_TEST crash in mip_solve.initial_solution_survives_papilo_crush, seen in CI run 41664476906 on #2247).
This adds a small patch that only calls the callback when the value changes. Release builds are unaffected, since they have no asserts and an unchanged-value callback did nothing harmful.
I reproduced the crash about 1 in 100 runs on an assertion-enabled build of main. With the patch, 500 runs had no Papilo abort, and 4 runs hit the old condition and passed.
Upstream: scipopt/papilo#103 (issue) and scipopt/papilo#104 (PR). When a Papilo update includes it, this patch and the PATCH_COMMAND can be removed.
The other abort seen in those runs, current_incumbent.size() == leaf_problem.num_cols, is #1909 and is not changed here.
changeRow called the coefficient-changed callback even when the merged
value equalled the old one. The Sparsify activity update asserts that the
old and new coefficients differ, so sub-MIP presolve aborted in builds with
assertions on (INCUMBENT_CALLBACK_TEST, about 1 in 100 runs). Only call the
callback when the value changes.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
No actionable comments were generated in the recent review.
Walkthrough
The Papilo SparseStorage merge path now calls coeffChanged only when the merged coefficient differs from its previous value. CMake applies this patch to the fetched Papilo source.
NVIDIA/cuopt#2203: Changes the pinned Papilo revision and patch list in the same FetchContent_Declare block.
Suggested labels:bug, non-breaking
Suggested reviewers:kh4ster, tmckayus
Merge Risk: Minimal · up to c2fae
The intended Papilo callback guard and patch wiring introduce no identified merge-blocking risk.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title "Patch Papilo to skip unchanged coefficients in sparsify" matches the main change: a new patch to Papilo's SparseStorage so the coefficient-changed callback runs only when the merged value d…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Description check
The description directly explains the Papilo patch, the unchanged-coefficient callback issue, the assertion failure, testing, and the intended upstream follow-up. It matches the changeset.
Full details: Title check
Explanation
The title "Patch Papilo to skip unchanged coefficients in sparsify" matches the main change: a new patch to Papilo's SparseStorage so the coefficient-changed callback runs only when the merged value differs from the old value. It names the patched library, the behavior change, and the context (sparsify). It is specific and does not include file lists or vague terms.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
Reviewing files that changed from the base of the PR and between 3d71290 and c2faefe.
Files selected for processing: 2
cpp/CMakeLists.txt
cpp/patches/papilo_skip_unchanged_coeff.patch
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bugSomething isn't workingnon-breakingIntroduces a non-breaking change
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
SparseStorage::changeRowin Papilo called the coefficient-changed callback even when the merged value equalled the old one. The Sparsify activity update asserts the two differ, so a sub-MIP presolve aborted in builds with assertions on (theINCUMBENT_CALLBACK_TESTcrash inmip_solve.initial_solution_survives_papilo_crush, seen in CI run 41664476906 on #2247).This adds a small patch that only calls the callback when the value changes. Release builds are unaffected, since they have no asserts and an unchanged-value callback did nothing harmful.
I reproduced the crash about 1 in 100 runs on an assertion-enabled build of main. With the patch, 500 runs had no Papilo abort, and 4 runs hit the old condition and passed.
Upstream: scipopt/papilo#103 (issue) and scipopt/papilo#104 (PR). When a Papilo update includes it, this patch and the
PATCH_COMMANDcan be removed.The other abort seen in those runs,
current_incumbent.size() == leaf_problem.num_cols, is #1909 and is not changed here.Checklist
🤖 Generated with Claude Code