Use inplace when building the NLP model

jbcaillau commented 3 months ago

@PierreMartinon @ocots @gergaud it is actually possible but takes a rather deep revision of CTBase, abstract / fun API:

an OCP could be declared inplace when created, requesting then that every function providing dyn / cost / constraints) be inplace
from the abstract syntax, it is possible to generate inplace code only
lot of memory saving expected, I suppose
in the "building a better NLP" line, I think (but might be wrong) that https://github.com/control-toolbox/CTDirect.jl/issues/183#issuecomment-2268306797 is a higher priority

On a side note: more and more, I think that the functional API should be kept for internal use only.

jbcaillau commented 3 months ago

in the "building a better NLP" line, I think (but might be wrong) that Pass sparse structure of constraints to NLPModels #183 (comment) is a higher priority

@amontoison your opinion on top priority for a better NLP (performance improvement) between (i) passing the sparse structure and (ii) going fully inplace?

amontoison commented 3 months ago

in the "building a better NLP" line, I think (but might be wrong) that Pass sparse structure of constraints to NLPModels #183 (comment) is a higher priority

@amontoison your opinion on top priority for a better NLP (performance improvement) between (i) passing the sparse structure and (ii) going fully inplace?

In-place (ii) is very important because memory allocations are expensive and you will also remove some allocations in the AD part so you will have a speed-up in different part of the code.

(i) is just a one-shot gain speed-up at the creation of your NLP model.

You can work on both but the priority is higher on (ii).

jbcaillau commented 3 months ago

in the "building a better NLP" line, I think (but might be wrong) that Pass sparse structure of constraints to NLPModels #183 (comment) is a higher priority

@amontoison your opinion on top priority for a better NLP (performance improvement) between (i) passing the sparse structure and (ii) going fully inplace?

In-place (ii) is very important because memory allocations are expensive and you will also remove some allocations in the AD part so you will have a speed-up in different part of the code.

(i) is just a one-shot gain speed-up at the creation of your NLP model.

You can work on both but the priority is higher on (ii).

👍🏽 thanks for the insight. priority on (ii), then.

jbcaillau commented 3 months ago

@jbcaillau For the record:

julia> foo!(y, x) = begin
       y[:] .= x
       return 
       end
foo! (generic function with 1 method)

julia> y = [0]; foo!(y, 3); y
1-element Vector{Int64}:
 3

julia> y = [0]; foo!(y, [3]); y
1-element Vector{Int64}:
 3

jbcaillau commented 2 months ago

jbcaillau commented 1 month ago

@PierreMartinon PR merged https://github.com/control-toolbox/CTBase.jl/pull/271 up to you 🤞🏾!

ocots commented 1 month ago

@PierreMartinon PR merged https://github.com/control-toolbox/CTBase.jl/pull/271 up to you 🤞🏾!

Need a new release. Y must be updated.

jbcaillau commented 1 month ago

@PierreMartinon PR merged control-toolbox/CTBase.jl#271 up to you 🤞🏾!

Need a new release. Y must be updated.

✅ @PierreMartinon you can use CTBase.jl v0.14.0

PierreMartinon commented 1 month ago

ok, thanks !

jbcaillau commented 1 month ago

@PierreMartinon attaching allocation sizes with the current out of place code on the benchmark created by @0Yassine0 https://github.com/0Yassine0/COTS.jl/issues/24#issuecomment-2343573263

Model_Benchmark_TTonly_file.pdf

jbcaillau commented 1 month ago

PS. @PierreMartinon @0Yassine0 note that definition such the dynamics in the problem below (source here) will not generate in place code; the dynamics actually needs to be "inlined" inside @def. See also https://github.com/control-toolbox/CTBase.jl/issues/292

jbcaillau commented 1 month ago

@PierreMartinon any progress?

PierreMartinon commented 1 month ago

@jbcaillau @amontoison Still hunting for type unstabilities and allocations, but at least the getters are fine now !

Back to the whole inplace aspect, I am starting to wonder if it will change much: the function for the NLP constraints has already been inplace since basically the beginning, and the OCP functions are typically used as:

c[i:j] = ocp.some_constraint(...) for all the different constraints kinds
c[i:j] = x_i_plus_1 - (x_i + h_i * ocp.dynamics(...)) for the dynamics (here I wrote the formula for explicit Euler but the general form is the same for other discretizations)

Seeing this, the destination for the OCP functions is already allocated in c, so do we really benefit from them being inplace ? Inplace version currently looks like

ocp.some_constraints(view c[i:j], ...)
for the dynamics it's a bit convoluted ocp.dynamics(view c[i:j], ...) c[i:j] = - h_i * c[i:j] - x_i + x_i_plus_1

Am I missing something ?

jbcaillau commented 1 month ago

hi @PierreMartinon

@jbcaillau @amontoison Still hunting for type unstabilities and allocations, but at least the getters are fine now !

good, what did you do, in the end?

c[i:j] = ocp.some_constraint(...) for all the different constraints kinds [...]

ocp.some_constraints(view c[i:j], ...)

Er... precisely: ocp.some_constraints(@view c[i:j], ...) is in place while the previous one is not, that's the whole point. Agree? (See foo2 below, though.)

julia> foo1(x) = begin
       r = zeros(2); r = [x[1] + x[3], x[2]^2]; return r
       end
foo1 (generic function with 1 method)

julia> foo2(x) = [x[1] + x[3], x[2]^2]
foo2 (generic function with 1 method)

julia> foo!(r, x) = begin
       r[:] .= [x[1] + x[3], x[2]^2]
       end
foo! (generic function with 1 method)

julia> let r = zeros(2)
       x = [1., 2., 3.]
       println(@allocated r = foo1(x))
       println(@allocated r = foo2(x))
       println(@allocated foo!(r, x))
       end
160
80
80

PierreMartinon commented 1 month ago

Currently I get 15 + 15 + 18 allocs for inplace vs 16 + 17 + 20 allocs for out of place. But maybe this is due to the more general type problem with the functions themselves, ie the inplace should have 0 allocs and the out of place would still have a few ?

Inplace:

    144            .          .               # 2. path constraints 
    145            .          .               # Notes on allocations:.= seems similar 
    146            .          .               if docp.dim_u_cons > 0 
    147            .          .                   if docp.is_inplace 
    148            .         15                       docp.control_constraints[2]((@view c[offset+1:offset+docp.dim_u_cons]),ti, ui, v) 
    149            .          .                   else 
    150            .          .                       c[offset+1:offset+docp.dim_u_cons] = docp.control_constraints[2](ti, ui, v) 
    151            .          .                   end 
    152            .          .               end 
    153            .          .               if docp.dim_x_cons > 0  
    154            .          .                   if docp.is_inplace 
    155            .         15                       docp.state_constraints[2]((@view c[offset+docp.dim_u_cons+1:offset+docp.dim_u_cons+docp.dim_x_cons]),ti, xi, v) 
    156            .          .                   else 
    157            .          .                       c[offset+docp.dim_u_cons+1:offset+docp.dim_u_cons+docp.dim_x_cons] = docp.state_constraints[2](ti, xi, v) 
    158            .          .                   end 
    159            .          .               end 
    160            .          .               if docp.dim_mixed_cons > 0  
    161            .          .                   if docp.is_inplace 
    162            .         18                       docp.mixed_constraints[2]((@view c[offset+docp.dim_u_cons+docp.dim_x_cons+1:offset+docp.dim_u_cons+docp.dim_x_cons+docp.dim_mixed_cons]), ti, xi, ui, v) 
    163            .          .                   else 
    164            .          .                       c[offset+docp.dim_u_cons+docp.dim_x_cons+1:offset+docp.dim_u_cons+docp.dim_x_cons+docp.dim_mixed_cons] = docp.mixed_constraints[2](ti, xi, ui, v) 
    165            .          .                   end 
    166            .          .               end

Out of place:

    144            .          .               # 2. path constraints 
    145            .          .               # Notes on allocations:.= seems similar 
    146            .          .               if docp.dim_u_cons > 0 
    147            .          .                   if docp.is_inplace 
    148            .          .                       docp.control_constraints[2]((@view c[offset+1:offset+docp.dim_u_cons]),ti, ui, v) 
    149            .          .                   else 
    150            .         16                       c[offset+1:offset+docp.dim_u_cons] = docp.control_constraints[2](ti, ui, v) 
    151            .          .                   end 
    152            .          .               end 
    153            .          .               if docp.dim_x_cons > 0  
    154            .          .                   if docp.is_inplace 
    155            .          .                       docp.state_constraints[2]((@view c[offset+docp.dim_u_cons+1:offset+docp.dim_u_cons+docp.dim_x_cons]),ti, xi, v) 
    156            .          .                   else 
    157            .         17                       c[offset+docp.dim_u_cons+1:offset+docp.dim_u_cons+docp.dim_x_cons] = docp.state_constraints[2](ti, xi, v) 
    158            .          .                   end 
    159            .          .               end 
    160            .          .               if docp.dim_mixed_cons > 0  
    161            .          .                   if docp.is_inplace 
    162            .          .                       docp.mixed_constraints[2]((@view c[offset+docp.dim_u_cons+docp.dim_x_cons+1:offset+docp.dim_u_cons+docp.dim_x_cons+docp.dim_mixed_cons]), ti, xi, ui, v) 
    163            .          .                   else 
    164            .         20                       c[offset+docp.dim_u_cons+docp.dim_x_cons+1:offset+docp.dim_u_cons+docp.dim_x_cons+docp.dim_mixed_cons] = docp.mixed_constraints[2](ti, xi, ui, v) 
    165            .          .                   end 
    166            .          .               end

Well, since it's still a bit better I guess we can move on to drop the out of place version as we discussed.

PierreMartinon commented 2 weeks ago

Hi @ocots, @jbcaillau When do we want to drop support for out-of-place OCPs ?

This means that we only accept

abstract definitions (preferred way)
functional definitions written inplace (with inplace=true) Former out-of-place functional OCPs would need to be rewritten in either form. I've done it for CTDirect.

NB. we could make inplace the default for functional OCPs also, and eventually remove out-of-place OCPs completely at some point ?

control-toolbox / CTDirect.jl

Use inplace when building the NLP model #188