Skip to content
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.

Sign up for GitHub

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

Improve performance of coefficient modification #135

Merged
merged 3 commits into from
Jul 25, 2018
Merged

Improve performance of coefficient modification #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9e1481
improve performance of constraint LHS modification
rdeits Jul 24, 2018
b05e110
fix deprecations
rdeits Jul 24, 2018
726908b
more tests with non-Cint indices
rdeits Jul 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/grb_common.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ ivec(v::IVec) = v
fvec(v::FVec) = v
cvec(v::CVec) = v

ivec(v::Vector) = convert(IVec, v)
fvec(v::Vector) = convert(FVec, v)
cvec(v::Vector) = convert(CVec, v)
ivec(v::AbstractVector) = convert(IVec, v)
fvec(v::AbstractVector) = convert(FVec, v)
cvec(v::AbstractVector) = convert(CVec, v)

ivec(s::Integer) = Cint[s]

Expand Down
38 changes: 30 additions & 8 deletions src/grb_constrs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ function get_sos(model::Model, start::Integer, len::Integer)
n = num_vars(model)
numnzP = Array{Cint}(1)
cbeg = Array{Cint}(len+1)

ret = @grb_ccall(getsos, Cint, (
Ptr{Void},
Ptr{Cint},
Expand Down Expand Up @@ -314,11 +314,23 @@ function del_constrs!(model::Model, idx::Vector{Cint})
end
end

function chg_coeffs!(model::Model, cidx::Real, vidx::Real, val::Real)
Copy link
Member

Choose a reason for hiding this comment

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

Are these types okay? Shouldn't cidx and vidx be Cints and val be a Float64?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The types specified in the ccall call are Ref{Cint} and Ref{Float64}, so Julia will automatically insert a call to Base.cconvert which will do the correct conversion (or fail with a MethodError). For example:

julia> Base.cconvert(Ref{Cint}, 1)
Base.RefValue{Int32}(1)

julia> Base.cconvert(Ref{Cint}, Int8(1))
Base.RefValue{Int32}(1)

julia> Base.cconvert(Ref{Cint}, 1.0)
Base.RefValue{Int32}(1)

Copy link
Member

Choose a reason for hiding this comment

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

Has it always done this? I'm sure I've been stung by it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure. I think a lot of this code was written before I'd even heard of Julia...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah a lot of it can be cleaned up

ret = @grb_ccall(chgcoeffs, Cint, (
Ptr{Void},
Cint,
Ref{Cint},
Ref{Cint},
Ref{Float64}),
model, 1, cidx - 1, vidx - 1, val)
if ret != 0
throw(GurobiError(model.env, ret))
end
end

chg_coeffs!{T<:Real, S<:Real}(model::Model, cidx::T, vidx::T, val::S) = chg_coeffs!(model, Cint[cidx], Cint[vidx], Float64[val])
chg_coeffs!{T<:Real, S<:Real}(model::Model, cidx::Vector{T}, vidx::Vector{T}, val::Vector{S}) = chg_coeffs!(model, convert(Vector{Cint},cidx), convert(Vector{Cint},vidx), fvec(val))
function chg_coeffs!(model::Model, cidx::Vector{Cint}, vidx::Vector{Cint}, val::FVec)
chg_coeffs!(model::Model, cidx::AbstractVector{<:Real}, vidx::AbstractVector{<:Real}, val::AbstractVector{<:Real}) =
chg_coeffs!(model, ivec(cidx), ivec(vidx), fvec(val))

function chg_coeffs!(model::Model, cidx::IVec, vidx::IVec, val::FVec)
(length(cidx) == length(vidx) == length(val)) || error("Inconsistent argument dimensions.")

numchgs = length(cidx)
Expand All @@ -335,6 +347,7 @@ function chg_coeffs!(model::Model, cidx::Vector{Cint}, vidx::Vector{Cint}, val::
end

function getcoeff!(val::FVec, model::Model, cidx::Integer, vidx::Integer)
Base.depwarn("getcoeff!(val, model, cidx, vidx) is deprecated. Instead you can retrieve a coefficient without allocating a vector by doing `val = getcoeff(model, cidx, vidx)`", :grb_getcoeff)
@assert length(val) == 1
ret = @grb_ccall(getcoeff, Cint, (
Ptr{Void},
Expand All @@ -346,8 +359,17 @@ function getcoeff!(val::FVec, model::Model, cidx::Integer, vidx::Integer)
throw(GurobiError(model.env, ret))
end
end

function getcoeff(model::Model, cidx::Integer, vidx::Integer)
Copy link
Member

Choose a reason for hiding this comment

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

Same with these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just added a couple more tests to verify that this works fine even when cidx and vidx are not ::Cint.

out = Array{Float64}(1)
getcoeff!(out::FVec, model::Model, cidx::Integer, vidx::Integer)
return out[1]
end
out = Ref{Float64}()
ret = @grb_ccall(getcoeff, Cint, (
Ptr{Void},
Cint,
Cint,
Ref{Float64}),
model, cidx - 1, vidx - 1, out)
if ret != 0
throw(GurobiError(model.env, ret))
end
out[]
end
12 changes: 6 additions & 6 deletions test/MathProgBase/large_coefficients.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@ using Gurobi, MathProgBase, Base.Test
# Min 1.1e100x
# x >= 1
m = MathProgBase.LinearQuadraticModel(GurobiSolver())
MathProgBase.loadproblem!(m, Array(Float64, (0, 1)), [1],[Inf], [1.1e100], Float64[], Float64[], :Min)
MathProgBase.loadproblem!(m, Array{Float64}(0, 1), [1],[Inf], [1.1e100], Float64[], Float64[], :Min)
MathProgBase.optimize!(m)
@test MathProgBase.getsolution(m) == [1.0]
@test MathProgBase.getobjval(m) == 1e100

# Max -1.1e100x
# x >= 1
m = MathProgBase.LinearQuadraticModel(GurobiSolver())
MathProgBase.loadproblem!(m, Array(Float64, (0, 1)), [1],[Inf], [-1.1e100], Float64[], Float64[], :Max)
MathProgBase.loadproblem!(m, Array{Float64}(0, 1), [1],[Inf], [-1.1e100], Float64[], Float64[], :Max)
MathProgBase.optimize!(m)
@test MathProgBase.getsolution(m) == [1.0]
@test MathProgBase.getobjval(m) == -1e100
Expand All @@ -24,28 +24,28 @@ using Gurobi, MathProgBase, Base.Test
# Min x
# x >= 1.1e30
m = MathProgBase.LinearQuadraticModel(GurobiSolver())
MathProgBase.loadproblem!(m, Array(Float64, (0, 1)), [1.1e30],[Inf], [1], Float64[], Float64[], :Min)
MathProgBase.loadproblem!(m, Array{Float64}(0, 1), [1.1e30],[Inf], [1], Float64[], Float64[], :Min)
MathProgBase.optimize!(m)
@test MathProgBase.status(m) == :Infeasible

# Max x
# x <= -1.1e30
m = MathProgBase.LinearQuadraticModel(GurobiSolver())
MathProgBase.loadproblem!(m, Array(Float64, (0, 1)), [-Inf],[-1.1e30], [1], Float64[], Float64[], :Max)
MathProgBase.loadproblem!(m, Array{Float64}(0, 1), [-Inf],[-1.1e30], [1], Float64[], Float64[], :Max)
MathProgBase.optimize!(m)
@test MathProgBase.status(m) == :Infeasible

# Min x
# x >= -1.1e30
m = MathProgBase.LinearQuadraticModel(GurobiSolver())
MathProgBase.loadproblem!(m, Array(Float64, (0, 1)), [-1.1e30],[Inf], [1], Float64[], Float64[], :Min)
MathProgBase.loadproblem!(m, Array{Float64}(0, 1), [-1.1e30],[Inf], [1], Float64[], Float64[], :Min)
MathProgBase.optimize!(m)
@test MathProgBase.status(m) == :Unbounded

# Max x
# x <= 1.1e30
m = MathProgBase.LinearQuadraticModel(GurobiSolver())
MathProgBase.loadproblem!(m, Array(Float64, (0, 1)), [-Inf],[1.1e30], [1], Float64[], Float64[], :Max)
MathProgBase.loadproblem!(m, Array{Float64}(0, 1), [-Inf],[1.1e30], [1], Float64[], Float64[], :Max)
MathProgBase.optimize!(m)
@test MathProgBase.status(m) == :Unbounded

Expand Down
51 changes: 51 additions & 0 deletions test/constraint_modification.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
using Gurobi
using Base.Test
using Gurobi: getcoeff


@testset "changing coefficients" begin
A = spzeros(2, 2)
A[1, 1] = 1.0
A[1, 2] = 2.0
A[2, 1] = 3.0
b = [0.1, 0.2]
f = [0.0, 0.0]
model = gurobi_model(Gurobi.Env(); f=f, A=A, b=b)

@test getcoeff(model, 1, 1) == 1.0

# Verify that we can pass any kind of `::Integer` to `getcoeff`
@test getcoeff(model, 1, Int8(1)) == 1.0
@test getcoeff(model, UInt32(1), UInt32(1)) == 1.0

@test getcoeff(model, 1, 2) == 2.0
@test getcoeff(model, 2, 1) == 3.0
@test getcoeff(model, 2, 2) == 0.0

chg_coeffs!(model, 2, 1, 1.5)
# before updating the model, we still get the old coefficient value
@test getcoeff(model, 2, 1) == 3.0
# after updating the model, we see the new coefficient value
update_model!(model)
@test getcoeff(model, 2, 1) == 1.5

# Verify that we are automatically converting the scalars to
# Cint and Float64 as necessary
chg_coeffs!(model, Int8(2), Int128(1), Float32(2.0))
update_model!(model)
@test getcoeff(model, 2, 1) == 2.0

chg_coeffs!(model, [1, 2], [1, 1], [5.0, 6.5])
update_model!(model)
@test getcoeff(model, 1, 1) == 5.0
@test getcoeff(model, 2, 1) == 6.5
@test getcoeff(model, 1, 2) == 2.0
@test getcoeff(model, 2, 2) == 0.0

chg_coeffs!(model, Int8[1, 2], Int8[1, 1], Float32[1.0, 2.0])
update_model!(model)
@test getcoeff(model, 1, 1) == 1.0
@test getcoeff(model, 2, 1) == 2.0
@test getcoeff(model, 1, 2) == 2.0
@test getcoeff(model, 2, 2) == 0.0
end
2 changes: 2 additions & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,5 @@ end
@testset "MathOptInterface Tests" begin
evalfile("MOIWrapper.jl")
end

include("constraint_modification.jl")