abstract solver type and behavior #26

dpo · 2022-07-12T02:44:40Z

The objective of this PR is to introduce a simple AbstractSolver type from which other solvers can derive.

The end goal is to be able to perform efficient re-solves without reallocating a stats structure. If a user wants special storage for certain components of the stats structure (e.g., sparse vectors for bound constraints multipliers), they can allocate their own GenericExecutionStats and call solve!(solver, model, stats) directly.

cf: #3
related: #25
cc @sshin23

codecov · 2022-07-12T02:47:18Z

Codecov Report

Base: 93.57% // Head: 92.41% // Decreases project coverage by -1.15% ⚠️

Coverage data is based on head (a381459) compared to base (c4c047e).
Patch coverage: 60.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #26      +/-   ##
==========================================
- Coverage   93.57%   92.41%   -1.16%     
==========================================
  Files           2        3       +1     
  Lines         140      145       +5     
==========================================
+ Hits          131      134       +3     
- Misses          9       11       +2

Impacted Files	Coverage Δ
src/solver.jl	`60.00% <60.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.

github-actions · 2022-07-12T02:59:37Z

Package name	latest	stable
CaNNOLeS.jl
DCI.jl
DerivativeFreeSolvers.jl
JSOSolvers.jl
NLPModelsIpopt.jl
Percival.jl
RipQP.jl
SolverTest.jl

abelsiqueira · 2022-07-12T06:59:58Z

What about creating AbstractOptSolver as well, to differentiate what Krylov methods can derive from. Or should we leave that for traits?

tmigot

That's a good idea. I made a suggestion of a docstring, feel free to adapt. You could also make a short unit test for this using dummy_solver to increase coverage.

src/solver.jl

tmigot · 2022-07-12T07:09:23Z

If I understand correctly, the stats is not stored within the solver structure, which is what is done in Krylov.jl for instance.

github-actions · 2022-07-12T08:14:52Z

Package name	latest	stable
CaNNOLeS.jl
DCI.jl
DerivativeFreeSolvers.jl
JSOSolvers.jl
NLPModelsIpopt.jl
Percival.jl
RipQP.jl
SolverTest.jl

geoffroyleconte · 2022-07-12T13:52:47Z

Should we export AbstractSolver and solve! ?

dpo · 2022-07-12T15:54:11Z

If I understand correctly, the stats is not stored within the solver structure, which is what is done in Krylov.jl for instance.

Well, good point. We should talk about that. Krylov.jl is a bit different because there are different stats structures, so a user would be supposed to know which kind they should pass to which solver. I guess storing stats inside the solver makes that part easier for the user.

Another difference is that Krylov solvers return the solver object at the end.

From a conceptual point of view, I find it quite logical that

a solver object holds storage necessary to perform a solve;
a stats object holds storage necessary to represent the result of a solve;
solving returns a stats object.

But perhaps others see things differently?!

dpo · 2022-07-12T15:56:44Z

What about creating AbstractOptSolver as well, to differentiate what Krylov methods can derive from. Or should we leave that for traits?

Let's discuss that too. I like the idea of traits; we're not using them enough. How do people feel?

github-actions · 2022-07-12T16:13:45Z

Package name	latest	stable
CaNNOLeS.jl
DCI.jl
DerivativeFreeSolvers.jl
JSOSolvers.jl
NLPModelsIpopt.jl
Percival.jl
RipQP.jl
SolverTest.jl

abelsiqueira · 2022-07-12T18:42:48Z

Well, good point. We should talk about that. Krylov.jl is a bit different because there are different stats structures, so a user would be supposed to know which kind they should pass to which solver. I guess storing stats inside the solver makes that part easier for the user.

Another difference is that Krylov solvers return the solver object at the end.

From a conceptual point of view, I find it quite logical that

a solver object holds storage necessary to perform a solve;

a stats object holds storage necessary to represent the result of a solve;

solving returns a stats object.

But perhaps others see things differently?!

I agree with your points. But that means we change Krylov (in the future)?
About the stats being different structures, is it the case that we should allow that for Opt solvers as well? Should we have AbstractSolverOutput and Krylov outputs derived from that?

dpo · 2022-07-25T21:13:12Z

I agree with your points. But that means we change Krylov (in the future)?
About the stats being different structures, is it the case that we should allow that for Opt solvers as well? Should we have AbstractSolverOutput and Krylov outputs derived from that?

Maybe, and maybe :-). I guess Krylov and optimization solvers should be consistent. A difference is that Krylov stats don't have a solver_specific part. We should evaluate whether solver_specific makes solve!() type unstable or not.

dpo · 2022-08-06T01:34:44Z

Rebased and added the AbstractOptimizationSolver type so Krylov.jl can do things differently, at least in the near future.

Adding @amontoison to the conversation.

github-actions · 2022-08-06T01:45:57Z

Package name	latest	stable
CaNNOLeS.jl
DCISolver.jl
DerivativeFreeSolvers.jl
JSOSolvers.jl
NLPModelsIpopt.jl
Percival.jl
RipQP.jl
SolverBenchmark.jl
SolverTest.jl

tmigot

Just two very minor comments. The rest looks good to me. We can add unit tests with dummy_solver in a separate PR.

src/solver.jl

dpo · 2022-08-12T19:02:38Z

Still working on the dummy solver. Trying to make it a good example to follow.

github-actions · 2022-08-12T19:14:15Z

Package name	latest	stable
CaNNOLeS.jl
DCISolver.jl
DerivativeFreeSolvers.jl
JSOSolvers.jl
NLPModelsIpopt.jl
Percival.jl
RipQP.jl
SolverBenchmark.jl
SolverTest.jl

dpo · 2022-08-12T19:14:36Z

Something broke: https://github.com/JuliaSmoothOptimizers/SolverCore.jl/runs/7812176580?check_suite_focus=true#step:6:222. Where is that coming from?

github-actions · 2022-08-12T19:15:44Z

Package name	latest	stable
CaNNOLeS.jl
DCISolver.jl
DerivativeFreeSolvers.jl
JSOSolvers.jl
NLPModelsIpopt.jl
Percival.jl
RipQP.jl
SolverBenchmark.jl
SolverTest.jl

geoffroyleconte · 2022-08-12T19:37:58Z

Something broke: https://github.com/JuliaSmoothOptimizers/SolverCore.jl/runs/7812176580?check_suite_focus=true#step:6:222. Where is that coming from?

Could it be that some matrix that was previously dense and is now sparse? Maybe jac in dummy_solver? I've had similar warnings with the SolverBenchmark tests, but the tests still passed so I though it was normal.

dpo · 2022-08-14T03:23:34Z

Yes I guess ADNLPModel's jac now returns a sparse matrix and A' \ b doesn't work for sparse A: JuliaSparse/SparseArrays.jl#115

@tmigot Could you confirm that that's due to a change in ADNLPModels?

dpo · 2022-08-14T03:34:42Z

See #34.

tmigot · 2022-08-15T12:19:28Z

Yes I guess ADNLPModel's jac now returns a sparse matrix and A' \ b doesn't work for sparse A: JuliaSparse/SparseArrays.jl#115

@tmigot Could you confirm that that's due to a change in ADNLPModels?

Yes, I can confirm that jac is now sparse in ADNLPModels (same goes for jac_lin and jac_nln).

github-actions · 2022-09-02T04:40:21Z

Package name	latest	stable
CaNNOLeS.jl
DCISolver.jl
DerivativeFreeSolvers.jl
JSOSolvers.jl
NLPModelsIpopt.jl
Percival.jl
RipQP.jl
SolverBenchmark.jl
SolverTest.jl

github-actions · 2022-09-02T04:46:59Z

Package name	latest	stable
CaNNOLeS.jl
DCISolver.jl
DerivativeFreeSolvers.jl
JSOSolvers.jl
NLPModelsIpopt.jl
Percival.jl
RipQP.jl
SolverBenchmark.jl
SolverTest.jl

dpo · 2022-09-02T14:15:19Z

Finally got around to revising the dummy solver that's in here. If it looks ok, we could extract it and use it in the documentation.

github-actions · 2022-09-02T14:42:40Z

Package name	latest	stable
CaNNOLeS.jl
DCISolver.jl
DerivativeFreeSolvers.jl
JSOSolvers.jl
NLPModelsIpopt.jl
Percival.jl
RipQP.jl
SolverBenchmark.jl
SolverTest.jl

tmigot

Thank you! This looks great.

test/test_stats.jl

src/solver.jl

test/dummy_solver.jl

github-actions · 2022-09-02T23:35:54Z

Package name	latest	stable
CaNNOLeS.jl
DCISolver.jl
DerivativeFreeSolvers.jl
JSOSolvers.jl
NLPModelsIpopt.jl
Percival.jl
RipQP.jl
SolverBenchmark.jl
SolverTest.jl

dpo · 2022-09-06T20:17:06Z

This is still not ideal. If the user wants to check the iteration counter, it must be done via the stats. We have to decide whether the contents of stats can be trusted in the callback (e.g., what about the multipliers? Or the solver-specific data?)

amontoison · 2022-09-06T21:46:08Z

Rebased and added the AbstractOptimizationSolver type so Krylov.jl can do things differently, at least in the near future.

Adding @amontoison to the conversation.

Hi guys! Why is it relevant to have KrylovSolver <: AbstractOptimizationSolver?

dpo · 2022-09-06T23:54:38Z

Hi guys! Why is it relevant to have KrylovSolver <: AbstractOptimizationSolver?

It's not. It should be KrylovSolver <: Solver, so they can behave differently than optimization solvers.

tmigot · 2022-09-07T04:11:22Z

This is still not ideal. If the user wants to check the iteration counter, it must be done via the stats. We have to decide whether the contents of stats can be trusted in the callback (e.g., what about the multipliers? Or the solver-specific data?)

Is this connected with this precise PR?

I think it should be sufficient to be precise in the docstring what fields of the stats are updated by algorithm? And add a reset!(stats) at the beginning of each algorithm.

dpo · 2022-09-07T18:51:22Z

I agree. I should at least modify the dummy solver to illustrate how to do it.

dpo · 2022-09-08T22:17:46Z

@tmigot I added a test to demo the callback feature and modified the dummy solver accordingly.

github-actions · 2022-09-08T22:36:00Z

Package name	latest	stable
CaNNOLeS.jl
DCISolver.jl
DerivativeFreeSolvers.jl
FletcherPenaltySolver.jl
JSOSolvers.jl
NLPModelsIpopt.jl
Percival.jl
RipQP.jl
SolverBenchmark.jl
SolverTest.jl

tmigot · 2022-09-09T12:05:29Z

This looks great, thanks!

dpo · 2022-09-09T14:15:51Z

CirrusCI is apparently having some unrelated issues. I'm going to go ahead and merge.

dpo requested review from abelsiqueira, tmigot and geoffroyleconte July 12, 2022 02:44

tmigot reviewed Jul 12, 2022

View reviewed changes

src/solver.jl Show resolved Hide resolved

sshin23 mentioned this pull request Jul 12, 2022

[Breaking change] Change names to follow JSO conventions MadNLP/MadNLP.jl#194

Merged

abelsiqueira mentioned this pull request Jul 27, 2022

Evaluate if solver_specific makes solve! type unstable #30

Closed

dpo force-pushed the abstract-solver branch from d6d21df to a8d9bd8 Compare August 6, 2022 01:34

tmigot reviewed Aug 12, 2022

View reviewed changes

src/solver.jl Outdated Show resolved Hide resolved

src/solver.jl Show resolved Hide resolved

tmigot mentioned this pull request Aug 15, 2022

Move dummy_solver to src #36

Closed

dpo force-pushed the abstract-solver branch from 02b2ee2 to 7add2a0 Compare September 2, 2022 04:36

dpo force-pushed the abstract-solver branch from 7add2a0 to 964846a Compare September 2, 2022 14:34

dpo requested review from tmigot and removed request for abelsiqueira September 2, 2022 18:18

tmigot approved these changes Sep 2, 2022

View reviewed changes

test/test_stats.jl Outdated Show resolved Hide resolved

test/test_stats.jl Show resolved Hide resolved

tmigot reviewed Sep 2, 2022

View reviewed changes

src/solver.jl Outdated Show resolved Hide resolved

tmigot reviewed Sep 2, 2022

View reviewed changes

src/solver.jl Outdated Show resolved Hide resolved

tmigot reviewed Sep 2, 2022

View reviewed changes

test/dummy_solver.jl Outdated Show resolved Hide resolved

dpo added 2 commits September 2, 2022 19:26

abstract solver type and behavior

a8af8a4

convert dummy_solver

389d83d

dpo force-pushed the abstract-solver branch from 964846a to 389d83d Compare September 2, 2022 23:26

geoffroyleconte approved these changes Sep 6, 2022

View reviewed changes

dpo mentioned this pull request Sep 8, 2022

update deps, stats API JuliaSmoothOptimizers/RegularizedOptimization.jl#76

Merged

demo and test callback

a381459

dpo merged commit a394b3a into main Sep 9, 2022

dpo deleted the abstract-solver branch September 9, 2022 14:16

abstract solver type and behavior #26

abstract solver type and behavior #26

Conversation

dpo commented Jul 12, 2022

codecov bot commented Jul 12, 2022 • edited Loading

Codecov Report

github-actions bot commented Jul 12, 2022

abelsiqueira commented Jul 12, 2022

tmigot left a comment

Choose a reason for hiding this comment

tmigot commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

geoffroyleconte commented Jul 12, 2022

dpo commented Jul 12, 2022

dpo commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

abelsiqueira commented Jul 12, 2022

dpo commented Jul 25, 2022

dpo commented Aug 6, 2022

github-actions bot commented Aug 6, 2022

tmigot left a comment

Choose a reason for hiding this comment

dpo commented Aug 12, 2022

github-actions bot commented Aug 12, 2022

dpo commented Aug 12, 2022

github-actions bot commented Aug 12, 2022

geoffroyleconte commented Aug 12, 2022 • edited Loading

dpo commented Aug 14, 2022

dpo commented Aug 14, 2022

tmigot commented Aug 15, 2022

github-actions bot commented Sep 2, 2022

github-actions bot commented Sep 2, 2022

dpo commented Sep 2, 2022

github-actions bot commented Sep 2, 2022

tmigot left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 2, 2022

dpo commented Sep 6, 2022

amontoison commented Sep 6, 2022

dpo commented Sep 6, 2022

tmigot commented Sep 7, 2022

dpo commented Sep 7, 2022

dpo commented Sep 8, 2022

github-actions bot commented Sep 8, 2022

tmigot commented Sep 9, 2022

dpo commented Sep 9, 2022

codecov bot commented Jul 12, 2022 •

edited

Loading

geoffroyleconte commented Aug 12, 2022 •

edited

Loading