[style] naming wrapped c functions

[IssamT]

Cplex has cpxchgsense

The corresponding julia function should be chg_sense or chgsense ?

[mlubin]

The intermediate chg_sense seems arbitrary because we wouldn't allow the abbreviation chg. You could pick the style-compliant naming change_sense or maybe for clarity c_api_chgsense?

[IssamT]

Sorry, I have just noticed that I opened this issue in JuMP.jl instead of Cplex.jl
You are right about chg_sense not being fully compliant. I prefer change_sense then.

[odow]

I prefer c_api_chgsense since we should make these functions as tight to the C API as possible.

There are lots of functions in Gurobi that have levels and levels of redicrection, for example
this add_constr! calls this add_constr! which calls this add_constr! which actually is the wrapper of the C function addconstr.

If we made a style guide that the low-level function is to be called c_api_addconstr, then we can distinguish between helper functions (which I think we should try to avoid when wrapping the C API) and the actual API call.

[IssamT]

I agree with @odow about not having too many level of indirections. But this is an argument against naming functions c_api_xxx() which constrain the freedom of the developper: If MOI uses a function xxx() which is less specific than the one in the C API, we will need to have both the specific function wrapper c_api_xxx() and the less specific "helper" function xxx() that will be called by MOI/LQOI. This probably won't happen often for Cplex since LQOI was designed to match Cplex/Gurobi's interface, but the style guide should be the same for all wrappers.

PS: I assume that if we name a function c_api_xxx(), it should have the same functionality as the C function. I also assume that we don't want to have a mix of naming, with some wrapping functions starting with c_api and others which don't.

[odow]

If MOI uses a function xxx() which is less specific than the one in the C API, we will need to have both the specific function wrapper c_api_xxx() and the less specific "helper" function xxx() that will be called by MOI/LQOI.

MOI should not use the less specific function. It should use the c_api_xxx function. If it needs a helper function, that should be in the MOIWrapper.jl file, documented, and tested.

Currently we are in the situation where there are lots of helper functions floating around that are not documented or well tested. It is also not obvious just be looking at the function name or the arguments which C function it ultimately ends up calling.

We should be pushing people to use MOI / JuMP in direct mode rather than the low-level solver interfaces.

I also assume that we don't want to have a mix of naming, with some wrapping functions starting with c_api and others which don't.

Yes. I would like to refactor the whole Gurobi.jl package to remove the unnecessary exports, simplify all the calls, and change all the C functions to c_api_xxx. I'm not sure if there is a good way of deprecating exports though...

[IssamT]

I think I didn't explain well what I meant by the less specific wrapping function. Consider this simple example:

#Solution 1

function get_var_lb(model::Model, col::Cint)
    lb = Vector{Cdouble}(1)
    @cpx_ccall(getlb, Cint, ( Ptr{Void}, Ptr{Void}, Ptr{Cdouble}, Cint, Cint),
                      model.env.ptr, model.lp, lb, col-Cint(1), col-Cint(1))
    return lb[1]
end

function LQOI.get_variable_lowerbound(model::Optimizer, column::Int)
    get_var_lb(model.inner, Cint(column))
end

get_var_lb does not offer all the functionality provided by cpxgetlb because it only gives the lower bound for one variable and that's enough to implement LQOI interface.

The other solution forces the wrapping function to offer the full functionality as the C function and restrict it later either by using a helper function (solution 2A) or by increasing the complexity in all the calling functions (solution 2B).

#Solution 2A

function c_api_getlb(model::Model, col_start::Cint, col_end::Cint)
    lb = Vector{Cdouble}(1)
    @cpx_ccall(getlb, Cint, ( Ptr{Void}, Ptr{Void}, Ptr{Cdouble}, Cint, Cint),
                      model.env.ptr, model.lp, lb, col_start - Cint(1), col_end-Cint(1))
    return lb[1]
end

function get_var_lb(model::Model, col::Cint)
     return get_var_lb(model.inner, Cint(column), Cint(column))
end

function LQOI.get_variable_lowerbound(model::Optimizer, column::Int)
    return get_var_lb(model.inner, Cint(column))
end

#Solution 2B

function c_api_getlb(model::Model, col_start::Cint, col_end::Cint)
    lb = Vector{Cdouble}(1)
    @cpx_ccall(getlb, Cint, ( Ptr{Void}, Ptr{Void}, Ptr{Cdouble}, Cint, Cint),
                      model.env.ptr, model.lp, lb, col_start - Cint(1), col_end-Cint(1))
    return lb[1]
end

function LQOI.get_variable_lowerbound(model::Optimizer, column::Int)
    get_var_lb(model.inner, Cint(column), Cint(column))
end

There is a 3rd solution that I'd exclude which is to keep solution 1 but only rename get_var_lb into c_api_getlb

So which solution exactly you prefer?

I prefer solution 1 because it leaves the flexibility for the developer of the wrapping function to use or not the same functionality as the C API.

[odow]

I would prefer the following for a few reasons:

• the c_api_xxx are more or less self documenting since the name is the CPLEX function
• the arguments to c_api_getlb are very close to the actual C call
• all the features of the C function are exposed
• there are no helper functions

"""
    c_api_getlb(model::Model, dest, column_start::Int, column_end::Int)

Get the lower bounds of a list of contiguous variables in the (one-indexed)
columns `column_start` to `column_end`, and store the result in `dest`. 

If `column_start != column_end`, `dest` must be a `Vector{Float64}` with length
`column_end - column_start + 1`. If `column_start == column_end`, then it is
more efficient to pass `dest` as  `Ref{Float64}()`, since this avoids
constructing a one-element array.
"""
function c_api_getlb(model::Model, dest, column_start::Int, column_end::Int)
    @assert length(dest) == column_end - column_start + 1
    stat = @cpx_ccall(getlb, Cint, 
                      (Ptr{Void}, Ptr{Void}, Ptr{Cdouble}, Cint, Cint),
                      model.env.ptr, model.lp, dest, column_start-1, column_end-1)
    if stat != 0
        throw(CplexError(model.env, stat))
    end
end

function LQOI.get_variable_lower_bound(model::Optimizer, column::Int)
    lower_bound = Ref{Float64}()
    c_api_getlb(model.inner, lower_bound, column, column)
    return lower_bound[]
end

[IssamT]

The only thing I am saying is that :

Those two are really advantages of solution2

• the c_api_xxx are more or less self documenting since the name is the CPLEX function
• the arguments to c_api_getlb are very close to the actual C call

But those 2 are not.

• all the features of the C function are exposed
• there are no helper functions

You could choose to have the exact same code you have written, with the name get_lower_bounds. Deciding to call it c_api_getlb means you have no more choice for the implementation. the function must fit the c api and there must be an increasing complexity in the LQOI interface if it doesn't match it.

However, I agree that the first two advantages are probably strong enough to go for solution2, Although I d keep the flexibility of using solution2A (with helper functions) when the complexity in the calling functions is substantial.

[odow]

Deciding to call it c_api_getlb means you have no more choice for the implementation. the function must fit the c api and there must be an increasing complexity in the LQOI interface if it doesn't match it.

Exactly :) This is what I want. Let's move the complexity away from the C API wrapper, and into the (hopefully) well tested LQOI interface functions.

To avoid going back and forth with the same points, perhaps we should let others weigh in to see what they think.

[IssamT]

If there are no objections on this by tomorrow, I'll go with what we agreed on :)

[odow]

I'm not sure we have agreed on this...?

[IssamT]

I thought yes. we go with c_api_xxx functions. no?