Add DenseKKTSystem structure #65

frapac · 2021-08-30T11:16:41Z

Slight revisitation of the AbstractKKTSystem interface
New constructor that allows to build an AbstractKKTSystem directly from a NonLinearProgram
Add special tests to ensure that DenseKKTSystem matches exactly SparseKKTSystem
Add Random to tests' dependencies

codecov · 2021-08-30T11:27:50Z

Codecov Report

Merging #65 (9a33164) into master (64657de) will increase coverage by 0.64%.
The diff coverage is 97.34%.

@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   86.69%   87.33%   +0.64%     
==========================================
  Files          30       30              
  Lines        3141     3213      +72     
==========================================
+ Hits         2723     2806      +83     
+ Misses        418      407      -11

Impacted Files	Coverage Δ
src/matrixtools.jl	`100.00% <ø> (+17.39%)`	⬆️
src/interiorpointsolver.jl	`93.15% <96.92%> (+0.26%)`	⬆️
src/kktsystem.jl	`98.72% <97.11%> (-1.28%)`	⬇️
src/nonlinearprogram.jl	`82.60% <100.00%> (+62.60%)`	⬆️
src/MadNLP.jl	`50.00% <0.00%> (-12.50%)`	⬇️
src/utils.jl	`73.68% <0.00%> (-7.80%)`	⬇️
src/Interfaces/MOI_interface.jl	`84.37% <0.00%> (-0.60%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64657de...9a33164. Read the comment docs.

frapac · 2021-08-30T13:06:36Z

Solve #36

sshin23

Hey @frapac what an incredible work! It seems that the AbstractKKTSystem abstraction you built really pays here. It is even quite surprising that dense formulation just works with this small change. I only have a few minor comments. Could you run a benchmark just to make sure that we don't have an unexpected performance regression? I think running the benchmark with --quick option would be enough here.

sshin23 · 2021-08-31T03:27:34Z

src/interiorpointsolver.jl

@@ -331,13 +332,26 @@ function Solver(nlp::NonlinearProgram;
    set_blas_num_threads(opt.blas_num_threads; permanent=true)

    @trace(logger,"Initializing variables.")
-    ind_ineq = findall(nlp.gl.!=nlp.gu)
-    ns = length(ind_ineq)
+    info_constraints = get_index_constraints(nlp; fixed_variable_treatment=opt.fixed_variable_treatment)


This makes the code way cleaner :) Maybe just rename it consistently as ind_cons?

sshin23 · 2021-08-31T03:35:12Z

src/interiorpointsolver.jl

+
+function set_con_scale!(con_scale::AbstractVector, jac::SparseMatrixCOO, nlp_scaling_max_gradient)
+    for i in 1:nnz(jac)
+        row = jac.I[i]


Maybe @inbounds here

Good catch, indeed.

sshin23 · 2021-08-31T03:43:47Z

src/kktsystem.jl

+    ind_fixed::Vector{Int}
+    jacobian_scaling::VT
+end
+


sshin23 · 2021-08-31T03:46:19Z

src/kktsystem.jl

+
+    # If the the problem is unconstrained, then KKT system is directly equal
+    # to the Hessian (+ some regularization terms)
+    aug_com = if (m == 0)


interesting, maybe there are things we can do less (or further) in the NLP formulation as well as the algorithm when it is unconstrained.

Indeed. We may want to use a different line-search in the unconstrained case, too. Maybe that's something we want to investigate in the future.

sshin23 · 2021-08-31T03:48:20Z

src/kktsystem.jl

+    jac::MT
+    pr_diag::VT
+    du_diag::VT
+    diag_hess::VT


why do we separately need diag_hess, which we don't have in sparse KKT?

In sparse KKT, pr_diag and du_diag are defined implicitly in the KKT's COO matrix, by adding artificial duplicate entries to the sparsity pattern. That allows to do the regularization seamlessly.

In dense KKT, things are a bit different, as we cannot store pr_diag and du_diag directly in the dense KKT matrix. Each time we update the regularization, we need to update the diagonal terms in the dense KKT matrix, as

for i in 1:n aug_com[i, i] = hess[i, i] + pr_diag[i]

In case aug_com === hess, we are adding two time the terms pr_diag in the second regularization. That's why I thought it would be better to store the diagonal terms of the Hessian matrix once, and then do the regularization as

for i in 1:n aug_com[i, i] = diag_hess[i] + pr_diag[i]

But there might be a better solution!

sshin23 · 2021-08-31T03:52:48Z

test/madnlp_test.jl

@@ -51,7 +50,6 @@ testset = [
        ()->MadNLP.Optimizer(
            inertia_correction_method=MadNLP.INERTIA_FREE,
            kkt_system=MadNLP.SPARSE_UNREDUCED_KKT_SYSTEM,
-            reduced_system=false,


oops. Great catch!

sshin23 · 2021-08-31T04:19:33Z

I'll add a benchmark for dense problems using the dense problems within CUTEst instances.

sshin23 · 2021-08-31T04:44:13Z

One observation is that the current DenseKKT system assumes a different behavior of NonlinearProgramming functions; in particular, the hessian and jacobian callback functions should behave differently. Maybe it would be good to introduce a new abstract type AbstractNonlinearProgram, and separate SparseNonlinearProgram and DenseNonlinearProgram. And this can be independent of the KKTSystem type, since some dense NLPs are in sparse NLP format and the vice versa is also possible.

Could you make those NonlinearProgram subtypes? I can work on MOI/NLPModels interfaces in a separate PR

frapac · 2021-09-01T07:59:41Z

You are raising an interesting question about NonlinearProgramming functions. Personally, I am in favor of keeping the current NonlinearProgramming as is. The reasons are

I think NonlinearProgramming (model) and KKTSystem (solver) are two different abstractions. We may want to solve a sparse nonlinear problem in dense mode (but the reverse is rarely true, indeed).
In my opinion, specifying dense callbacks in a NonlinearProgramming object should happen at the dense level, by adding new callbacks for the Jacobian and the Hessian supporting dense inputs:
```
    function eval_hess(hess::AbstractMatrix, x, l, sig)
        ...
    end
```
(but current situation is not ideal though, as we are defining the callbacks as closures)
but I agree that current situation is not great, too. When we are using MadNLP in dense mode, we should at least check that the corresponding dense callbacks for Jacobian and Hessian are available in the NonlinearProgramming model passed as input.

sshin23 · 2021-09-01T21:44:15Z

Indeed. Totally agreed.

I think one way we can check the availability of sparse (or dense) callback is by checking length(methods(nlp.lag_hess!,(AbstractVector,Any,Any,Any))) != 0. Does julia have a function like iscallable(f,[types])::Bool?

With regards to closures, I think we should discuss entirely relying on NLPModels and getting rid of NonlinearProgram. Then we can save time for supporting the MOI interface, which is quite complex. But of course, not in this PR :)

sshin23 · 2021-09-01T21:52:15Z

src/kktsystem.jl

+
+struct DenseKKTSystem{T, VT, MT} <: AbstractKKTSystem{T, MT}
+    hess::MT
+    jac::MT


Didn't realize this in my first review, but can't these be views so that we don't need to transfer this in _build_dense_kkt_system?

* add proper constructor for KKTSystems

frapac requested a review from sshin23 August 30, 2021 11:16

sshin23 approved these changes Aug 31, 2021

View reviewed changes

sshin23 reviewed Sep 1, 2021

View reviewed changes

frapac added 2 commits September 3, 2021 14:24

add DenseKKTSystem structure

4d32d25

* add proper constructor for KKTSystems

rescope with remarks from PR#65

fe8ee1f

frapac force-pushed the fp/densekktsystem branch from 748978c to fe8ee1f Compare September 3, 2021 14:01

check that dense callbacks are available when DenseKKTSystem is used

9a33164

sshin23 merged commit 23b5328 into MadNLP:master Sep 3, 2021

frapac mentioned this pull request Sep 13, 2021

Add support for dense nonlinear programming #36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DenseKKTSystem structure #65

Add DenseKKTSystem structure #65

frapac commented Aug 30, 2021

codecov bot commented Aug 30, 2021 •

edited

Loading

frapac commented Aug 30, 2021

sshin23 left a comment

sshin23 Aug 31, 2021

frapac Sep 1, 2021

sshin23 Aug 31, 2021

frapac Sep 1, 2021

sshin23 Aug 31, 2021

sshin23 Aug 31, 2021

frapac Sep 1, 2021

sshin23 Aug 31, 2021

frapac Sep 1, 2021

sshin23 Aug 31, 2021

sshin23 commented Aug 31, 2021

sshin23 commented Aug 31, 2021 •

edited

Loading

frapac commented Sep 1, 2021

sshin23 commented Sep 1, 2021 •

edited

Loading

sshin23 Sep 1, 2021

Add DenseKKTSystem structure #65

Add DenseKKTSystem structure #65

Conversation

frapac commented Aug 30, 2021

codecov bot commented Aug 30, 2021 • edited Loading

Codecov Report

frapac commented Aug 30, 2021

sshin23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sshin23 commented Aug 31, 2021

sshin23 commented Aug 31, 2021 • edited Loading

frapac commented Sep 1, 2021

sshin23 commented Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 30, 2021 •

edited

Loading

sshin23 commented Aug 31, 2021 •

edited

Loading

sshin23 commented Sep 1, 2021 •

edited

Loading