-
Notifications
You must be signed in to change notification settings - Fork 87
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
[Utilities] maintain order of variables in default_copy_to #2495
Conversation
So this, somewhat surprisingly, didn't break any tests. I don't know if that is a good thing or a bad thing... I guess it's because we're completely agnostic to the ordering of columns, so we always use |
Okay there are a few failures to look at in solver-tests. |
3c92105
to
73f3f52
Compare
73f3f52
to
401e9db
Compare
using Revise
import MathOptInterface as MOI
function build_src(M, N, p)
src = MOI.Utilities.Model{Float64}()
x = MOI.add_variables(src, N)
indices = unique(rand(1:N, round(Int, p * N)))
ci = MOI.add_constraint.(src, x[indices], MOI.GreaterThan(0.0))
indices = unique(rand(1:N, round(Int, p * N)))
ci = MOI.add_constraint.(src, x[indices], MOI.LessThan(1.0))
for _ in 1:M
i = rand(1:N)
indices = i:min(N, i + 10)
MOI.add_constraint(
src,
MOI.VectorOfVariables(x[indices]),
MOI.SecondOrderCone(length(indices)),
)
end
return src
end
julia> src = build_src(500_000, 1_000_000, 0.5)
MOIU.Model{Float64}
├ ObjectiveSense: FEASIBILITY_SENSE
├ ObjectiveFunctionType: MOI.ScalarAffineFunction{Float64}
├ NumberOfVariables: 1000000
└ NumberOfConstraints: 1287703
├ MOI.VectorOfVariables in MOI.SecondOrderCone: 500000
├ MOI.VariableIndex in MOI.GreaterThan{Float64}: 393979
└ MOI.VariableIndex in MOI.LessThan{Float64}: 393724 master julia> dest = MOI.Utilities.Model{Float64}(); GC.gc(); @time MOI.copy_to(dest, src);
1.415108 seconds (2.18 M allocations: 468.760 MiB, 7.19% gc time)
julia> dest = MOI.Utilities.Model{Float64}(); GC.gc(); @time MOI.copy_to(dest, src);
1.462140 seconds (2.18 M allocations: 485.807 MiB, 7.48% gc time)
julia> dest = MOI.Utilities.Model{Float64}(); GC.gc(); @time MOI.copy_to(dest, src);
1.292704 seconds (2.18 M allocations: 485.807 MiB)
julia> dest = MOI.Utilities.Model{Float64}(); GC.gc(); @time MOI.copy_to(dest, src);
1.426020 seconds (2.18 M allocations: 468.760 MiB, 8.28% gc time) This PR julia> dest = MOI.Utilities.Model{Float64}(); GC.gc(); @time MOI.copy_to(dest, src);
1.684707 seconds (3.15 M allocations: 598.370 MiB, 9.53% gc time)
julia> dest = MOI.Utilities.Model{Float64}(); GC.gc(); @time MOI.copy_to(dest, src);
1.691145 seconds (3.15 M allocations: 598.370 MiB, 8.83% gc time)
julia> dest = MOI.Utilities.Model{Float64}(); GC.gc(); @time MOI.copy_to(dest, src);
1.664689 seconds (3.15 M allocations: 598.370 MiB, 8.72% gc time)
julia> dest = MOI.Utilities.Model{Float64}(); GC.gc(); @time MOI.copy_to(dest, src);
1.805403 seconds (3.15 M allocations: 598.370 MiB, 8.82% gc time) So things are a little slower, but that is as could be expected. A few hundred ms slowdown for really large problems (10^6 variables and constraints) seems reasonable to ensure that we get ordered variables. |
So there are a bunch of solver-test failures that look problematic. |
Let's just see how many are because of this PR: https://github.com/jump-dev/MathOptInterface.jl/actions/runs/9491359380 |
I think a bunch might be because of #2508 |
1409c2d
to
00b22bf
Compare
OSQP failure is unrelated: osqp/OSQP.jl#126 |
The DSDP failure is suspicious: I assume it's because the variables are out of order, so we end up bridging it with some slack variables, which increases the problem size. Previously, we would have added the constraint as a variable, permuting the original variable order. |
Yes, I think we can ignore it |
My discussion with @blegat after the developer call is: this seems reasonable. There might be performance edge cases we haven't considered, but this is really a single sort, so it likely isn't the bottleneck. Fixing the order of variables is more important. |
So I didn't run BilevelJuMP was using |
This PR is an investigation of what we would need to implement to close #2493. It is still very rough around the edges; I need to clean it up and profile for performance, but as a demo, the case from #2493 now works:
TODOs