-
Notifications
You must be signed in to change notification settings - Fork 3
Support QP interface via MOI #30
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
base: main
Are you sure you want to change the base?
Conversation
src/MOI_wrapper.jl
Outdated
| # Is this a QP or an LP? | ||
| has_quadratic_objective = length(qobj_matrix_values) > 0 | ||
| if has_quadratic_objective && has_integrality | ||
| error("cuOpt does not support models with quadratic objectives _and_ integer variables") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what the best error / error message is here, happy to modify this as requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that cuOpt does not support MIQPs, only MILP or continuous QP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is okay for now.
As other options: you could remove the error and make it so that TerminationStatus was MOI.INVALID_MODEL. Or you could throw(MOI.SetAttributeNotAllowed(obj_attr, "cuOpt does not support ...")) but it's a coin toss whether the objective or integer constraint is the problem. And MOI doesn't have a bridge to fix this so the error is going to propagate to the user anyway.
64e7474 to
3a13f8c
Compare
|
From the README:
Which version of cuOpt introduced this QP API? |
It was added to the C API in cuOpt v25.12 (See release notes) |
|
@rgsl888prabhu should weigh in on if we want to bump the minimum supported version of cuOpt to 25.12 or add appropriate error messages when users try to solve QPs with an older version. Bumping the required version is ok with me since it makes maintenance of the wrapper easier. |
|
I think DCO is failing because the commit made via the github UI were not signed off |
Signed-off-by: mtanneau <[email protected]>
Signed-off-by: mtanneau <[email protected]>
Signed-off-by: mtanneau <[email protected]>
Signed-off-by: mtanneau <[email protected]>
Signed-off-by: mtanneau <[email protected]>
Signed-off-by: mtanneau <[email protected]>
c1313e4 to
067d973
Compare
Thank you, fixed and rebased |
rg20
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for implementing the QP interface for cuOpt!
I think the cuOpt version support might need changes in cuOpt.jl file. The QP support only works from 25.12.
| v = qterm.coefficient | ||
| if i == j | ||
| # Adjust diagonal coefficients to match cuOpt convention | ||
| v /= 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is accurate.
cuOpt expects users to provide the true objective function.
For example: if you are minimizing x1^2 + x2^2, the matrix should be [1 0; 0 1], cuOpt internally minimizes for (1/2) [x1 x2] [2 0; 0 2] [x1; x2] in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cuOpt expects users to provide the true objective function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens to the off-diagonal terms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cuOpt is not necessarily expecting the matrix to be symmetric.
If the objective is xT Q x + cT x, we internally symmetrize and solve for (1/2) xT (Q + QT) x + cT x
So if the objective is x1^2 + x2^2 + 2 x1 x2,
Q can be: [1 2; 0 1], or [1 0; 2 1] or [1 1; 1; 1]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(including the links for completeness)
- cuOpt docs: https://docs.nvidia.com/cuopt/user-guide/latest/lp-qp-features.html#quadratic-programming
- MOI docs: https://jump.dev/MathOptInterface.jl/stable/reference/standard_form/#MathOptInterface.ScalarQuadraticFunction
I believe the /2 factor for diagonal terms is required for correctness, but agreed it's not super clear why at first.
- The current MOI wrapper uses a JuMP-level data structure to store the optimization model as the user builds it. That internal representation abides by MOI conventions.
- when
optimize!gets called, the problem data is flushed from the JuMP-level data store into a cuOpt-level data structure --> this is why this PR mostly modifies thecopy_tofunction. - Hence, when we access the quadratic objective in
copy_to, we get as input anMOI.ScalarQuadraticFunction, which abides by the MOI convention described in the docs above.
Here is a small example to illustrate this
using JuMP
model = Model()
@variable(model, x)
@variable(model, y)
@objective(model, Min, x*x + 2*x*y + 3*y*y)
objective_function(model) # x² + 2 x*y + 3 y²
# Now, access the MOI representation
F = MOI.get(model, MOI.ObjectiveFunctionType())
f = MOI.get(model, MOI.ObjectiveFunction{F}())
f.quadratic termsthe last line outputs
3-element Vector{MathOptInterface.ScalarQuadraticTerm{Float64}}:
MathOptInterface.ScalarQuadraticTerm{Float64}(2.0, MOI.VariableIndex(1), MOI.VariableIndex(1))
MathOptInterface.ScalarQuadraticTerm{Float64}(2.0, MOI.VariableIndex(1), MOI.VariableIndex(2))
MathOptInterface.ScalarQuadraticTerm{Float64}(6.0, MOI.VariableIndex(2), MOI.VariableIndex(2))--> you can see that the diagonal coefficients were multiplied by 2. This was done by MOI in accordance with its convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rg20 I can add the following:
- add link to the MOI docs on quadratic function in that comment, to provide additional context
- add a couple of unit tests to validate the cuOpt solution and objective value when solving QP from MOI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My conclusion with this all is that the only valid way is to test, test, test, test, and test. There are too many subtleties to try and logically reason about the transformations, and every time I do, I end up making a mistake.
There are tests in MOI for various cases, but these are precisely the ones that you're skipping because of the upstream bug... 😢 ("test_objective_qp_ObjectiveFunction_zero_ofdiag" and "test_objective_qp_ObjectiveFunction_edge_cases").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtanneau thanks for explaining the subtleties in the MOI wrapper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added more unit tests that specifically trigger the QP objective.
Notes:
- the upstream issue regarding QP with no constraints might get fixed in cuopt v26.02 (tracking Use augmented system when there are no constraints NVIDIA/cuopt#765), which would allow to run more MOI-level tests.
- if the team is OK with adding JuMP as a test dependency, I'm happy to add similar tests where the QP is built from JuMP, not from MOI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JuMP shouldn't be a test dependency; MOI-style tests should be sufficient
@mlubin Yes, lets bump it to 25.12. |
Signed-off-by: mtanneau <[email protected]>
Signed-off-by: mtanneau <[email protected]>
Signed-off-by: mtanneau <[email protected]>
Signed-off-by: mtanneau <[email protected]>
This PR introduces support for solving Quadratic Programs with cuOpt.
Notes:
Please see extra comments in the diff