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.

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

Re-write the MOI wrapper #101

Merged
merged 18 commits into from
Aug 13, 2019
Merged

Re-write the MOI wrapper #101

merged 18 commits into from
Aug 13, 2019

Conversation

odow
Copy link
Member

@odow odow commented Jun 17, 2019

Following in the footsteps of jump-dev/Gurobi.jl#216

Benchmarks compared to LQOI (part of this improvement comes from disabling GLPK.jl's pre-emptive checks).

========== Results ==========

delete_constraint
BenchmarkTools.TrialJudgement: 
  time:   -86.29% => improvement (5.00% tolerance)
  memory: -36.28% => improvement (1.00% tolerance)

add_variable_constraints
BenchmarkTools.TrialJudgement: 
  time:   -43.03% => improvement (5.00% tolerance)
  memory: -56.50% => improvement (1.00% tolerance)

add_variables
BenchmarkTools.TrialJudgement: 
  time:   -43.54% => improvement (5.00% tolerance)
  memory: -36.22% => improvement (1.00% tolerance)

add_constraint
BenchmarkTools.TrialJudgement: 
  time:   -60.16% => improvement (5.00% tolerance)
  memory: -54.59% => improvement (1.00% tolerance)

add_variable
BenchmarkTools.TrialJudgement: 
  time:   -52.77% => improvement (5.00% tolerance)
  memory: -38.20% => improvement (1.00% tolerance)

delete_variable_constraint
BenchmarkTools.TrialJudgement: 
  time:   -81.85% => improvement (5.00% tolerance)
  memory: -96.20% => improvement (1.00% tolerance)

add_variable_constraint
BenchmarkTools.TrialJudgement: 
  time:   -76.90% => improvement (5.00% tolerance)
  memory: -89.06% => improvement (1.00% tolerance)

add_constraints
BenchmarkTools.TrialJudgement: 
  time:   -44.17% => improvement (5.00% tolerance)
  memory: -39.17% => improvement (1.00% tolerance)

delete_variable
BenchmarkTools.TrialJudgement: 
  time:   -36.80% => improvement (5.00% tolerance)
  memory: +2591.58% => regression (1.00% tolerance)

Closes #102
Closes #99
Closes #95
Closes #93
Closes #92

@coveralls
Copy link

coveralls commented Jun 17, 2019

Coverage Status

Coverage increased (+8.2%) to 76.934% when pulling 9b2b4f8 on od/moi9 into a90525d on master.

src/MOI_wrapper.jl Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Jun 17, 2019

Codecov Report

Merging #101 into master will increase coverage by 5.54%.
The diff coverage is 88.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
+ Coverage   75.44%   80.99%   +5.54%     
==========================================
  Files           4        4              
  Lines        1409     1836     +427     
==========================================
+ Hits         1063     1487     +424     
- Misses        346      349       +3
Impacted Files Coverage Δ
src/infeasibility_certificates.jl 68.96% <100%> (+43.1%) ⬆️
src/MOI_wrapper.jl 88.86% <88.59%> (+5.38%) ⬆️
src/GLPK.jl 78.59% <0%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a90525d...4e62866. Read the comment docs.

MOIT.contlineartest(OPTIMIZER, MOIT.TestConfig(basis = true), [
# This requires an infeasiblity certificate for a variable bound.
"linear12",
# VariablePrimalStart not supported.
Copy link
Member

Choose a reason for hiding this comment

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

Is this a TODO or does GLPK not support providing feasible starting points at all?

Copy link
Member Author

@odow odow Jun 20, 2019

Choose a reason for hiding this comment

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

From what I can tell, you have to provide a complete valid basis, so this is a VariablePrimalStart not supported.

test/MOI_wrapper.jl Outdated Show resolved Hide resolved
type::TypeEnum
name::String
# Storage for constraint names associated with variables because GLPK
# can only store names for variables and proper constraints.
Copy link
Member

Choose a reason for hiding this comment

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

Now that single-variable constraints are tied to the index of the corresponding variable, I'm wondering if we need to support names for these constraints. You can always lookup the constraint by the variable index or name directly, and no solvers natively support names for bounds.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be happy to support this. But it means that you can't give a constraint the same name as a variable if you want to look it up.

Copy link
Member

Choose a reason for hiding this comment

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

But it means that you can't give a constraint the same name as a variable if you want to look it up.

I'm proposing to make setting ConstraintName on SingleVariable constraint into an error. This wouldn't change lookups otherwise.

To look up the bound on a variable, you first look up the variable. The index of the variable gives you the index of the bound constraint.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good.

src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI_wrapper.jl Outdated Show resolved Hide resolved

MOI.supports(::Optimizer, ::MOI.Name) = true
MOI.supports(::Optimizer, ::MOI.Silent) = true
MOI.supports(::Optimizer, ::MOI.ConstraintSet, c) = true
Copy link
Member

Choose a reason for hiding this comment

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

I am surprised there is no ambiguity error because you don't give the type of c

Copy link
Member

Choose a reason for hiding this comment

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

@odow odow merged commit f56b8c7 into master Aug 13, 2019
@odow odow deleted the od/moi9 branch August 13, 2019 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants