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

WIP: MOI #139

Closed
wants to merge 38 commits into from
Closed

WIP: MOI #139

wants to merge 38 commits into from

Conversation

odow
Copy link
Member

@odow odow commented Jul 21, 2017

My guess is that this will change a lot by the final form, and once we play around with implementing more wrappers.

This is now in fairly robust form. It passes virtually every MOI test (with the exception of some conic ones that aren't supported i.e. SOC, EXP etc).

Still to do:

  • callbacks

Dict{SVConstrRef{GE}, MOI.VariableReference}(),
Dict{SVConstrRef{EQ}, MOI.VariableReference}(),
Dict{SVConstrRef{MOI.Interval{Float64}}, MOI.VariableReference}()
)
Copy link
Member Author

Choose a reason for hiding this comment

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

@joaquimg what are your thoughts on this approach (and shouldn't you be asleep)?

Copy link
Member

Choose a reason for hiding this comment

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

I like it, as you saw, I was brainstorming with myself yesterday. My only problem with the Dictis that after deleting a variable one would have to loop the entire Dict. But the Array has the problem of possibly growing a lot. And a linked list might be slow to access a single element.
You could keep going with dicts and I got for arrays and then we compare, the interface are similar anyways...

Copy link
Member

@joaquimg joaquimg Jul 24, 2017

Choose a reason for hiding this comment

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

[solved] do you have a better name for: getconstraintlist(map::ConstraintMapping, ::MOI.ScalarAffineFunction{Float64}, ::MOI.EqualsTo{Float64}) = map.equal_to ?
You already had constraint_storage better indeed.

@joaquimg
Copy link
Member

I have a second question, I think adding constraints and variables, one by one might be slow, but I haven´t benchmarked yet... Planning to do that soon.

@joaquimg
Copy link
Member

I did the comparison for Xpress:
https://gist.github.com/joaquimg/5dbaf83e7555b94d79f4d01f8ed53119

It is a 10x difference!

can you do it for CPLEX and/or Gurobi?

@odow
Copy link
Member Author

odow commented Jul 24, 2017

That's still only half a milisecond to add 1000 variables though...

equal_to::Dict{LinConstrRef{EQ}, Int}

# references to variable
variable_upper_bound::Dict{SVConstrRef{LE}, MOI.VariableReference}
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that solvers could skip these mappings by matching up the constraint reference number with the variable reference number, since multiple bounds per variable are not allowed.

Copy link
Member Author

Choose a reason for hiding this comment

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

But can't I add a lower bound and a separate upper bound? Just not two upper bounds?

Copy link
Member

Choose a reason for hiding this comment

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

You can add a separate lower bound and upper bound. You can't add two upper bounds. The wrapper could store a variable attribute vector with the possible bound statuses per variable:

  • No bounds
  • Lower bound only
  • Upper bound only
  • Lower and upper bound, separately
  • Lower and upper bound via EqualTo
  • Lower and upper bound via Interval

You can check if a ConstraintReference{SingleVariable,GreaterThan} is valid by checking the bound flag of the corresponding variable (only if lower bound only, or lower and upper separately).

Copy link
Member

Choose a reason for hiding this comment

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

Do you think it is useful to have "No bounds" and "Lower and upper bound, separately"? I think they are just consequence of the others.

Copy link
Member

Choose a reason for hiding this comment

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

I meant for this list to be mutually exclusive so you can have a single vector that identifies the status of the bounds.

last_variable_reference::UInt64
variable_mapping::Dict{MOI.VariableReference, Int}
variable_references::Vector{MOI.VariableReference}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need variable_mapping and variable_references, cant we just use keys(variable_mapping)?

@joaquimg
Copy link
Member

What about people that use CPLEX.jl directly? Should this be a new package since there are so many changes? I was thinking about first implementing a simple MOI and then update the solver low level API. Whats in you mind?

@odow
Copy link
Member Author

odow commented Jul 31, 2017

I just moved everything out of the way for now. The new ccall methods I'm prepending cpx_. I'll probably keep all the old methods as they were.

@odow
Copy link
Member Author

odow commented Aug 7, 2017

Okay this is in fairly robust form. @joehuchette did you have any ideas about callbacks?

@joehuchette
Copy link
Contributor

I don't think we've settled on how callbacks fit into MOI yet. Are they broken on this branch?

@odow
Copy link
Member Author

odow commented Aug 9, 2017

Okay I just reenabled the old code. For now the old MPB compatible CplexSolver has been renamed to CplexMPBSolver. It really makes sense to split these out into separate packages though. A CPLEX.jl that contains the low-level wrapper. A MathOptInterfaceCPLEX.jl with CplexMOISolver(), and MathProgBaseCPLEX.jl with CplexMPBSolver()

@odow odow mentioned this pull request Apr 29, 2018
@odow
Copy link
Member Author

odow commented Sep 3, 2018

Closing in favor of #181

@odow odow closed this Sep 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants