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

Run all tests against bridged SDPA model #2357

Merged
merged 5 commits into from
Dec 23, 2023
Merged

Run all tests against bridged SDPA model #2357

merged 5 commits into from
Dec 23, 2023

Conversation

blegat
Copy link
Member

@blegat blegat commented Dec 13, 2023

Closes #2375

When looking over the failing tests of https://github.com/blegat/SDPLR.jl/, I noticed some were due to MOI bugs. When looking at how to reproduce them, I found out that we could have detected them by running against all the tests. Looking back at the history, it seems I initially just called nametest because it wasn't so easy to run all the tests. When @odow migrated to the new test infrastructure, he used include = ["ConstraintName", "VariableName"], so that's its equivalent to nametest. This did not take the opportunity brought by this new test infrastructure to run it against all the tests. These tests are usually failing for all solvers so fixing them should help many solver wrappers at the same time.

This brings up a few bugs that I suggest we first fix in separate PRs with their own tests:

GeometricSDPA (so it should help SCS, ECOS, COSMO, SeDuMi, SDPT3, CDCS, Clarabel, ...)

StandardSDPA (so it should help CSDP, SDPA, SDPLR, DSDP, ...)

@odow
Copy link
Member

odow commented Dec 14, 2023

test_multiobjective_vector_nonlinear I would have expected all of these to be skipped

@blegat
Copy link
Member Author

blegat commented Dec 14, 2023

They are skipped for the SDPA, CSDP, SDPLR, ... solvers but not for the tests in lazy_bridge_optimizer since we don't exclude it:

function MOI.supports(
::StandardSDPAModel{T},
::MOI.ObjectiveFunction{
<:Union{MOI.VariableIndex,MOI.ScalarQuadraticFunction{T}},
},
) where {T}
return false
end

@odow
Copy link
Member

odow commented Dec 14, 2023

Oh. Let's just exclude it. They're not really meant to be bridged like this just yet.

@odow
Copy link
Member

odow commented Dec 22, 2023

I'm taking a look at some of the remaining issues

@odow
Copy link
Member

odow commented Dec 22, 2023

test_model_ListOfVariablesWithAttributeSet

import MathOptInterface as MOI
include("test/Bridges/sdpa_models.jl")
model =
    MOI.instantiate(StandardSDPAModel{Float64}; with_bridge_type = Float64)
config = MOI.Test.Config(
    exclude = Any[MOI.optimize!, MOI.SolverName, MOI.SolverVersion],
)
MOI.Test.test_model_ListOfVariablesWithAttributeSet(model, config)

test_model_LowerBoundAlreadySet

import MathOptInterface as MOI
include("test/Bridges/sdpa_models.jl")
model =
    MOI.instantiate(StandardSDPAModel{Float64}; with_bridge_type = Float64)
config = MOI.Test.Config(
    exclude = Any[MOI.optimize!, MOI.SolverName, MOI.SolverVersion],
)
MOI.Test.test_model_LowerBoundAlreadySet(model, config)

test_model_UpperBoundAlreadySet

import MathOptInterface as MOI
include("test/Bridges/sdpa_models.jl")
model =
    MOI.instantiate(StandardSDPAModel{Float64}; with_bridge_type = Float64)
config = MOI.Test.Config(
    exclude = Any[MOI.optimize!, MOI.SolverName, MOI.SolverVersion],
)
MOI.Test.test_model_UpperBoundAlreadySet(model, config)

test_model_ScalarFunctionConstantNotZero

import MathOptInterface as MOI
include("test/Bridges/sdpa_models.jl")
model =
    MOI.instantiate(StandardSDPAModel{Float64}; with_bridge_type = Float64)
config = MOI.Test.Config(
    exclude = Any[MOI.optimize!, MOI.SolverName, MOI.SolverVersion],
)
MOI.Test.test_model_ScalarFunctionConstantNotZero(model, config)

@odow
Copy link
Member

odow commented Dec 22, 2023

So it makes sense that the test_model_UpperBoundAlreadySet and test_model_LowerBoundAlreadySet tests fail, because they're getting reformulated into something.

But look at this:

julia> model =
           MOI.instantiate(GeometricSDPAModel{Float64}; with_bridge_type = Float64);

julia> x = MOI.add_variable(model)
MOI.VariableIndex(1)

julia> set2 = MOI.GreaterThan(0.0)
MathOptInterface.GreaterThan{Float64}(0.0)

julia> set1  = MOI.EqualTo(0.0)
MathOptInterface.EqualTo{Float64}(0.0)

julia> ci = MOI.add_constraint(model, x, set1)
MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.EqualTo{Float64}}(1)

julia> print(model)
Feasibility

Subject to:

VariableIndex-in-EqualTo{Float64}
 v[1] == 0.0

julia> ci = MOI.add_constraint(model, x, set1)
MathOptInterface.ConstraintIndex{MathOptInterface.VariableIndex, MathOptInterface.EqualTo{Float64}}(1)

julia> print(model)
Feasibility

Subject to:

VectorAffineFunction{Float64}-in-Zeros
 ┌               ┐
 │-0.0 + 1.0 v[1]│
 └               ┘  Zeros(1)

VariableIndex-in-EqualTo{Float64}
 v[1] == 0.0

# Cannot substitute `MOI.VariableIndex(1)` as it is bridged into `0.0 + 1.0 MOI.VariableIndex(-1)`.
# This seems okay. We can't get a list of variables if they are
# bridged.
"test_model_ListOfVariablesWithAttributeSet",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a maybe. I didn't look too deeply if you know better

@odow
Copy link
Member

odow commented Dec 22, 2023

Copy link
Member

@odow odow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. There are still a couple of stragglers, but I don't now what we should do about them. We're much better than before.

@blegat blegat merged commit ede6d1c into master Dec 23, 2023
59 of 61 checks passed
@blegat blegat deleted the bl/full_test_SDPA branch December 23, 2023 08:00
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.

[Bridges] bug with multiple variable bounds
2 participants