Fix bug in setting a negative lower bound of a SOC constrained variable with add_constraints

[henriquebecker91]

The first commit adds a test uncovering a bug in SOC constraints. The bug was that defining a negative lower bound on the t variable of a SOC constraint by means of add_constraints (batch version) silently corrupted the related VariableInfo object, but using add_constraint (single-variable version) worked.

The second commit solves the bug by reworking many methods to work correctly with batch/vector (that is where the bug appeared) and changing single-element methods to wrap arguments and call the batch/vector variant. This way there will exist no difference in behavior between single-element and batch methods.

I know it is rude to ping specific people to review (specially in these times of Covid19), but if @odow could review it quickly, I can send a PR tomorrow for the caching of lower and upper bounds that we discussed last week. The bound cache PR was almost ready when I stumbled in this bug, and had to fix it first, before being able to get the caching to work. If the solution is rejected (or needs major changes) then my code for caching bounds would need to be rewritten and, consequently, I prefer to wait the review of this PR before sending the next PR.

[odow]

This is quite complicated, and I'm not suggesting that you need to do this, but a better way might be to use jump-dev/MathOptInterface.jl#1046.

Alternatively, we could implement the adding a 1.0t >= 0.0 constraint, instead of mucking with the bounds.

Thoughts?

I should also note, that we went away from wrapping everything in length-1 arrays, because people were using Gurobi.jl in a soft-realtime setting, and didn't want the extra allocations.

[henriquebecker91]

Sincerely, I put a lot of effort on this, and I do not even use SOC constraints (I have learned about them doing this), so I do not think I have the hearth necessary to change the core of the solution, what do you think of a compromise? I get the dedicated single-element methods back without extra allocations, keep the new batch methods, and put comments on both to remember that future changes in one of them need to be replicated at the other. The vector/batch version of add_constraints already existed and was written to be optimized (i.e., do just one or two calls to the batch method of the Gurobi C API), it was just incorrectly written in this specific case, so I imagine there was demand for batch methods too. This way both user cases "pay allocations but do a single call to the API" and "do not pay allocations for single variables (or broadcasted/repeated calls)" are fulfilled. Everyone is happy (ok, I am sure it is not everyone, but this is the closest I can promise).

I do not mind if someone changes how exactly SOC works in the future, but for now I would like this to be working to be able to push the PR I am truly interested into.

[odow]

So, if I understand the issue, the problem is that _set_bounds ignores SOC variables. Is this a simpler fix then (I haven't tested):

function _set_bounds(
    model::Optimizer,
    indices::Vector{MOI.ConstraintIndex{MOI.SingleVariable, S}},
    sets::Vector{S}
) where {S}
    lower_columns, lower_values = Int[], Float64[]
    upper_columns, upper_values = Int[], Float64[]
    for (c, s) in zip(indices, sets)
        lower, upper = _bounds(s)
        info = _info(model, c)
        if lower !== nothing
            if info.num_soc_constraints == 0
                push!(lower_columns, info.column)
                push!(lower_values, lower)
            else
                _set_variable_lower_bound(model, info, lower)
            end
        end
        if upper !== nothing
            push!(upper_columns, info.column)
            push!(upper_values, upper)
        end
    end
    if length(lower_columns) > 0
        set_dblattrlist!(model.inner, "LB", lower_columns, lower_values)
    end
    if length(upper_columns) > 0
        set_dblattrlist!(model.inner, "UB", upper_columns, upper_values)
    end
    _require_update(model)
end

[henriquebecker91]

Yes, this also solves the bug (I tested with the commit that introduces the test). At cost of MOI.add_constraints not being actually optimized if there are SOC constraints (and MOI.set not being optimized for the vector case). I think I can make the bound cache patch work with this solution easily. You want me to revert the changes of the second commit and use this solution instead?

[odow]

You want me to revert the changes of the second commit and use this solution instead?

Yes. Test from the first commit. Then this instead of the second. Then you can make the bound caching PR :)

[henriquebecker91]

Done. Should I wait it to be merged into master to make the PR from the master commit? I am kinda noobish in terms of git workflow.

[odow]

And you post a screenshot of the tests passing please? Probably easiest to wait for this to be merged to master, which I'll do if you confirm tests pass.

[henriquebecker91]