Conversation
Codecov ReportBase: 93.57% // Head: 92.41% // Decreases project coverage by
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
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. |
|
|
What about creating |
tmigot
left a comment
tmigot
left a comment
There was a problem hiding this comment.
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.
|
If I understand correctly, the |
|
|
Should we export |
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
But perhaps others see things differently?! |
Let's discuss that too. I like the idea of traits; we're not using them enough. How do people feel? |
|
I agree with your points. But that means we change Krylov (in the future)? |
Maybe, and maybe :-). I guess Krylov and optimization solvers should be consistent. A difference is that Krylov stats don't have a |
|
Rebased and added the Adding @amontoison to the conversation. |
|
tmigot
left a comment
tmigot
left a comment
There was a problem hiding this comment.
Just two very minor comments. The rest looks good to me. We can add unit tests with dummy_solver in a separate PR.
|
Still working on the dummy solver. Trying to make it a good example to follow. |
|
|
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 |
|
Yes I guess ADNLPModel's @tmigot Could you confirm that that's due to a change in ADNLPModels? |
|
See #34. |
Yes, I can confirm that |
|
|
|
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. |
|
|
|
This is still not ideal. If the user wants to check the iteration counter, it must be done via the |
Hi guys! Why is it relevant to have |
It's not. It should be |
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 |
|
I agree. I should at least modify the dummy solver to illustrate how to do it. |
|
@tmigot I added a test to demo the callback feature and modified the dummy solver accordingly. |
|
|
This looks great, thanks! |
|
CirrusCI is apparently having some unrelated issues. I'm going to go ahead and merge. |
5 participants
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