JSOSolvers.jl icon indicating copy to clipboard operation
JSOSolvers.jl copied to clipboard

Replace `V(undef, nvar)` by `similar(nlp.meta.x0)` in `TrunkSolver`?

Open paraynaud opened this issue 2 years ago • 7 comments

Hey guys (@dpo @tmigot), I was wondering why the AbstractVector required by TrunkSolver are defined with V(undef, nvar) and not with similar(nlp.meta.x0)?

https://github.com/JuliaSmoothOptimizers/JSOSolvers.jl/blob/15550bdd2282c7fb1d7a5fcd78078ac271385b57/src/trunk.jl#L84

I'm currently working on PartitionedStructures.jl and PartiallySeparableNLPModels.jl to make solvers (starting with trunk) exploit fully the partially separable structure. I'm defining PartitionedVector<:AbstractVector in PartitionedStructure.jl, and my plan is to set nlp.meta.x0 as a partitioned vector. This way, any object similar to x0 keeps the same partitioned structure. In addition, I will overload all operations required by trunk() (and cg()) on PartitionedVectors.

Defining vectors with V(undef, nvar) prevents any use of the partitioned structure.

paraynaud avatar Sep 01 '22 14:09 paraynaud

You're right. We should be using similar.

dpo avatar Sep 01 '22 15:09 dpo

Hi @paraynaud ! Could you open a PR to fix this? We now have a solver structure for all the solvers, so this issue might appear not only in TrunkSolver

tmigot avatar Sep 21 '22 21:09 tmigot

I spoke with @amontoison since the same problem appears in KrylovSolvers, the way AbstractVectors are defined (with undef) seems to be the good way to do it. What I understood from Alexis, using similar in place of S(undef,nvar) will downgrade (a lot) the performances of cg if it is called with SparseVector (or an AbstractVector which is not a DenseVector).

If everyone is okay with the similar replacement I can open a PR.

Anyway, I will have to define my own TrunkSolver constructor dedicated to PartiallySeparableNLPModels, since the implementation of PartitionedVectors has become quite complex (similar is not enough unfortunately).

paraynaud avatar Sep 21 '22 22:09 paraynaud

I don't use similar in Krylov.jl because the type of the right-hand side b is not always the most appropriate to store the Krylov bases. Based on the type of b, I determine the most appropriate dense storage (S) for Krylov bases with the function ktypeof.
https://github.com/JuliaSmoothOptimizers/Krylov.jl/blob/main/src/krylov_utils.jl#L221-L248

amontoison avatar Sep 21 '22 22:09 amontoison

Looks good to me, I checked and only R2 uses similar right now.

tmigot avatar Sep 21 '22 23:09 tmigot

Should we extract the Krylov mechanism out of Krylov so it can be used here too?

dpo avatar Sep 22 '22 00:09 dpo

It can be used here already :) You just need to import Krylov.ktypeof and call it.

amontoison avatar Sep 22 '22 04:09 amontoison