Skip to content

Conversation

blegat
Copy link
Member

@blegat blegat commented Jan 25, 2018

This is needed to solve jump-dev/JuMP.jl#1092

@mlubin
Copy link
Member

mlubin commented Jan 25, 2018

I don't know if this really fits in the attribute or parameter framework. It's get-only and we'll never want to copy it. Maybe a separate solvername method is more appropriate?

Also it's unfortunate but I think we need to do another big renaming of the MOI abstractions. "Solver" isn't currently a clear MOI term, but we're converging to it in MOIU and JuMP, e.g.,
https://github.com/JuliaOpt/MathOptInterfaceUtilities.jl/blob/d9d1c4f20b1667e440b2a54831fefe8a117e433a/src/instancemanager.jl#L5

@blegat
Copy link
Member Author

blegat commented Jan 25, 2018

The advantage of it being a solver parameter is that the InstanceManager can do

function MOI.get(m::InstanceManager, param::MOI.AbstractSolverParameter)
    MOI.get(m.solver, param)
end

so it does not need to do a particular case for solvername.
This shows that AbstractSolverParameter can also have a meaning for get-only parameter.

@mlubin
Copy link
Member

mlubin commented Jan 25, 2018

This doesn't really fit with my understanding of a "parameter" though, it's not something that affects the solution procedure. "Attribute" vs "parameter" might have been the wrong naming choice. Using "property" as the union of the two doesn't make much sense either.
https://github.com/JuliaOpt/MathOptInterface.jl/blob/831b87ba2dcfd9b8cf50ff73e32c856cb3488f82/src/attributes.jl#L37

What about "InstanceAttribute" and "SolverAttribute"? This again pushes on us to rename AbstractSolverInstance to AbstractSolver and AbstractStandaloneInstance to AbstractInstance.

@blegat
Copy link
Member Author

blegat commented Jan 25, 2018

That seems better, but I like AbstractInstance for the supertype of both.
What about AbstractSolver <: AbstractInstance and AbstractStandaloneInstance <: AbstractInstance ? Or do you have a better idea for the super type ?

@mlubin
Copy link
Member

mlubin commented Jan 25, 2018

I have trouble calling a solver a kind of instance, but on the other hand it doesn't bother me to say that a solver implements the instance interface. This is awkward to deal with in Julia. Usually you'd duck-type it and not even have a super type, right?

@blegat
Copy link
Member Author

blegat commented Jan 26, 2018

We could duck-type but it seems unfortunate to loose the abstract type for types implementing the MOI interface. Most of the time, we need to use AbstractInstance. The distinction between AbstractStandaloneInstance and AbstractSolverInstance and the separation is sometimes blurry (e.g. instance storing results) so the only thing that really separates them is optimize!. Even AbstractSolverAttribute could also be supported by AbstractStandaloneInstance, see JuliaOpt/MathOptInterfaceBridges.jl#61.
I support renaming SolverParameter to SolverAttribute but I am not convinced that we should get rid of AbstractInstance.

@mlubin
Copy link
Member

mlubin commented Jan 26, 2018

What about AbstractSolver replaces AbstractSolverInstance and AbstractInstance replaces AbstractStandaloneInstance, with InstanceLike = Union{AbstractSolver,AbstractInstance}. Solvers and instances are fundamentally different things that just overlap in their API in some places for convenience. It doesn't make sense to call a copy-only solver an "instance," but "instance-like" is less objectionable. I'm open to bikeshedding on InstanceLike.

@blegat
Copy link
Member Author

blegat commented Jan 26, 2018

That would work, I like InstanceLike ! We are also using the Like suffix in MultivariatePolynomials.

@mlubin mlubin mentioned this pull request Feb 4, 2018
@blegat blegat mentioned this pull request Feb 11, 2018
@blegat
Copy link
Member Author

blegat commented Mar 5, 2018

I have update the PR, do you prefer SolverName or OptimizerName ?

@codecov-io
Copy link

codecov-io commented Mar 5, 2018

Codecov Report

Merging #201 into master will increase coverage by 0.38%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
+ Coverage   98.07%   98.46%   +0.38%     
==========================================
  Files          23       23              
  Lines        3904     4961    +1057     
==========================================
+ Hits         3829     4885    +1056     
- Misses         75       76       +1
Impacted Files Coverage Δ
src/attributes.jl 93.33% <ø> (ø) ⬆️
src/Test/contquadratic.jl 100% <0%> (ø) ⬆️
src/Test/contlinear.jl 100% <0%> (ø) ⬆️
src/Test/contconic.jl 100% <0%> (ø) ⬆️
src/Test/modellike.jl 100% <0%> (ø) ⬆️
src/Test/intconic.jl 100% <0%> (ø) ⬆️
src/Test/intlinear.jl 97.61% <0%> (+0.19%) ⬆️
src/Utilities/model.jl 99.59% <0%> (+0.56%) ⬆️
src/Utilities/cachingoptimizer.jl 90.47% <0%> (+1.47%) ⬆️
src/Utilities/mockoptimizer.jl 96.62% <0%> (+1.75%) ⬆️

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 a8f50cb...e2f702b. Read the comment docs.

@mlubin
Copy link
Member

mlubin commented Mar 9, 2018

I'm not bothered by SolverName but some might see it as inconsistent.

@mlubin
Copy link
Member

mlubin commented Mar 9, 2018

Then again I'm also responsible for defVar.

@blegat
Copy link
Member Author

blegat commented Mar 10, 2018

What do you mean by defVar ?

@odow
Copy link
Member

odow commented Mar 10, 2018

@blegat
Copy link
Member Author

blegat commented Mar 10, 2018

Ah ok got it :D @odow @joaquimg ok with SolverName ?

@joaquimg
Copy link
Member

Ok!

@odow
Copy link
Member

odow commented Mar 10, 2018

Not bothered.

@blegat blegat merged commit 4f6097c into master Mar 11, 2018
@mlubin mlubin deleted the bl/solvername branch March 11, 2018 13:59
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.

5 participants