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

API idea #6

Closed
daviddelaat opened this issue Sep 23, 2013 · 19 comments
Closed

API idea #6

daviddelaat opened this issue Sep 23, 2013 · 19 comments

Comments

@daviddelaat
Copy link

This is just an API idea I was having: Instead of having functions like linprog, mixintprog, setmipsolver, etc, there could also be one method solve(problem, solver) with immutable problem and solver types.

Example:
Instead of writing

setlpsolver(:GLPK)
solution = linprog(c, A, sense, b, lb, ub; options...)

one would write

solution = solve(LP(c, A, sense, b, lb, ub), GLPK(solveroptions))

MathProgBase.jl would then provide the immutable types LP, MIP, LinprogSolution, etc, and GLPK.jl would depend on MathProgBase.jl and provide the immutable type GLPK as well as the functions solve(problem::LP, solver::GLPK) and solve(problem::MIP, solver::GLPK).

@mlubin
Copy link
Member

mlubin commented Sep 23, 2013

I like this idea, and I agree that the API could use some revision. This actually alleviates a bit the need to have solver-independent parameters, because the user could store multiple solver objects, e.g.

glpksolver = GLPK(glpkoptions)
clpsolver = CLP(clpoptions)

and change solvers by passing in different objects.

One issue is the name clash. I don't think it's a great idea for the GLPK module to export a GLPK type. Also the GLPK MathProgBase interface is currently in a different package. We could use symbols instead and keep the "registry" of solvers that we have now, so that solver objects are created with LPSolver(:GLPK,solveroptions), for example.

The second issue, which is a bit more important, is that users shouldn't need to know about different solvers, there should be a reasonable default. At the same time, making the solver explicit in the function call could encourage people to experiment with different solvers, which is always a good thing. I could be convinced either way about this, but I'm in favor of making the MathProgBase code a bit more complex by storing a default solver so that users can make simple calls to solve(LP(c, A, sense, b, lb, ub)). In this case, we could remove the setlpsolver and setmipsolver functions and require using the solve(problem,solver) syntax if you want to change the default solver.

@carlobaldassi @lindahua, any feedback?

@lindahua
Copy link
Member

I generally agree with your comments. I feel it is the right API to have solver types while proving a default solver.

@mlubin
Copy link
Member

mlubin commented Sep 24, 2013

Also until JuliaLang/julia#2327 is resolved, I think we should shy away from using a name like solve, which could be exported from various modules. What I can do now is create these solver objects to clean up that side of the issue.

mlubin added a commit that referenced this issue Oct 1, 2013
@mlubin
Copy link
Member

mlubin commented Oct 1, 2013

@daviddelaat, I've pushed a change that partially implements your proposal. Let me know what you think about it.

@daviddelaat
Copy link
Author

@mlubin: Thanks! That looks very nice, exactly what I mean.

I posted in JuliaLang/julia#4345 about the solve issue.

@daviddelaat
Copy link
Author

BTW, just for reference, here is a very incomplete solve based interface in SemidefiniteProgramming.jl: https://github.com/daviddelaat/SemidefiniteProgramming.jl/blob/master/src/solvers.jl
(there is a warning now since it still uses the solve method there)

@IainNZ
Copy link
Member

IainNZ commented Oct 1, 2013

@daviddelaat just so you know, we are still interested in incorporating SDP into JuMP - we're just trying to get it right first time in a non-hacky way that meshes nicely with LP/MIP/NLP/etc

@mlubin
Copy link
Member

mlubin commented Oct 17, 2013

@lindahua successfully motivated me to clean up this mess :). On the newapi branch I've reworked the interface as follows:

  • Renamed LinprogSolver to MathProgSolver
  • We're not passing around modules any more. Solvers should implement an immutable subtype of SolverNameAndOptions (defined in MathProgSolverInterface.jl), and implement a method of model that takes an instance of SolverNameAndOptions and returns an instance of MathProgSolver. For example, you can create a Cbc model with model(CbcSolver(LogLevel=1)). This properly fixes The model API need to be changed #8.
  • The same SolverNameAndOptions objects can be passed to linprog and mixintprog to specify the solver and any options. We're now properly dispatching on the type of the solver object, so type inference will work, and it's possible for solvers to override the implementation of linprog if desired.
  • Somewhat cleaner code to set the default solver. This is now all in the defaultsolvers.jl file.

I've implemented this in Clp.jl and Cbc.jl. To get a sense of what it looks like, see: https://github.com/mlubin/Clp.jl/blob/newapi/src/ClpSolverInterface.jl#L43.

Let me know what you guys think (@daviddelaat, @carlobaldassi). Hopefully we can settle down on a stable API and close this issue.

@lindahua
Copy link
Member

@mlubin I am happy with your new design. Except that I am not completely comfortable with the name SolverNameAndOptions. For example, CbcSolver is a sub-type of SolverNameAndOptions, while the result of model(CbcSolver( ... )) is an instance of MathProgSolver.

What about the following naming changes:

SolverNameAndOptions --> AbstractMathProgSolver
MathProgSolver --> AbstractMathProgModel

In this way, the function model takes in an instance of AbstractMathProgSolver (e.g. CbcSolver(), GurobiSolver()) and returns an instance of AbstractMathProgModel (people would expect what the model function returns is a model of some kind).

@mlubin
Copy link
Member

mlubin commented Oct 19, 2013

That's a good logical distinction with much cleaner names than I came up with. I'll make these changes, and unless there are any other objections, I'll merge in a day or so.

@mlubin
Copy link
Member

mlubin commented Oct 20, 2013

The newapi branch has been merged. Thanks to everyone for the feedback, this is a real cleanup of the API.

@carlobaldassi
Copy link
Member

GLPK is now complying with the new API.

Miles, when you bump MathProgBase in METADATA, can you please also bump GLPKMathProgInterface at the same time? (And probably Gurobi, if it's ready.) In this way people updating will continue to see things working, more or less.

@mlubin
Copy link
Member

mlubin commented Oct 21, 2013

Thanks for quickly implementing this, Carlo. I'll be sure to bump everything together.

@mlubin
Copy link
Member

mlubin commented Oct 23, 2013

This has all be pushed to metadata now.

@carlobaldassi
Copy link
Member

Thanks a lot Miles. What do you guys think about finally moving away from v0.0.0? It seems to me that the API and docs shape is now good enough for this. Also, I'm using this stuff on a regular basis at work, and I suspect others are in the same situation; having some degree of stability would help a lot I think, especially collaboration with coworkers.
If you agree, I just propose to tag v0.1.0 of MathProgBase and move on.

@mlubin
Copy link
Member

mlubin commented Oct 25, 2013

You can go ahead and tag v0.1.0, I think we're ready.

@carlobaldassi
Copy link
Member

v0.1.0 tagged!

@lindahua
Copy link
Member

congrats!

@mlubin
Copy link
Member

mlubin commented Oct 27, 2013

Nice!

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

No branches or pull requests

5 participants