Skip to content
This repository has been archived by the owner on Jun 14, 2020. It is now read-only.

RFC: refactor the solver-facing API #7

Merged
merged 10 commits into from
May 8, 2018
Merged

RFC: refactor the solver-facing API #7

merged 10 commits into from
May 8, 2018

Conversation

odow
Copy link
Member

@odow odow commented May 1, 2018

This PR refactors the solver-facing API of LQOI.

I have chosen long, explict names in snake_case.

Remaining decisions

  • the name of lqs_char

  • get_rhs or `get_right_hand_side``

  • whether to tag this as a separate package, or incorporate into MOI.
    Pros of incorporating: it is easier to incorporate changes (i.e. the upcoming redefinition of ScalarAffineFunction etc.
    Cons of incorporating: testing.

This was referenced May 1, 2018
# function lqs_setlogfile!(m::LinQuadOptimizer, path) end
#
# # TODO(@joaquimg): what is this?
# function lqs_getprobtype(m::LinQuadOptimizer) end
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 remove these 3

@odow
Copy link
Member Author

odow commented May 7, 2018

Okay @joaquimg, this is in a nicer state. Any objections to the renaming?

@odow odow changed the title WIP: refactor the solver-facing API RFC: refactor the solver-facing API May 7, 2018
@joaquimg
Copy link
Member

joaquimg commented May 7, 2018

I will take a look today

"""
function lqs_getcoef(m::LinQuadOptimizer, row, col) end
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 got rid of this because I couldn't find it used anywhere

"""
function lqs_chgrngval!(m::LinQuadOptimizer, rows, vals) end# later
function change_range_value! end
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'm still open to an add_ranged_constraint! method instead of this.

"""
lqs_getobj(m)::Vector{Float64}
get_linear_objective!(m, x::Vector{Float64})
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 made this in-place.


"""
lqs_getdj!(m, x::Vector{Float64})
get_reducedcosts!(m, x::Vector{Float64})
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this shoud be get_variable_dual_solution!


"""
lqs_dualfarkas!(m, x::Vector{Float64})
get_farkasdual!(m, x::Vector{Float64})
Copy link
Member Author

Choose a reason for hiding this comment

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

For consistency get_farkas_dual!


"""
lqs_getray!(m, x::Vector{Float64})
get_unboundedray!(m, x::Vector{Float64})
Copy link
Member Author

Choose a reason for hiding this comment

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

For consistency get_unbounded_ray!


"""
lqs_terminationstatus(m)
get_terminationstatus(m)
Copy link
Member Author

Choose a reason for hiding this comment

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

get_termination_status


"""
lqs_primalstatus(m)
get_primalstatus(m)
Copy link
Member Author

Choose a reason for hiding this comment

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

get_primal_status


"""
lqs_dualstatus(m)
get_dualstatus(m)
Copy link
Member Author

Choose a reason for hiding this comment

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

get_dual_status

@joaquimg
Copy link
Member

joaquimg commented May 7, 2018 via email

@odow
Copy link
Member Author

odow commented May 7, 2018

I will check getcoef usage in xpress.

It doesn't matter if it is used in Xpress. It only matters if it is used in LQOI.

@joaquimg
Copy link
Member

joaquimg commented May 7, 2018

Yep, agree. But there is one function in LQOI that is extremely inefficient the one to query constraints and coefficients. This function might be a way to go that was forgotten in time and space.
Sure you can delete it and we add it back again if necessary.

@odow
Copy link
Member Author

odow commented May 7, 2018

Sure you can delete it and we add it back again if necessary.

This is one argument for keepling LQOI and MOI separate. MOI won't be changing but we might be changing LQOI...

@joaquimg
Copy link
Member

joaquimg commented May 7, 2018

Yes, there is performance tweaks to happen in LQOI, I think we should tag it separately (so its usable) and then we merge it into MOI (when stable)

@joaquimg
Copy link
Member

joaquimg commented May 7, 2018

I am in favour of rhs, it is a bit different but I think its a well established jargon

@odow
Copy link
Member Author

odow commented May 8, 2018

Okay, I've finished the renaming. Gurobi and GLPK both pass.

I propose merging this, registering LQOI in METADATA, then merging and tagging releases of both Gurobi and GLPK.

@mlubin
Copy link
Member

mlubin commented May 8, 2018

@odow do you mean tagging Gurobi and GLPK with the LQOI dependency?

@odow
Copy link
Member Author

odow commented May 8, 2018

do you mean tagging Gurobi and GLPK with the LQOI dependency?

Yes.

I'm hestitant to merge LQOI into MOI in the short term as it is hard to test, and may yet change. Maybe we keep as separate until JuMP 0.19 comes out, then it should be stable enough to merge into MOI.

@joaquimg
Copy link
Member

joaquimg commented May 8, 2018

I agree with

I propose merging this, registering LQOI in METADATA, then merging and tagging releases of both Gurobi and GLPK.

@mlubin
Copy link
Member

mlubin commented May 8, 2018

Gurobi, Cplex, Xpress, and GLPK are pretty important parts of the JuMP ecosystem. It's not great that they're on shaky ground by depending on LQOI. But if it makes things easier for now, go ahead. Bootstrapping all the solver interfaces is hard, after all.

@joaquimg
Copy link
Member

joaquimg commented May 8, 2018

I think doing this for now would bring testers for both MOI and JuMP, meanwhile we think on how to incorporate LQOI into MOI and test it.
A MockLinQuadOptimizer looks like a way to go.

@odow
Copy link
Member Author

odow commented May 8, 2018

The alternative is to merge LQOI into MOI and just bump version bounds more often. Most of the improvements won't change the solver-facing API so they can just be patch releases...

@mlubin
Copy link
Member

mlubin commented May 8, 2018

It will be a bit annoying if we have to keep bumping all the MOI upper bounds if there are LQOI breaking changes. If you expect to do this more than twice per MOI bump then it's probably better to leave it as a separate package for now.

@joaquimg
Copy link
Member

joaquimg commented May 8, 2018

I think they might be breaking

@odow
Copy link
Member Author

odow commented May 8, 2018

Let's punt on the decision for a bit. I'm going to merge this now, and then have a look at what needs to be implemented ASAP to avoid a lot of breaking changes in the future.

@odow odow merged commit ce60996 into master May 8, 2018
@odow odow deleted the renaming branch May 8, 2018 04:00
@blegat
Copy link
Member

blegat commented May 8, 2018

I agree with @joaquimg and @odow , we need to at least wait until LQOI (same for SDOI) is able to test it by itself (e.g. without relying on an external solver). This might happen after JuMP v0.19, it would be nice if we don't delay JuMP v0.19 too much :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants