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

[WIP] Create specific operator for hprod #423

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

abelsiqueira
Copy link
Member

@abelsiqueira abelsiqueira commented Sep 23, 2022

First jab at avoiding the implicit change of hess_op is up for discussion.

To use it on a code like Trunk, instead of writing H = hess_op!(nlp, x, Hv), we will just write H.x .= x. We can probably wrap that under update!(H, x), but it does not seem necessary.

Issues:

  • Verbose
  • There is currently no way to pass an existing x to create an HprodOperator, but that could be changed, if we want to. The API is becoming too convoluted, though.
  • The hess_op! with rows, cols, vals will still suffer from the same implicit issue, unless we also create a HprodOperator for it.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 23, 2022

Package name latest stable
ADNLPModels.jl
AmplNLReader.jl
CUTEst.jl
CaNNOLeS.jl
DCISolver.jl
FletcherPenaltySolver.jl
JSOSolvers.jl
LLSModels.jl
NLPModelsIpopt.jl
NLPModelsJuMP.jl
NLPModelsModifiers.jl
NLPModelsTest.jl
PDENLPModels.jl
Percival.jl
QuadraticModels.jl
SolverBenchmark.jl
SolverCore.jl
SolverTest.jl
SolverTools.jl

@tmigot
Copy link
Member

tmigot commented Sep 25, 2022

I don't like the H.x .= x since for the following type of operator that wouldn't do anything:

function hess_op!(
  nlp::GridapPDENLPModel,
  x::AbstractVector,
  Hv::AbstractVector;
  obj_weight::Real = one(eltype(x)),
)
  @lencheck nlp.meta.nvar x Hv
  rows = nlp.pdemeta.Hrows
  cols = nlp.pdemeta.Hcols
  vals = hess_coord!(nlp, x, nlp.workspace.Hvals, obj_weight = obj_weight)
  return hess_op!(nlp, rows, cols, vals, Hv)
end

@dpo
Copy link
Member

dpo commented Sep 25, 2022

This is looking good, thanks!

The hess_op! with rows, cols, vals will still suffer from the same implicit issue, unless we also create a HprodOperator for it.

Could it be another constructor?

@tmigot
Copy link
Member

tmigot commented Sep 25, 2022

Ok, so if I understand. Whenever we reimplemented hess_op!, we would now have to specialize HprodOperator!(nlp, x, Hv) and update!(::ModelOperator, x). Is that right?

If we want to reuse the structure for another nlp of the same size, we would also need a update!(::ModelOperator, nlp) and a way to broadcast nlp assuming they respect some conditions.

@abelsiqueira
Copy link
Member Author

Some comments on the comments

I don't like the H.x .= x since for the following type of operator that wouldn't do anything:
...
Ok, so if I understand. Whenever we reimplemented hess_op!, we would now have to specialize HprodOperator!(nlp, x, Hv) and update!(::ModelOperator, x). Is that right?

Yes, I think it will be necessary, but only models that feel the need to update hess_op! itself. They would, instead, update HprodOperator, so the additional work is the same (well, almost the same).

If we want to reuse the structure for another nlp of the same size, we would also need a update!(::ModelOperator, nlp) and a way to broadcast nlp assuming they respect some conditions.

Maybe we need a update!(::Solver, nlp). Consider the following:

solver = TrunkSolver(nlp) # solver.H exists
nlp = Problem1()
output = solve!(solver, nlp)
# do things
nlp = Problem2() 
### Option 1: Update solver.H ???
### Option 2: Update solver
output = solve!(solver, nlp)

Option 1 requires specific knowledge of the solver. Option 2 can be a no-op for many solvers. Also, we can accept keyword arguments to update other solver aspects, like subsolver.

@dpo

The hess_op! with rows, cols, vals will still suffer from the same implicit issue, unless we also create a HprodOperator for it.
Could it be another constructor?

Not like this, because ModelOperator stores x. We would need to store rows, cols, vals, I think.
Do we have any use cases for this already? I think our solvers using rows, cols, vals are all factorization.

@codecov
Copy link

codecov bot commented Oct 4, 2022

Codecov Report

Base: 99.50% // Head: 98.66% // Decreases project coverage by -0.83% ⚠️

Coverage data is based on head (7ceda76) compared to base (d6c1a43).
Patch coverage: 75.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #423      +/-   ##
==========================================
- Coverage   99.50%   98.66%   -0.84%     
==========================================
  Files          13       14       +1     
  Lines         801      823      +22     
==========================================
+ Hits          797      812      +15     
- Misses          4       11       +7     
Impacted Files Coverage Δ
src/NLPModels.jl 100.00% <ø> (ø)
src/nlp/operator.jl 74.07% <74.07%> (ø)
src/nlp/api.jl 99.75% <100.00%> (-0.01%) ⬇️
src/nlp/meta.jl 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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

Successfully merging this pull request may close these issues.

3 participants