update solvers to follow ProximalAlgorithms #19
Conversation
There was a problem hiding this comment.
I think it is always good to simplify stuff! I also think solve! is not really needed and it was getting things overcomplicated. The code looks good to me, I assume we just need to merge and tag ProximalAlgorithms, check if tests are passing here and then merge&tag. Demos (#14) and benchmarks (#15 ) have been outdated for a while so I wouldn't worry too much for breaking changes!
|
I think we can take the opportunity of getting rid of REQUIRE and update to new dependencies system. |
Codecov Report
@@ Coverage Diff @@
## master #19 +/- ##
==========================================
- Coverage 92.19% 91.52% -0.68%
==========================================
Files 17 17
Lines 423 413 -10
==========================================
- Hits 390 378 -12
- Misses 33 35 +2
Continue to review full report at Codecov.
|
3 participants
This PR hands over the solution method ("solver") completely to ProximalAlgorithms, following the proposal in JuliaFirstOrder/ProximalAlgorithms.jl#22
Benefits, roughly: simpler code (more deletions than additions), no need to duplicate concepts, no need to duplicate documentation, better decoupling and separation of concerns.
NOTE: this PR is breaking in that it removes the
solve!function. When going through the code I realized that this was a bit confusing, also in the documentation.solve!is meant to essentially allow, as far as I recall, pre-allocating all the buffers and so on for the solver, and save on allocations for example when sequences of problems need to be solved. I'm not sure how crucial it is to have this in a high-level modeling package like this?Tests and docs are updated as well.