Make the trust region subsolver exchangeable. #294
Conversation
kellertuer
changed the title
Codecov Report
@@ Coverage Diff @@
## master #294 +/- ##
==========================================
- Coverage 99.74% 99.73% -0.01%
==========================================
Files 76 78 +2
Lines 7090 7206 +116
==========================================
+ Hits 7072 7187 +115
- Misses 18 19 +1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
While this rework per se is done (besides the idea above to add a second subsolver), we still miss a edit: The docs could probably be shortened a bit, the TR docs currently describe a lot of things where we usually referred to the corresponding papers. I will update that the next days. |
# Conflicts: # docs/src/solvers/truncated_conjugate_gradient_descent.md # src/data/artificialDataFunctions.jl # src/solvers/trust_regions.jl # test/solvers/test_trust_regions.jl
# Conflicts: # docs/src/solvers/DouglasRachford.md
…st region sub objective.
but we might really first need 0.15 registered and Manifolds.jl and ManifoldDiff.jl bumped on dependencies before continuing
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
# Conflicts: # Project.toml # docs/Project.toml # docs/src/solvers/ChambollePock.md # ext/ManoptManifoldsExt/ARC_CG.jl # src/data/artificialDataFunctions.jl
but there is still some work to do, to make the new state constructors nice.
|
This still needs a bit of rework, for best of cases also for the other states with sub solvers to be unified in interface and even easy to use with default sub solvers. |
|
Hm, in reworking Lanczos / The sub solvers Objective I seem to have introduced a bug that is really hard to narrow down where that comes from. edit: What a side effect, a stopping criterion modified the state, which they never should do. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
@blegat since we discussed a possible tutorial about methods required for a solver... I did not yet write such a tutorial here, but since I was changing a few things in the docs anyways, I wrote a short section https://manoptjl.org/previews/PR294/solvers/gradient_descent.html#Technical-Details that we could add to every solver, mentioning, which functions are required. Would that help for the use case you described in JuliaManifolds/Manifolds.jl#658? |
kellertuer
added
the
Ready-for-Review
|
Everything is working now, I might have to check for a missing line in coverage and whether that is fixable, but the rest is done and good to go – the TR subsolver is fully replaceable. |
Yes, I think that is a very valuable addition |
|
Cool. I can add more on a next PR, when I write the tutorial then – that might also help further to read those technical details. |
This should resolve #240 and unifies notation as well.
To keep track:
ηthe subsolver iterate is now calledYη_Cauchythe Cauchy point to check for the randomised variant is now calledZrandomwill not only be true/false but will allow to set the amount of randomnessThis is still an early stage, but I wanted to keep track here. Especially, setting initial values and checking the radius afterwards are still quite intertwined.