JuliaSmoothOptimizers / NLPModelsModifiers.jl

Other
9 stars 9 forks source link

Error with SolverBenchmark #121

Open MaxenceGollier opened 1 month ago

MaxenceGollier commented 1 month ago

I am trying to benchmark solvers on a Diagonal Quasi-Newton Model. I get two issues:

  1. I get type DiagonalQNModel has no field counters
  2. When giving a Quasi-Newton Model to Ipopt, I get MethodError : no method matching hess_structure!(::NLPModelsModifiers.DiagonalQNModel{...},...)

Here is an example

using BenchmarkTools, Dates, LinearAlgebra, JLD2

using Percival, NLPModels, NLPModelsIpopt, SolverBenchmark, SolverCore, NLPModelsModifiers
using OptimizationProblems, ADNLPModels, OptimizationProblems.ADNLPProblems

meta = OptimizationProblems.meta
problem_names = meta[(meta.has_equalities_only.==1).&(meta.has_bounds.==0).&(meta.has_fixed_variables.==0).&(meta.variable_nvar.==0), :name]
problem_list = (SpectralGradientModel(eval(Meta.parse(name))(), σ=1.0) for name in problem_names)

solvers = Dict(
  :ipopt =>
    nlp -> ipopt(
      nlp,
    ),
  :percival =>
    nlp -> percival(
      nlp,
    ),
)

stats = bmark_solvers(solvers, problem_list)

There is no reason for not having a hess_structure! method at least for the DiagonalQNModel type because it is trivial. I can also add the counters as a field of the different structures if you think it is appropriate.

dpo commented 1 month ago

I probably didn’t implement counters and structure because diagonal QN operators are only used in the nonsmooth solvers at this point. You are right that they should be added.

tmigot commented 1 month ago

@MaxenceGollier thanks for the issue. For the other quasi-Newton models, this is the line that adds the counters field:

https://github.com/JuliaSmoothOptimizers/NLPModelsModifiers.jl/blob/5e6e79f01bd781ee6ad4caa4342c65a3dc753b51/src/quasi-newton.jl#L117

It is a macro that redirects the counters field to the original nlp.

I agree with @dpo it makes sense to have Hessian functions implemented for AbstractDiagonalQNModel

tmigot commented 1 month ago

@MaxenceGollier would you have some time to open two PRs for these two issues?

MaxenceGollier commented 1 month ago

@MaxenceGollier thanks for the issue. For the other quasi-Newton models, this is the line that adds the counters field:

https://github.com/JuliaSmoothOptimizers/NLPModelsModifiers.jl/blob/5e6e79f01bd781ee6ad4caa4342c65a3dc753b51/src/quasi-newton.jl#L117

It is a macro that redirects the counters field to the original nlp.

I agree with @dpo it makes sense to have Hessian functions implemented for AbstractDiagonalQNModel

I don't get it, This macro does not seem to work as if nlp is type QuasiNewtonNLP, then nlp.counters doesn't exist, what should I modify ? Just adding the field is not correct ? (as I did in #122)

MaxenceGollier commented 1 month ago

I have an issue for adding the hessian structure with the field nnzh of nlp.meta, I opened an issue in NLPModels.jl

MaxenceGollier commented 1 month ago

@MaxenceGollier thanks for the issue. For the other quasi-Newton models, this is the line that adds the counters field:

https://github.com/JuliaSmoothOptimizers/NLPModelsModifiers.jl/blob/5e6e79f01bd781ee6ad4caa4342c65a3dc753b51/src/quasi-newton.jl#L117

It is a macro that redirects the counters field to the original nlp.

I agree with @dpo it makes sense to have Hessian functions implemented for AbstractDiagonalQNModel

Actually, wouldn't this macro be more appropriate ?

macro add_counters(Model, inner)
  :($Model.counters = $Model.$inner.counters)
end

since we want to access to the field counters with AbstractDiagonalQNModel type

tmigot commented 1 month ago

Actually, the @default_counters macro does more than just accessing the inner counters see https://github.com/JuliaSmoothOptimizers/NLPModels.jl/blob/b8454fbb8bb1399c1c88b7597db88756536e501f/src/nlp/utils.jl#L119 That's why I am really trying to make it work.

tmigot commented 1 month ago

Just for reference, the meta discussion is connected to https://github.com/JuliaSmoothOptimizers/NLPModelsModifiers.jl/issues/29