Implementation Questions

[odow]

Some feedback from @tkoolen from gitter. Bolded statements are replies to his questions. People are welcome to edit my replies.

First, some observations about MOI{SCS, ECOS}. These are just my notes from digging into the code. Please correct me if anything is wrong. MOISCS and MOIECOS each define an AbstractSolverInstance, which stores a solver-specific version of an optimization problem. The MOI documentation says that the data should be loaded in the solver’s API, but for SCS and ECOS that’s not really the case; it’s just put in a format that’s easily loadable into the solver’s API. The actual loading (as I would call it) happens in MOI.optimize!. So really the {SCS, ECOS}SolverInstances are very limited in their capabilities; you can basically only copy an AbstractInstance into them and then solve. They currently don’t support problem modification (the MOIU.loadconstraint! function is only used during the copying process).

Now for OSQP. OSQP gets a lot of its strength from its support for problem data modification. To support that, the modify{constraint!, objective!} methods for OSQPSolverInstance would ideally directly call the OSQP.update_* methods, directly modifying the problem data loaded into OSQP itself, as opposed to maintaining a list of changes to be made upon the next call to optimize!. That would probably entail loading the data into OSQP in copy!, similar to what @mlubin proposed for SCS.
@mlubin said: "I'm also in favor of loading the data into OSQP on copy!, that's the intention of the API"

Additionally, methods for MOI.canmodify{constraint, objective} are needed. There are two problems here:

1. for constraints, I don’t think I can currently query that information from OSQP. OSQP only allows modification of constraints if the sparsity structure is maintained, and the only way to know the sparsity structure given the current OSQP interface is to try modification and see if you get an error code.
2. for the objective function, I believe MOI.canmodifyobjective cannot capture the constraints that OSQP places on objective modification (again, the sparsity structure must be maintained).
   It's okay to return true from canmodifyXXX and then an error on modifyXX! if the data doesn't meet the necessary conditions.

Here's what I have at the moment: tkoolen/OSQP.jl@tk/mathprogbase...tk/mathoptinterface. Untested.

• I can't use the MOIT tests because I only advertise support for Interval constraint sets (which I think is the right thing to do). I thought that the proper way to add support for LessThan etc. would be to have a CombineIntoIntervalBridge in MOIU, similar to SplitIntervalBridge.
  If it isn't too much effort, you should probably implement LessThan, GreaterThan, and EqualTo since these are pretty fundamental. There was some initial debate about whether to even have LessThan etc or just an Interval set that could be (-inf, u] and the distinct sets won out.

Here are some more general comments that I wrote down while coding (@mlubin requested this):

• Document the features of MOIU properly (took some time to even figure out what the package was meant for).

JuliaOpt/MathOptInterfaceBridges.jl#89

• MOI.empty!, MOI.isempty are not properly documented
  Fixed by Add doc for empty! and isempty #215

• MOI documentation: what if get is erroneously called when canget returns false? Do implementors need to worry about that?
  It is the user's responsibility to check that canget returns true. Ref Clarity get/set! when canget/set returns false #210

• MOI.get with vector variable indices: is that really needed? I saw an overload in SCS that just broadcasted to the scalar function, https://github.com/JuliaOpt/MathOptInterfaceSCS.jl/blob/a0f8467245d8b0a1814cf7248b174c1462990ae5/src/attributes.jl#L51. Can’t that be the default fallback?
  Some solvers may support efficiently querying a list (for example) variable values in a single API call, compared to say, N calls. Ref Fallbacks for vector attributes #212

• Why is there no MOI.supportsobjective?
  Since objectives are attributes, you should use MOI.canset(instance, MOI.ObjectiveFunction{T}())

• In ScalarAffineFunction, why not use a Vector{Pair{VariableIndex, T}}, so as to ensure that you don’t have more coeffs than vars? Same thing for some of the other functions.
  This seems like a reasonable proposal. Ref Use Pair{VariableIndex, T} instead of separate variable and coefficient lists #211 for further discussion.

• I’m still not sure whether I should advertise support for vector-valued constraints in OSQP. Perhaps document that better.
  In general, the trend has been to offer as many different constraint types as possible. If OSQP supports adding a big vector block at once, then it makes sense to support that. If it only supports adding one constraint at a time, less so. Ref 51c8f72 and Discuss implementing all constraint types #213

[odow]

@tkoolen - can you elaborate on "MOI.empty!, MOI.isempty are not properly documented" What wasn't clear?

[tkoolen]

Re: MOI.empty!, MOI.isempty: currently the only mention of them is in a side note of AbstractSolverParameter:

The difference between AbstractSolverParameter and AbstractInstanceAttribute lies in the behavior of isempty, empty! and copy!.

which doesn't tell you how to properly implement isempty and empty!, just that there's some opaque difference in AbstractSolverParameter and AbstractInstanceAttribute behavior between them.

I would have expected their docstrings to be shown next to optimize! and free! (if they're only for AbstractSolverInstances, which is also not clear) or next to copy!. It's also just so that you don't forget about them as you're working your way through the list.

[tkoolen]

It's okay to return true from canmodifyXXX and then an error on modifyXX! if the data doesn't meet the necessary conditions.

That should probably be documented.

[tkoolen]

Re: get without canget: how about a checkedget convenience function that checks that canget returns true before calling get? Right now it's pretty easy to get lazy about the canget checks everywhere. Not married to the name checkedget by the way.

[mlubin]

@tkoolen see my comment in #210. I disagree that it should be required to call canget before get.

[mlubin]

Wrong issue closed

[blegat]

Yes, sorry :-P