-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ensure_integer_bounds #8
Conversation
test/lp/ensure_integer_bounds.jl
Outdated
varTypes = [MathOptPresolve.GENERAL_INTEGER, MathOptPresolve.CONTINUOUS, | ||
MathOptPresolve.BINARY] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come varTypes
only has 3
elements and we're not getting an error when loading the problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed odd, would have thought this would catch it:
MathOptPresolve.jl/src/problem_data.jl
Lines 122 to 124 in 5a64d16
if var_types !== nothing | |
nvar == length(var_types) || error("") | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we figure out why this is happening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the first version, the test function wasn't called in runtests.jl.
test/lp/ensure_integer_bounds.jl
Outdated
varTypes = [MathOptPresolve.GENERAL_INTEGER, MathOptPresolve.CONTINUOUS, | ||
MathOptPresolve.BINARY] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed odd, would have thought this would catch it:
MathOptPresolve.jl/src/problem_data.jl
Lines 122 to 124 in 5a64d16
if var_types !== nothing | |
nvar == length(var_types) || error("") | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as naming goes, I would prefer something in the likes of "Round integer bounds", following, e.g., the convention in this paper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@BochuanBob there are merge conflicts now that #7 landed. Thankfully, looks like they should be easy to fix. Can you fixup the PR? We can switch to the |
Codecov Report
@@ Coverage Diff @@
## master #8 +/- ##
==========================================
+ Coverage 45.17% 46.56% +1.39%
==========================================
Files 12 13 +1
Lines 870 902 +32
==========================================
+ Hits 393 420 +27
- Misses 477 482 +5
Continue to review full report at Codecov.
|
Thanks, @BochuanBob, nice work! |
Added a function to ensure the bounds of integer variables to be integral.