-
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
Coefficient Strengthening #14
Conversation
-Implement and create tests for coef strengthening -Ignore ranged constraints (i.e., d <= a'x <= b)
-Add coef strengthening to presolve.jl -Add testset
src/mip/coefficient_strengthening.jl
Outdated
@@ -0,0 +1,99 @@ | |||
function maximal_activity(row::RowOrCol{T}, j::Int, ucol::Vector{T}, lcol::Vector{T})::T where {T} |
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.
What is this function doing exactly? Are you trying to compute the maximum activity of a given row?
If so, your implementation is not correct: there may be some variables that have been removed from the problem.
You need to explicitly check this with ps.colflag
.
I suggest you use the code that's currently in lp/forcing_row
:
# Implied row bounds
row = ps.pb0.arows[i]
l_ = u_ = zero(T)
for (j, aij) in zip(row.nzind, row.nzval)
ps.colflag[j] || continue
if aij < zero(T)
l_ += aij * ps.ucol[j]
u_ += aij * ps.lcol[j]
else
l_ += aij * ps.lcol[j]
u_ += aij * ps.ucol[j]
end
isfinite(l_) || isfinite(u_) || break # infinite bounds
end
You need access to ps
(the PresolveData
struct), and the row row
you're interested in (or its index i
).
At the end of this loop, you get l_
and u_
which are the lower and upper activity bounds on row i
.
I you want to skip variable j0
in the computation, just update the check to
(ps.colflag[j] && j != j0) || continue
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.
To @mtanneau's point, a short docstring above the method definition would be helpful to understand what this function does.
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.
It would be good to move the code snippet from forcing_row
to a separate util.jl file.
src/presolve.jl
Outdated
@@ -414,6 +415,9 @@ function presolve!(ps::PresolveData{T}) where {T} | |||
# Round the bounds of integer variables are integers. | |||
round_integer_bounds!(ps) | |||
|
|||
# Coeficient strengthening |
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.
# Coeficient strengthening | |
# Coefficient strengthening |
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.
Bump
src/presolve.jl
Outdated
Perform coefficient strengthening for integer/binary variables | ||
on every constraints. | ||
|
||
Called once at the very beginning of the presolve procedure. |
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.
Called once at the very beginning of the presolve procedure. | |
Called once at the beginning of the presolve procedure. |
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.
Bump
src/mip/coefficient_strengthening.jl
Outdated
function single_row_strengthening(row::RowOrCol{T}, b::T, m::Int, j::Int, ucol::Vector{T}, lcol::Vector{T}) where {T} | ||
# perform coef strengthening for one constraints of the from a'x < b | ||
# and return new a' and b' | ||
flag = false # wether there is an update |
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.
Two spaces after the end of the line + typo
flag = false # wether there is an update | |
flag = false # whether there is an update |
src/mip/coefficient_strengthening.jl
Outdated
|
||
m = findfirst(isequal(j), row.nzind) | ||
if m == nothing || (lrow > -inf && urow < inf) | ||
continue #skipping ranged constraints and |
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.
continue #skipping ranged constraints and | |
continue # skipping ranged constraints and |
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.
and?
src/mip/coefficient_strengthening.jl
Outdated
|
||
# update collumns of A in problem data | ||
# and update the sparse matrix in the case where the new coefficient is close to 0 | ||
if abs(new_coef) <= eps(T) |
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.
We should use a drop tolerance here.
cc @joehuchette
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.
Agreed. The eps(T)
should be a configurable tolerance in PresolveOptions
. Also, this should likely be a separate DropSmallCoefficients
Rule.
There is some work to do before this can be merged.
[EDIT: dropping range constraints] |
src/mip/coefficient_strengthening.jl
Outdated
@@ -0,0 +1,99 @@ | |||
function maximal_activity(row::RowOrCol{T}, j::Int, ucol::Vector{T}, lcol::Vector{T})::T where {T} |
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.
To @mtanneau's point, a short docstring above the method definition would be helpful to understand what this function does.
src/mip/coefficient_strengthening.jl
Outdated
@@ -0,0 +1,99 @@ | |||
function maximal_activity(row::RowOrCol{T}, j::Int, ucol::Vector{T}, lcol::Vector{T})::T where {T} |
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.
It would be good to move the code snippet from forcing_row
to a separate util.jl file.
src/mip/coefficient_strengthening.jl
Outdated
end | ||
|
||
function single_row_strengthening(row::RowOrCol{T}, b::T, m::Int, j::Int, ucol::Vector{T}, lcol::Vector{T}) where {T} | ||
# perform coef strengthening for one constraints of the from a'x < b |
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.
# perform coef strengthening for one constraints of the from a'x < b | |
# perform coefficient strengthening for one constraint of the form a'x \leq b |
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.
What is the mathematical definition for the coefficient strengthening you are doing? Can you include it in the docstring?
src/mip/coefficient_strengthening.jl
Outdated
a = deepcopy(row.nzval) | ||
if a[m] > 0 | ||
d = b - maximal_activity(row, j, ucol, lcol) - a[m]*(ucol[j]-1) | ||
if a[m] >= d && d > 0 |
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.
Small suggestion: These can be chained as a[m] >= d > 0
src/mip/coefficient_strengthening.jl
Outdated
row.nzval = -a | ||
ps.lrow[i] = -b | ||
|
||
# update collumns of A in problem data |
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.
# update collumns of A in problem data | |
# update columns of A in problem data |
C = T[1, 1, 1, 1] | ||
lc = T[0, 1, 0, 2] | ||
uc = T[1, 5//2, 1, 3] | ||
lr = T[-1e30, -1e30] |
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.
Update for new Inf standard
|
||
|
||
@test ps.urow == T[5, 9//2] | ||
@test ps.lrow == T[-1e30, -1e30] |
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.
Ditto
C = T[1, 1, 1, 1] | ||
lc = T[0, 0, 0, -3] | ||
uc = T[2, 1, 2, 10^30] | ||
lr = T[-1e30, -1e30] |
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.
Again
lc = T[-4, -3//2, 9, 0, -1] | ||
uc = T[2, 1, 15, 1, 1] | ||
lr = T[2, -1e30] | ||
ur = T[1e30, 13/3] |
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.
Again
My concern about ranged constraints is that if you apply the strengthening to both sides you will end up with distinct linear pieces, which would require us adding extra constraints to our representation. Is this not the case? If it is, is it worth the added complexity? |
Good point :)
If it's too much hassle at this point we can drop it. |
@Anhtu07: Thoughts? My initial (offline) suggestion was to punt completely on range constraints for the time being; I hadn't considered the "yes" branch. It's worth thinking about. |
I think there will be conflict most of the time when using CS on ranged constraints. For example, consider |
All right, I'm convinced. Let's skip range-constraints. |
- using Inf instead of 1e30
Codecov Report
@@ Coverage Diff @@
## master #14 +/- ##
==========================================
+ Coverage 46.83% 52.62% +5.78%
==========================================
Files 14 15 +1
Lines 901 1011 +110
==========================================
+ Hits 422 532 +110
Misses 479 479
Continue to review full report at Codecov.
|
src/mip/coefficient_strengthening.jl
Outdated
@@ -52,9 +52,9 @@ function coefficient_strengthening!(ps::PresolveData{T}, j::Int, inf::T = T(1e30 | |||
urow = ps.urow[i] | |||
|
|||
m = findfirst(isequal(j), row.nzind) | |||
if m == nothing || (lrow > -inf && urow < inf) | |||
if m == nothing || (lrow > -Inf && urow < Inf) |
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.
just to ensure type-stability
if m == nothing || (lrow > -Inf && urow < Inf) | |
if m == nothing || (lrow > T(-Inf) && urow < T(Inf)) |
src/mip/coefficient_strengthening.jl
Outdated
continue #skipping ranged constraints and | ||
elseif urow < inf | ||
elseif urow < Inf |
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.
elseif urow < Inf | |
elseif urow < T(Inf) |
src/mip/coefficient_strengthening.jl
Outdated
@@ -73,7 +73,7 @@ function coefficient_strengthening!(ps::PresolveData{T}, j::Int, inf::T = T(1e30 | |||
ps.pb0.acols[j].nzval[k] = new_coef | |||
end | |||
end | |||
elseif lrow > -inf | |||
elseif lrow > -Inf |
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.
elseif lrow > -Inf | |
elseif lrow > T(-Inf) |
- remove deepcopy - perform coefficient strengthening row by row instead of col by col
- no longer using findfirst, which may cost nzlog(nz) worst-case time complexity
- Update way in which the maximal activity of a row except variable j is compute
Co-authored-by: mtanneau <9593025+mtanneau@users.noreply.github.com>
- update all problem's parameters in the same block
src/mip/coefficient_strengthening.jl
Outdated
Perform coefficient strengthening on each row and on each integer variables | ||
|
||
In particular, for the i-th constraints of the form | ||
aᵢ x ⩽ bᵢ and let xⱼ be a integer/binary variable |
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.
End sentence with a period.
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.
Bump
src/mip/coefficient_strengthening.jl
Outdated
aᵢⱼ ≔ aᵢⱼ - d | ||
bᵢ ≔ bᵢ - duⱼ | ||
Also perform coefficient strengthening on constraints of the form aᵢ x ⩾ cᵢ | ||
with similar update rule |
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.
End with a period
src/mip/coefficient_strengthening.jl
Outdated
Also perform coefficient strengthening on constraints of the form aᵢ x ⩾ cᵢ | ||
with similar update rule | ||
|
||
Skip ranged constraints, i.e, cᵢ ⩽ aᵢ x ⩽ bᵢ |
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.
Make a complete sentence (add "We" at beginning, end with a period).
src/mip/coefficient_strengthening.jl
Outdated
|
||
Skip ranged constraints, i.e, cᵢ ⩽ aᵢ x ⩽ bᵢ | ||
|
||
The new coefficients are stored by updating ps.pb0 |
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.
Period
src/mip/coefficient_strengthening.jl
Outdated
where Mᵢⱼ is maximal activity of the i-th row without considering xⱼ | ||
and uⱼ is upper bound of xⱼ, then | ||
aᵢⱼ ≔ aᵢⱼ - d | ||
bᵢ ≔ bᵢ - duⱼ |
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 currently reads as an identity; I would rephrase to make clear that this is the transformation you apply.
bᵢ ≔ bᵢ - duⱼ | |
and uⱼ is upper bound of xⱼ, then we transform | |
aᵢⱼ <- aᵢⱼ - d | |
bᵢ <- bᵢ - duⱼ |
src/mip/coefficient_strengthening.jl
Outdated
return new_coef, new_bound | ||
end | ||
|
||
function lowerbound_strengthening!(ps::PresolveData{T}, i::Int, j_index::Int, j::Int, min_act) where {T} |
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.
Does this actually modify ps
? If not, remove the !
. (And similarly for upperbound)
src/mip/coefficient_strengthening.jl
Outdated
end | ||
#update sup | ||
if coef > 0 | ||
sup = sup - (coef - new_coef) * ps.ucol[j] |
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.
sup = sup - (coef - new_coef) * ps.ucol[j] | |
sup -= (coef - new_coef) * ps.ucol[j] |
src/mip/coefficient_strengthening.jl
Outdated
if coef > 0 | ||
sup = sup - (coef - new_coef) * ps.ucol[j] | ||
else | ||
sup = sup - (coef - new_coef) * ps.lcol[j] |
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.
sup = sup - (coef - new_coef) * ps.lcol[j] | |
sup -= (coef - new_coef) * ps.lcol[j] |
src/mip/coefficient_strengthening.jl
Outdated
end | ||
#update inf | ||
if coef > 0 | ||
inf = inf - (coef - new_coef) * ps.lcol[j] |
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.
inf = inf - (coef - new_coef) * ps.lcol[j] | |
inf -= (coef - new_coef) * ps.lcol[j] |
src/mip/coefficient_strengthening.jl
Outdated
if coef > 0 | ||
inf = inf - (coef - new_coef) * ps.lcol[j] | ||
else | ||
inf = inf - (coef - new_coef) * ps.ucol[j] |
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.
inf = inf - (coef - new_coef) * ps.ucol[j] | |
inf -= (coef - new_coef) * ps.ucol[j] |
src/mip/coefficient_strengthening.jl
Outdated
Perform coefficient strengthening on each row and on each integer variables | ||
|
||
In particular, for the i-th constraints of the form | ||
aᵢ x ⩽ bᵢ and let xⱼ be a integer/binary variable |
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.
Bump
src/mip/coefficient_strengthening.jl
Outdated
If a column is reduced to 0 after this step, it is set to be inactive | ||
|
||
""" | ||
function maximal_activity(row::Row{T}, ucol::Vector{T}, lcol::Vector{T})::T where {T} |
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.
Bump
src/mip/coefficient_strengthening.jl
Outdated
end | ||
end | ||
sup = ucol[nonzero_index]'pos_coef + lcol[nonzero_index]'neg_coef | ||
return T(sup) |
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.
Bump
src/mip/coefficient_strengthening.jl
Outdated
a[m] = a[m] - d | ||
b = b - d*ucol[j] | ||
flag = true | ||
function minimal_activity(row::Row{T}, ucol::Vector{T}, lcol::Vector{T})::T where {T} |
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.
Bump
src/mip/coefficient_strengthening.jl
Outdated
end | ||
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.
Bump
src/mip/coefficient_strengthening.jl
Outdated
end | ||
|
||
function zero_coefficient_strengthening!(ps::PresolveData{T}) where {T} | ||
# perform coefficient stregthening but if there is a coefficient is reduced to 0 |
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.
Bump
src/mip/coefficient_strengthening.jl
Outdated
|
||
function zero_coefficient_strengthening!(ps::PresolveData{T}) where {T} | ||
# perform coefficient stregthening but if there is a coefficient is reduced to 0 | ||
# it is still kept in the ps.pb0.arows and ps.pb0.acols |
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.
Bump
src/mip/coefficient_strengthening.jl
Outdated
sup = maximal_activity(row, ps.ucol, ps.lcol) | ||
inf = minimal_activity(row, ps.ucol, ps.lcol) |
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.
Bump
src/presolve.jl
Outdated
@@ -414,6 +415,9 @@ function presolve!(ps::PresolveData{T}) where {T} | |||
# Round the bounds of integer variables are integers. | |||
round_integer_bounds!(ps) | |||
|
|||
# Coeficient strengthening |
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.
Bump
src/presolve.jl
Outdated
Perform coefficient strengthening for integer/binary variables | ||
on every constraints. | ||
|
||
Called once at the very beginning of the presolve procedure. |
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.
Bump
@Anhtu07 There are some outstanding comments (I think) that I bumped. You can see them if you click the "Files Changed" tab up top. |
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.
One more thing: check whether rows/columns are active or not
Co-authored-by: mtanneau <9593025+mtanneau@users.noreply.github.com>
Co-authored-by: mtanneau <9593025+mtanneau@users.noreply.github.com>
src/presolve.jl
Outdated
coefficient_strengthening!(ps::PresolveData) | ||
|
||
Perform coefficient strengthening for integer/binary variables | ||
on every constraints. |
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.
on every constraints. | |
in every constraint. |
src/mip/coefficient_strengthening.jl
Outdated
return new_coef, new_bound | ||
end | ||
|
||
function zero_coefficient_strengthening!(ps::PresolveData{T}) where {T} |
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.
I'm not sure about this naming. It sounds like you're only applying coefficient strengthening on zero coefficients (maybe?). I would just call this _coefficient_strengthenening!
.
@@ -0,0 +1,178 @@ | |||
""" |
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.
The location of this docstring is a bit odd--usually it would go immediately before (no extra newline) before a function it's describing. Here, it's above maximal_activity
, but that's just really a helper function.
src/mip/coefficient_strengthening.jl
Outdated
If a row/column is reduced to 0 after this step, it will be set to be inactive. | ||
""" | ||
|
||
function maximal_activity(ps::PresolveData{T}, i::Int)::T where {T} |
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.
Consider moving maximal_activity
and minimal_activity
to util.jl
.
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.
I think we're reaching the end!
@Anhtu07 you will be the record holder for the longest PR :)
I have checked all the source code. Except for the removal of empty rows/columns, everything looks good. I have not checked the underlying math.
I have also not checked the correctness of tests, I trust you know what you're doing.
Co-authored-by: mtanneau <9593025+mtanneau@users.noreply.github.com>
Co-authored-by: Joey Huchette <joehuchette@gmail.com>
@mtanneau Thanks for letting me know, I will put that on my CV :) Trust me, future PR will not take as long you can review it one more time now. |
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
Given the large number of commits in this PR, I'd prefer we squash and merge, and clean the commit history when doing so.
Thanks for your hard work on this one, @Anhtu07 ! |
! Empty rows/columns are now stored with empty arrays for nzind and nzval