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 to MOI v1 #66

Merged
merged 7 commits into from
Jun 27, 2023
Merged

Update to MOI v1 #66

merged 7 commits into from
Jun 27, 2023

Conversation

blegat
Copy link
Contributor

@blegat blegat commented Jun 2, 2023

Closes #53

Would be helpful to have this working now generic number support is arriving in JuMP: jump-dev/JuMP.jl#3385

Comment on lines +97 to +98
optimizer.dualobj = parse(T, objValPrimalstring)
optimizer.primalobj = parse(T, objValDualstring)
Copy link
Owner

Choose a reason for hiding this comment

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

why are these swapped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now use MOI.FileFormats.SDPA which expects the SDP in geometric conic form / image form while before you were doing like CSDP and SDPA which expect it to be in standard conic form / kernel form

Comment on lines +13 to +16
if !iszero(constant)
func = copy(func)
func.constant = zero(constant)
end
Copy link
Owner

Choose a reason for hiding this comment

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

could you explain a bit more? It looks like we totally disregard the constant by throwing it away here, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's saved in a field of Optimizer in optimize! and added back when the user asks for the objective value.

Comment on lines 64 to 85
# FIXME investigate
# Expression: isapprox(MOI.get(model, MOI.ObjectiveValue()), T(2), config)
# Evaluated: isapprox(5.999999984012059, 2.0, ...
r"test_modification_delete_variables_in_a_batch$",
# FIXME investigate
# Expression: isapprox(MOI.get(model, MOI.ObjectiveValue()), objective_value, config)
# Evaluated: isapprox(-2.1881334077988868e-7, 5.0, ...
r"test_objective_qp_ObjectiveFunction_edge_case$",
# FIXME investigate
# Expression: isapprox(MOI.get(model, MOI.ObjectiveValue()), objective_value, config)
# Evaluated: isapprox(-2.1881334077988868e-7, 5.0, ...
r"test_objective_qp_ObjectiveFunction_zero_ofdiag$",
# FIXME investigate
# Expression: isapprox(MOI.get(model, MOI.ConstraintPrimal(), index), solution_value, config)
# Evaluated: isapprox(2.5058846553349667e-8, 1.0, ...
r"test_variable_solve_with_lowerbound$",
# FIXME investigate
# See https://github.com/jump-dev/SDPA.jl/runs/7246518765?check_suite_focus=true#step:6:128
# Expression: ≈(MOI.get(model, MOI.ConstraintDual(), c), T[1, 0, 0, -1, 1, 0, -1, -1, 1] / T(3), config)
# Evaluated: ≈([0.3333333625488728, -0.16666659692134123, -0.16666659693012292, -0.16666659692134123, 0.33333336253987234, -0.16666659692112254, -0.16666659693012292, -0.16666659692112254, 0.333333362548654], [0.3333333333333333, 0.0, 0.0, -0.3333333333333333, 0.3333333333333333, 0.0, -0.3333333333333333, -0.3333333333333333, 0.3333333333333333]
r"test_conic_PositiveSemidefiniteConeSquare_3$",
# FIXME
Copy link
Owner

Choose a reason for hiding this comment

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

are these copied from SDPA.jl? Some of them might pass here with the presolve (because we've found that is necessary to function correctly in some cases)

Copy link
Contributor Author

@blegat blegat Jun 2, 2023

Choose a reason for hiding this comment

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

Yes but I have not updated the presolve code yet. It should be udpated to work with MOI.FileFormats.SDPA.Model. Maybe https://github.com/mtanneau/MathOptPresolve.jl could be used ? I haven't checked in details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to take a look at https://github.com/mtanneau/MathOptPresolve.jl so to see if it has everything needed for convex problems and then recommend people to always try using this layer (in addition to the Dualization.jl layer which should always be tried for conic optimization).

Copy link
Owner

Choose a reason for hiding this comment

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

it seems like MathOptPresolve doesn't support MOI v1, and also doesn't have docs explaining how to use it

@blegat
Copy link
Contributor Author

blegat commented Jun 6, 2023

@ericphanson Any idea about the Convex.jl failing tests ? Is this only because we removed the presolve ?

@ericphanson
Copy link
Owner

Yeah, I think so. It is really essential to get anywhere close to correct results in many cases.

@ericphanson
Copy link
Owner

ericphanson commented Jun 6, 2023

Running the tests locally, I see

[ Info: Solving problem sdp_trace_logm_real with variant sdpa and type Double64
sdp_trace_logm_real: Error During Test at /Users/eph/.julia/packages/Convex/tSTAW/src/problem_depot/problems/sdp.jl:2074
  Test threw exception
  Expression: (p.optval, tr(C * log(evaluate(X))), atol = atol, rtol = rtol)
  MethodError: no method matching eigen!(::Hermitian{Double64, Matrix{Double64}}; sortby::typeof(LinearAlgebra.eigsortby))
  
  Closest candidates are:
    eigen!(::Union{Hermitian{T, var"#s967"}, Hermitian{Complex{T}, var"#s967"}, Symmetric{T, var"#s967"}} where var"#s967"<:(StridedMatrix{T} where T), ::AbstractMatrix{T}; sortby) where T<:Number
     @ LinearAlgebra ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/LinearAlgebra/src/symmetriceigen.jl:167
    eigen!(::Hermitian; tol) got unsupported keyword argument "sortby"
     @ GenericLinearAlgebra ~/.julia/packages/GenericLinearAlgebra/skQkq/src/eigenSelfAdjoint.jl:605
    eigen!(::StridedMatrix{T}; permute, scale, sortby) where T<:Union{Float32, Float64}
     @ LinearAlgebra ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/LinearAlgebra/src/eigen.jl:149
    ...
  
  Stacktrace:
   [1] kwerr(::NamedTuple{(:sortby,), Tuple{typeof(LinearAlgebra.eigsortby)}}, ::Function, ::Hermitian{Double64, Matrix{Double64}})
     @ Base ./error.jl:165
   [2] eigen(A::Matrix{Double64}; permute::Bool, scale::Bool, sortby::typeof(LinearAlgebra.eigsortby))
     @ LinearAlgebra ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/LinearAlgebra/src/eigen.jl:237
   [3] eigen
     @ ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/LinearAlgebra/src/eigen.jl:235 [inlined]
   [4] #eigvecs#98
     @ ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/LinearAlgebra/src/eigen.jl:270 [inlined]
   [5] eigvecs
     @ ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/LinearAlgebra/src/eigen.jl:270 [inlined]
   [6] matrixfunction(fn::Function, m::Matrix{Double64})
     @ DoubleFloats ~/.julia/packages/DoubleFloats/3G2HG/src/math/linearalgebra/matrixfunction.jl:3
   [7] log(m::Matrix{Double64})
     @ DoubleFloats ~/.julia/packages/DoubleFloats/3G2HG/src/math/linearalgebra/matrixfunction.jl:33
   [8] macro expansion
     @ ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/Test/src/Test.jl:478 [inlined]
   [9] sdp_trace_logm_real(handle_problem!::var"#22#25"{String, Symbol, DataType}, #unused#::Val{true}, atol::Float64, rtol::Float64, #unused#::Type{Double64})
     @ Convex.ProblemDepot ~/.julia/packages/Convex/tSTAW/src/problem_depot/problems/sdp.jl:2074

which looks like breakage in GenericLinearAlgebra to me.

Also:

[ Info: Solving problem sdp_trace_mpower_cplx_5_4 with variant sdpa_dd and type BigFloat
sdp_trace_mpower_cplx_5_4: Error During Test at /Users/eph/.julia/packages/Convex/tSTAW/src/problem_depot/problems/sdp.jl:1955
  Test threw exception
  Expression: (p.optval, evaluate(trace_mpower(B, t, C)), atol = atol, rtol = rtol)
  MethodError: no method matching (Schur{Complex})(::GenericLinearAlgebra.Schur{Complex{BigFloat}, Matrix{Complex{BigFloat}}})
  
  Closest candidates are:
    (Schur{CT})(::Schur{<:Real}) where CT<:Complex
     @ LinearAlgebra ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/LinearAlgebra/src/schur.jl:223
    (Schur{Complex})(::Schur{<:Complex})
     @ LinearAlgebra ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/LinearAlgebra/src/schur.jl:256
    (Schur{T})(::Schur{T}) where T
     @ LinearAlgebra ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/LinearAlgebra/src/schur.jl:257
    ...
  
  Stacktrace:
   [1] schurpow(A::Matrix{Complex{BigFloat}}, p::Rational{Int64})
     @ LinearAlgebra ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/LinearAlgebra/src/dense.jl:502
   [2] ^(A::Matrix{Complex{BigFloat}}, p::Rational{Int64})
     @ LinearAlgebra ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/LinearAlgebra/src/dense.jl:547
   [3] trace_mpower(A::Matrix{Complex{BigFloat}}, t::Rational{Int64}, C::Matrix{ComplexF64})
     @ Convex ~/.julia/packages/Convex/tSTAW/src/atoms/sdp_cone/trace_mpower.jl:78
   [4] evaluate(atom::Convex.TraceMpower)
     @ Convex ~/.julia/packages/Convex/tSTAW/src/atoms/sdp_cone/trace_mpower.jl:66
   [5] macro expansion
     @ ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/Test/src/Test.jl:478 [inlined]
   [6] sdp_trace_mpower_cplx_5_4(handle_problem!::var"#22#25"{String, Symbol, DataType}, #unused#::Val{true}, atol::Float64, rtol::Float64, #unused#::Type{BigFloat})
     @ Convex.ProblemDepot ~/.julia/packages/Convex/tSTAW/src/problem_depot/problems/sdp.jl:1955

and

[ Info: Solving problem sdp_trace_logm_cplx with variant sdpa_dd and type BigFloat
sdp_trace_logm_cplx: Error During Test at /Users/eph/.julia/packages/Convex/tSTAW/src/problem_depot/problems/sdp.jl:2102
  Test threw exception
  Expression: (p.optval, tr(C * log(evaluate(X))), atol = atol, rtol = rtol)
  type Schur has no field vectors
  Stacktrace:
   [1] getproperty(F::GenericLinearAlgebra.Schur{Complex{BigFloat}, Matrix{Complex{BigFloat}}}, s::Symbol)
     @ GenericLinearAlgebra ~/.julia/packages/GenericLinearAlgebra/skQkq/src/eigenGeneral.jl:76
   [2] log(A::Matrix{Complex{BigFloat}})
     @ LinearAlgebra ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/LinearAlgebra/src/dense.jl:822
   [3] macro expansion
     @ ~/.julia/juliaup/julia-1.9.0+0.x64.apple.darwin14/share/julia/stdlib/v1.9/Test/src/Test.jl:478 [inlined]
   [4] sdp_trace_logm_cplx(handle_problem!::var"#22#25"{String, Symbol, DataType}, #unused#::Val{true}, atol::Float64, rtol::Float64, #unused#::Type{BigFloat})
     @ Convex.ProblemDepot ~/.julia/packages/Convex/tSTAW/src/problem_depot/problems/sdp.jl:2102

seem like upstream issues too (not sure if from Base, GenericLinearAlgebra, or Convex)

@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2023

Codecov Report

Patch coverage: 86.90% and project coverage change: -14.41 ⚠️

Comparison is base (655008f) 88.56% compared to head (5e655f8) 74.16%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master      #66       +/-   ##
===========================================
- Coverage   88.56%   74.16%   -14.41%     
===========================================
  Files           6        7        +1     
  Lines         551      569       +18     
===========================================
- Hits          488      422       -66     
- Misses         63      147       +84     
Impacted Files Coverage Δ
src/SDPAFamily.jl 75.00% <ø> (ø)
src/params.jl 91.17% <ø> (ø)
src/MOI_wrapper.jl 79.50% <86.56%> (-3.12%) ⬇️
src/objective_function_filter.jl 86.66% <86.66%> (ø)
src/file_io.jl 66.91% <100.00%> (-33.09%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@blegat
Copy link
Contributor Author

blegat commented Jun 27, 2023

@ericphanson Tests are passing now, the documentation failure seems unrelated. Good to merge and release ? :)

Copy link
Owner

@ericphanson ericphanson left a comment

Choose a reason for hiding this comment

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

We should also:

  • breaking version bump
  • warning in readme that presolve is not enabled, linking to issue
  • update docs where presolve is discussed

I can do that as a separate PR though.

test/MOI_wrapper.jl Outdated Show resolved Hide resolved
@blegat
Copy link
Contributor Author

blegat commented Jun 27, 2023

Nice, tests are passing with BigFloat as well

@blegat
Copy link
Contributor Author

blegat commented Jun 27, 2023

I added the steps in #67

@ericphanson
Copy link
Owner

I think you were already running them, they were just under the category Float64 in the loop there

@ericphanson ericphanson merged commit 610e665 into ericphanson:master Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to MOI v1
3 participants