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

Update MOI.Test to support any coefficient that is a subtype of Real #841

Closed
chriscoey opened this issue Aug 16, 2019 · 17 comments
Closed
Labels
good first issue Status: help wanted This issue needs could use help from the community Submodule: Tests About the Tests submodule
Milestone

Comments

@chriscoey
Copy link
Contributor

chriscoey commented Aug 16, 2019

Looking for an easy way to help contribute to MOI?

Help us by updating the tests so that we can run them with a generic coefficient type, instead of just Float64.

Problem

I am trying to test a solver that works on a Model{T} for any T <: Real, but I can only run the vast majority of tests when T = Float64.

Solution

Things have changed quite a lot since the discussion below. Here's the latest (August 2021):

Step 1: pick a file from https://github.com/jump-dev/MathOptInterface.jl/tree/master/src/Test

Step 2: pick a test which contains explicit Float64s. Here's one

function test_variable_solve_with_upperbound(
model::MOI.ModelLike,
config::Config,
)
x = MOI.add_variable(model)
c1 = MOI.add_constraint(model, MOI.SingleVariable(x), MOI.LessThan(1.0))
@test x.value == c1.value
c2 = MOI.add_constraint(model, MOI.SingleVariable(x), MOI.GreaterThan(0.0))
@test x.value == c2.value
f = MOI.ScalarAffineFunction([MOI.ScalarAffineTerm(2.0, x)], 0.0)
MOI.set(model, MOI.ObjectiveFunction{typeof(f)}(), f)
MOI.set(model, MOI.ObjectiveSense(), MOI.MAX_SENSE)
_test_model_solution(
model,
config;
objective_value = 2.0,
variable_primal = [(x, 1.0)],
constraint_primal = [(c1, 1.0), (c2, 1.0)],
constraint_dual = [(c1, -2.0), (c2, 0.0)],
)
return
end

Step 3: rewrite it to use a type parameter T instead

function test_variable_solve_with_upperbound(
    model::MOI.ModelLike,
    config::Config{T},
) where {T}
    x = MOI.add_variable(model)
    c1 = MOI.add_constraint(model, MOI.SingleVariable(x), MOI.LessThan(one(T)))
    @test x.value == c1.value
    c2 = MOI.add_constraint(model, MOI.SingleVariable(x), MOI.GreaterThan(zero(T)))
    @test x.value == c2.value
    f = MOI.ScalarAffineFunction([MOI.ScalarAffineTerm(T(2), x)], zero(T))
    MOI.set(model, MOI.ObjectiveFunction{typeof(f)}(), f)
    MOI.set(model, MOI.ObjectiveSense(), MOI.MAX_SENSE)
    _test_model_solution(
        model,
        config;
        objective_value = T(2),
        variable_primal = [(x, one(T))],
        constraint_primal = [(c1, one(T)), (c2, one(T))],
        constraint_dual = [(c1, -T(2)), (c2, zero(T))],
    )
    return
end
@ericphanson
Copy link
Contributor

@JiazhengZhu and I would be interested in this too! Could someone point us to how to go about generalizing the tests?

@odow
Copy link
Member

odow commented Aug 28, 2019

Sure! The basic steps are:

  1. Add a number_type field to https://github.com/JuliaOpt/MathOptInterface.jl/blob/master/src/Test/config.jl#L1-L21
    so that users can set e.g., Float64, Rational, BigFloat, or Complex{Float64}.
  2. Pick a test like https://github.com/JuliaOpt/MathOptInterface.jl/blob/master/src/Test/contlinear.jl and replace all instances of Float64 with the type from config, ensuring that constant literals like https://github.com/JuliaOpt/MathOptInterface.jl/blob/6ac4e6d6ee47486062ab9a14816aca6d51328760/src/Test/contlinear.jl#L31 are promoted accordingly.
  3. Make a PR with a small number of changes (e.g., just one test for now).
  4. Once that PR is merged, pick another test and repeat. No need to convert everything in one go.

@blegat
Copy link
Member

blegat commented Aug 28, 2019

It might also be a type parameter of TestConfig so that the types local variables of the test functions are known at compile time

@ericphanson
Copy link
Contributor

In #855 some decisions were made about the details of doing this, and I think the following was more-or-less settled on for how to update a set of tests:

  • Change one test file per PR
  • Change test function signatures from f(args, config::TestConfig) to f(args, config::TestConfig{T}) where T
  • Use one(T) and zero(T) to replace 1.0 and 0.0 as numeric entries in test problems
  • Likewise, use T(n) to replace n.0 (for integer n)
  • Likewise, replace non-integer floats x by T(m//n) where x == m/n
  • Add type parameters to MOI.EqualTo, MOI.GreaterThan, MOI.LessThan when used as types (but not as constructors), and to MOI.ScalarAffineTerm, MOI.ScalarAffineFunction, MOI.VectorAffineTerm, and MOI.VectorAffineFunction.
  • Add spaces between elements of vectors, like [one(T), zero(T)] rather than [one(T),zero(T)]

This can be mostly done via find-and-replace. Slightly less obvious tricks:

  • For "T(n) to replace n.0", this can be done with regex-find (\d*?_?\d+?)\.0 and replace T($1)
  • For finding constructors but not types, search for e.g. MOI.EqualTo( (with the parenthesis to only get constructors)
  • For adding spaces between elements of vectors, regex-find (\[.+?),([^ ]), replace $1, $2, and repeat it until the file is invariant (since it adds only one space per vector per find-and-replace).

Tests to update:

And unit tests. Maybe those will need a different treatment though? I'm not really sure.

After the tests are updated, the tests of the tests will need to be updated. These will (hopefully) not pass until the internal use of Float64 numeric types in MOI are generalized, which should be done in separate PRs.

(Hope I got that all right, regarding the emergent plan).

@odow
Copy link
Member

odow commented Sep 1, 2019

This sounds fantastic. Small incremental steps.

@ericphanson
Copy link
Contributor

As an aside that may be relevant to those interested in this issue:

I've been setting up a framework similar to MOI's tests in Convex.jl, to allow solvers to test against Convex.jl's tests (what I've been calling the Convex.jl Problem Depot). The Convex.jl MOI branch uses this for its own tests, and I've generalized* them to arbitrary numeric type, which we are using to test the solvers in SDPAFamily.jl (né SDPA_GMP.jl).

*: They aren't fully generalized yet; so far I've just updated them so they use an MOIU.Model{T}, but the right-hand side of the tests are still only specified as Float64s, so they probably aren't suitable for setting very strict tolerances yet. However, they at least let you run through the whole solve process at high precision, which has already been helpful (highlighting a sorting issue in GenericLinearAlgebra and Float64 assumptions in MOI).

I see this testing framework as complementary to MOI's; it allows end-to-end testing of problems, with a different set of problems than MOI uses. An early version of this (just with Float64s) revealed #838 (the first correctness bug of MOI, at least as far as I could see looking at the bug label on the issue tracker), so I think it's useful.

@mtanneau
Copy link
Contributor

Is someone actively working on this? If not I may start to.

@ericphanson
Copy link
Contributor

That would be great! I’m not actively working on it now, and I don’t think @JiazhengZhu is either, but I can’t speak for anyone else.

@odow odow added good first issue Status: help wanted This issue needs could use help from the community Submodule: Tests About the Tests submodule labels Dec 14, 2020
@odow odow changed the title make tests run for generic reals Update MOI.Test to support any coefficient that is a subtype of Real Aug 13, 2021
@odow odow added this to the v1.x milestone Aug 31, 2021
@odow
Copy link
Member

odow commented Nov 19, 2021

We're cracking on with this #1667, #1668, #1669. But there are a loooooot of tests to go.

A simple search of Float64 shows:
image

@odow
Copy link
Member

odow commented Nov 23, 2021

I have been making progress. After #1681, only the 7000 lines of conic tests remain...

@odow
Copy link
Member

odow commented Nov 25, 2021

That was a pretty grinding few days, but it was faster than I thought it would be.

We now run a standard MOI.Test.runtests with BigFloat:

# Non-Float64 tests
MOI.Test.runtests(
MOI.Utilities.MockOptimizer(
MOI.Utilities.UniversalFallback(MOI.Utilities.Model{BigFloat}()),
BigFloat,
),
MOI.Test.Config(BigFloat),
exclude = [
# ========================= Expected failures ==========================
# UniversalFallback supports these tests.
"test_model_copy_to_UnsupportedAttribute",
"test_model_copy_to_UnsupportedConstraint",
"test_model_supports_constraint_ScalarAffineFunction_EqualTo",
"test_model_supports_constraint_VariableIndex_EqualTo",
"test_model_supports_constraint_VectorOfVariables_Nonnegatives",
],
)

This runs. all the tests automatically, so it should catch us re-introducing Float64-specific tests.

Closing because I think this is now done. The next release of MOI should be of interest to:

@odow odow closed this as completed Nov 25, 2021
@chriscoey
Copy link
Contributor Author

Nice work @odow

@odow
Copy link
Member

odow commented Nov 25, 2021

I can't promise I got every case, only that it works for BigFloat. For example, I think you'll run into a lot of issues if you use rational arithmetic, but that's probably to be expected because a lot of the tests don't have a rational solution.

We could potentially come up with a clever rational check if its a problem. @requires sqrt(T(2)) isa T?

julia> T = Rational{Int}
Rational{Int64}

julia> sqrt(T(2)) isa T
false

julia> T = BigFloat
BigFloat

julia> sqrt(T(2)) isa T
true

@chriscoey
Copy link
Contributor Author

Hypatia doesn't really work with rationals anymore so we won't need that, not sure about Tulip or COSMO

@mtanneau
Copy link
Contributor

Hypatia doesn't really work with rationals anymore so we won't need that, not sure about Tulip or COSMO

Same, I dropped Rational a while ago, in part because of the type-instability, in part because of numerical issues (the required precision would quickly explode)

@chriscoey
Copy link
Contributor Author

^ Hashtag relatable

@blegat
Copy link
Member

blegat commented Nov 28, 2021

CDDLib needs Rational{BigInt}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Status: help wanted This issue needs could use help from the community Submodule: Tests About the Tests submodule
Development

No branches or pull requests

5 participants