Add ModelWithQuad and privatize the product functions of QPBlockData - #3053
Add ModelWithQuad and privatize the product functions of QPBlockData#3053blegat wants to merge 15 commits into
Conversation
Making a first release with public add_constraint_jacobian_product, add_constraint_jacobian_transpose_product, and add_hessian_lagrangian_product commits MOI to three new function names. Instead, rename them _add_... and add ModelWithQuad/EvaluatorWithQuad: the evaluator implements the standard MOI.AbstractNLPEvaluator interface (including the product callbacks, composed inner-first so that store-semantics inner implementations such as ReverseAD do not clobber the QP contribution), so consumers only rely on existing generic functions and the QPBlockData internals stay private. ModelWithQuad routes affine and quadratic objectives and constraints to a QPBlockData and everything else to an inner model, forwards the MOI attribute queries of the QP block, and tracks where the objective lives. EvaluatorWithQuad remaps the variables of the QP block to their consecutive index in ordered_variables during MOI.initialize (variables absent from ordered_variables are parameters and keep their index, with the parameters dictionary aliased so per-solve value syncs are visible), and MOI.NLPBlockData(evaluator) assembles the combined constraint bounds.
No solver passes `inner = nothing`: the consumers all wrap an eager inner `Nonlinear.Model`, so drop the `set_objective(::Nothing, ::Nothing)` hook and the docstring sentence advertising it. A solver that manages its own nonlinear storage can still define `set_objective` for its inner type.
Nonlinear.set_objective is the single way to set the objective: it does everything the MOI.set method did, and it also accepts nothing to clear the objective, which the MOI attribute interface cannot express. The MOI getters stay since the function API has no counterpart for them.
ModelWithQuad now owns a MOI.Utilities.VariablesContainer and implements MOI.add_variable, guaranteeing variable indices 1:n like MOI.Utilities.MatrixOfConstraints, so the EvaluatorWithQuad no longer remaps the QP block. Parameters are added with MOI.add_constrained_variable: their indices are offset by _PARAMETER_OFFSET (moved here from Ipopt), QPBlockData is back to the simple offset-based _is_parameter instead of the parameters dictionary, and its parameters vector aliases the parameter storage of the inner Nonlinear.Model, so solvers no longer sync parameter values before a solve.
Sharing only when the inner model is a Nonlinear.Model silently left
qp.parameters empty for any other inner model type. Assume instead that the
inner model exposes its parameter values as parameters::Vector{T}, like
Nonlinear.Model does, and always alias it; an inner model without that field
fails loudly at construction.
They belong next to the convention; Ipopt and MadNLP defined them on their local alias of the function, which pirates it.
The substitution of the offset parameter indices by ParameterIndex before the inner model parses a function is index arithmetic tied to the parameter convention of the layer, so it belongs here: the generic add_constraint and set_objective now perform it, and the solvers just forward.
|
Some questions:
|
| indices offset by [`_PARAMETER_OFFSET`](@ref), and their values are stored in | ||
| the inner model through [`add_parameter`](@ref). The inner model must expose | ||
| that storage as `parameters::Vector{T}`, like [`Model`](@ref) does: | ||
| `qp.parameters` aliases it, so a parameter update is visible to both blocks. |
There was a problem hiding this comment.
Is this parameters::Vector{T} requirement supported by all solvers?
There was a problem hiding this comment.
This parameters::Vector{T} is just something both ArrayDiff and MOI.Nonlinear.Model should have. Solvers actually don't care about parameters. Ipopt for instance don't even support parameters, the MOI wrapper was just making them disappear. That's kind of why all this code is easy to share between solvers. Solvers just want simple callbacks. All of this is mostly just transforming all the complicated MOI things into these simple callbacks. We can almost reach a state where the solvers just forward all MOI things to their AD model and we don't even need to find Parameter when we grep their MOI wrapper.
| set the objective with [`set_objective`](@ref): affine and quadratic | ||
| functions are routed to the QP block, everything else to the inner model. | ||
| `objective_sink` records where the objective currently lives (`:none`, | ||
| `:quad` or `:inner`). |
There was a problem hiding this comment.
Don't use Symbol for things like enums.
|
|
||
| # Both blocks accumulate into the same variable-dimensional output. Call the | ||
| # inner evaluator FIRST because implementations are allowed to overwrite the | ||
| # output, and accumulate the QP block afterwards. |
There was a problem hiding this comment.
Which implementations?
And isn't implementations are allowed to overwrite -> implementations overwrite? I don't think we require the vector to be zeroed going in, so += is not correct.
Of course my other motivation is #3046 but I think we also do care about copy-paste. It's a bit like how
The fact that it's complicated is also kind of the reason to do it right just once, a bit like
This is essentially what this PR is doing except that we do it with composition. The advantage is that
We can do it in a follow-up PR. It can be either a third layer or added to |
Its docstring requires the order in which the variables were added, with the parameters interleaved, so ModelWithQuad records that order instead of appending the parameters after the variables.
An inner evaluator with constraint rows must support :Jac (and :Hess when the Hessian is queried), so do not silently skip it in jacobian_structure and hessian_lagrangian_structure.
MOI.eval_constraint_jacobian_transpose_product and MOI.eval_hessian_lagrangian_product store their result, zeroing the output, so calling the inner evaluator first makes pre-filling the output redundant. The Jacobian product only zeroes the rows of the QP block, into which the block accumulates; the inner evaluator stores its own rows.
_PARAMETER_OFFSET and _is_parameter are private and not part of any docs block, so the docstrings of QPBlockData and ModelWithQuad cannot link to them with @ref.
Add tests for the gradients of affine and quadratic objectives (affine and off-diagonal terms), the nonlinear objective gradient, the VariableIndex objective function type, the ConstraintSet getters and constraint-index filtering of every bound set, the Hessian and product evaluations of a parameterized QP block, nonlinear functions mentioning the parameter and the variable directly, and a parameter-free affine subfunction that the substitution leaves as is.
Add nonlinear constraints with quadratic subfunctions, with and without a parameter, and query ListOfSupportedNonlinearOperators through the layer. These methods were never compiled by the tests, which local coverage reports as non-executable lines but codecov reports as uncovered.
|
CI is now green on this PR and also all 5 other downstream PRs (except MadNLP but there seems to be an issue with the ci there). @odow any objection to merge ? |
|
If you're set on this ModelWithQuad thing, then can you merge each thing into one PR per package? It's kinda hard to review at the moment. I'm not overly enthused by having another 1000 lines of code in MOI... |
2 participants
As discussed in #3048 (comment), it might be better to directly go for
ModelWithQuadand have the solver use that directly so that we don't have to make the interface toQPBlockDatapublic and we can change it later.Github is suggesting me to "Start a pull request stack", I didn't know this feature, let's try.