Skip to content

Add extra safety net for relaxing out bounds - #3196

Merged
Opt-Mucca merged 3 commits into
latestfrom
fix-3190
Aug 5, 2026
Merged

Add extra safety net for relaxing out bounds#3196
Opt-Mucca merged 3 commits into
latestfrom
fix-3190

Conversation

@Opt-Mucca

Copy link
Copy Markdown
Collaborator

This fixes the bug presented in #3190

What I think the problem is: A bug was fixed in #3149 to stop conflict analysis dropping an explanation that would've invalidated the conflict. This somehow has lead to the chain of conflicts exhausting itself, and having no guards against it stuff gets a bit funny when a -1 index appears. My change just returns false when this happens.

@Opt-Mucca
Opt-Mucca requested a review from fwesselm August 3, 2026 14:05
@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 61.90476% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.92%. Comparing base (187c624) to head (44798cd).
⚠️ Report is 127 commits behind head on latest.

Files with missing lines Patch % Lines
highs/mip/HighsDomain.cpp 50.00% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           latest    #3196      +/-   ##
==========================================
- Coverage   72.92%   72.92%   -0.01%     
==========================================
  Files         436      436              
  Lines      106038   106087      +49     
  Branches    17071    17077       +6     
==========================================
+ Hits        77329    77364      +35     
- Misses      28433    28447      +14     
  Partials      276      276              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fwesselm fwesselm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @Opt-Mucca, the changes are fine. Would it make sense to have a method for this? See here.

Comment thread highs/mip/HighsDomain.cpp Outdated
Comment thread highs/mip/HighsDomain.cpp Outdated
Comment thread highs/mip/HighsDomain.cpp Outdated
Comment thread highs/mip/HighsDomain.cpp Outdated
@Opt-Mucca

Copy link
Copy Markdown
Collaborator Author

@fwesselm I'm not sure whether the others can trigger, but I very much like having a function for this, and it's way safer than the existing assert! I'm super happy if you swap this PR for your changes.

@fwesselm

fwesselm commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

@fwesselm I'm not sure whether the others can trigger, but I very much like having a function for this, and it's way safer than the existing assert! I'm super happy if you swap this PR for your changes.

@Opt-Mucca, I could just pull my branch into this one, if that is OK for you.

@Opt-Mucca

Copy link
Copy Markdown
Collaborator Author

@fwesselm sounds good!

@Opt-Mucca
Opt-Mucca merged commit 604fae2 into latest Aug 5, 2026
675 of 678 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants