Conversation
|
Super happy to see there is some interest in this!! You may be interested to check out (the now rather stale, unfortunately) work in https://github.com/madsuite-org/MadDiff.jl (and the forks linked in its readme). It would be great to finally land those features properly! |
|
Very nice work! I am glad we are pursuing this direction in MadNLP. We had a brief discussion together with @klamike . I think it would be good to provide also the reverse pass ( The only part I am a bit unsure with is the user interface. We should think a little bit about what does the user expect exactly. I don't have any better idea for the moment, but I think it's worth discussing. |
frapac
left a comment
There was a problem hiding this comment.
Overall looks good to me, I have only a few minor comments. I think this code should belongs to MadNLP, and that we should make sensitivity analysis a first-class citizen in our package suite.
I would suggest making explicit that we are computing the sensitivities using forward-mode AD, e.g. by adding _jvp in the name (I welcome any better suggestion of course). The reverse mode is slightly different, as the unreduced KKT system is not symmetric (hence the order in which we reduce the RHS matters). MadDiff figures this out.
| - `px`: variable block, `nvar × k` | ||
| - `py`: constraint block, `ncon × k` | ||
| """ | ||
| function backsolve_kkt!(solver::AbstractMadNLPSolver{T}, px::AbstractMatrix, py::AbstractMatrix) where T |
There was a problem hiding this comment.
genuine question: why keeping px and py split?
| - `py`: constraint block, `ncon × k` | ||
| """ | ||
| function backsolve_kkt!(solver::AbstractMadNLPSolver{T}, px::AbstractMatrix, py::AbstractMatrix) where T | ||
| get_status(solver) in (SOLVE_SUCCEEDED, SOLVED_TO_ACCEPTABLE_LEVEL) || |
There was a problem hiding this comment.
I would suggest reformulating as:
if get_status(solver) in (SOLVE_SUCCEEDED, SOLVED_TO_ACCEPTABLE_LEVEL)
...
end(I think it is more readable)
| ifree, ufree = _free_indices(cb) | ||
| x0 = get_x0(nlp) | ||
| dx, dy, dzl, dzu = (fill!(similar(x0, n, k), zero(T)) for n in (nvar, ncon, nvar, nvar)) | ||
| for j in 1:k |
There was a problem hiding this comment.
to decide together with @sshin23 : should we keep the for loop inside the function, or outside? If the user wants to assemble the full Jacobian, we have to make explicit that this is an expensive operation in the large-scale regime (we store a dense matrix with size (n+m, k))
| unpack_z!(view(dz, :, j), cb, view(zbuf, 1:nx)) | ||
| end | ||
| end | ||
| return (; dx, dy, dzl, dzu) |
There was a problem hiding this comment.
suggestion: maybe a named-tuple?
| """ | ||
| sensitivity(solver::AbstractMadNLPSolver, Hxθ::AbstractMatrix, Jθ::AbstractMatrix) = | ||
| backsolve_kkt!(solver, .-Hxθ, .-Jθ) | ||
| function sensitivity(solver::AbstractMadNLPSolver, θ...) |
There was a problem hiding this comment.
I would prefer keeping the argument θ explicit instead of using the splatting θ.... What is the type of θ exactly?
| - `restore_parameter`: restore `nlp[θ]` after the call | ||
| - `recompute_residuals`: recompute `primal_feas` and `dual_feas` at the new solution | ||
| """ | ||
| function sensitivity_result(solver::AbstractMadNLPSolver, θ, θnew; restore_parameter = true, recompute_residuals = false) |
There was a problem hiding this comment.
I am not sure that this particular operation should be implemented. I would let the user calls sensitivitity directly. Let me know what do you think.
| θ0 = copy(nlp[θ]) | ||
| dθ = copyto!(similar(θ0), θnew) .- θ0 | ||
| s = sensitivity(solver, θ) | ||
| nlp[θ] = θnew |
There was a problem hiding this comment.
This looks unfamiliar to me. Is it also particular to ExaModels?
| stats.multipliers_L .+= s.dzl * dθ | ||
| stats.multipliers_U .+= s.dzu * dθ | ||
| stats.objective = NLPModels.obj(nlp, stats.solution) | ||
| get_ncon(nlp) > 0 && NLPModels.cons!(nlp, stats.solution, stats.constraints) |
There was a problem hiding this comment.
for the two following lines, I would write explicit if statement
| end | ||
|
|
||
| _violation(v, l, u) = max(maximum(l .- v; init = zero(eltype(v))), maximum(v .- u; init = zero(eltype(v)))) | ||
| function _residuals!(stats::MadNLPExecutionStats, nlp) |
There was a problem hiding this comment.
Maybe add a comment to explain what this function is doing?
| (nlp::ParametricModel)(x, y) = (nlp.Hxθ, nlp.Jθ) | ||
| (nlp::ParametricModel)(::Θ, x, y) = nlp(x, y) | ||
| Base.getindex(nlp::ParametricModel, ::Θ) = nlp.θ | ||
| Base.setindex!(nlp::ParametricModel, v, ::Θ) = (nlp.θ .= v; nlp) |
There was a problem hiding this comment.
This is exactly the kind of overloading operations that is not standard in NLPModels. This can be a solution in the short-term, but I think it's worth discussing a long-term solution that can suit anybody. Maybe time to revive the discussion in JuliaSmoothOptimizers/NLPModels.jl#557 ?
I spoke w/ sungho earlier and we agree it may be a good time to bring back ParametricNLPModels.jl, i moved over the relevant api from these PRs and also merged some of ur ideas from ur older version to a madsuite-org version @klamike |
Adds 2 new functions to export/public API:
sensitivity(solver, p)where
solver = MadNLPSolver(...)aftersolve!ing andpis fromadd_parwithsensitivity = true.calculates
ds*/dpwheres* = (x*, y*, zl*, zu*), returned as(; dx, dy, dzl, dzu)sensitivity_result(p , p_new)evaluates
w*(p_new)by taking the step fromp->p_new, so basically justs* + ds*/dp*(p_new - p), returns aMadNLPExecutionStatsstruct w/ the model evaluated at the new solutionthe actual derivative calculations
dc/dpandd^2L/dxdphappens on the ExaModels side, since the AD happens there (also so support is general for other NLPModels solvers)minimal example of what it looks like in practice, tested and confirmed on cpu and gpu, combined with the corresponding ExaModels PR "Add differentiate wrt parameters for parametric sensitivity calculations":