Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
paraynaud opened this issue Sep 1, 2022 · 7 comments
Open

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

paraynaud opened this issue Sep 1, 2022 · 7 comments
Assignees

Comments

@paraynaud
Copy link
Member

paraynaud commented Sep 1, 2022

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)?

function TrunkSolver(

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.

@dpo
Copy link
Member

dpo commented Sep 1, 2022

You're right. We should be using similar.

@amontoison amontoison changed the title Replace V(undef, nvar) by similar(nlp.meta.x0 in TrunkSolver? Replace V(undef, nvar) by similar(nlp.meta.x0) in TrunkSolver? Sep 15, 2022
@tmigot
Copy link
Member

tmigot commented Sep 21, 2022

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

@paraynaud
Copy link
Member Author

paraynaud commented Sep 21, 2022

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).

@amontoison
Copy link
Member

amontoison commented Sep 21, 2022

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

@tmigot
Copy link
Member

tmigot commented Sep 21, 2022

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

@dpo
Copy link
Member

dpo commented Sep 22, 2022

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

@amontoison
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants