Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

The Riemannian Interior Point Newton Method #399

Merged
merged 267 commits into from
Aug 2, 2024
Merged

The Riemannian Interior Point Newton Method #399

merged 267 commits into from
Aug 2, 2024

Conversation

kellertuer
Copy link
Member

@kellertuer kellertuer commented Jun 28, 2024 •
edited
Loading

This is my (third) summer project: This is initial code for a Riemannian Interior Point Newton method based on the master thesis of @mstokkenes, where of course documentation and testing are not part of master theses, but exploring how this algorithms works and illustrating that is works.
A Matlab code for the same idea this is based on (and from the authors of the original paper) is https://github.com/GALVINLAI/RIPM, though here the notation is already much closer to that of Manopt.jl

🛣️ Roadmap

  • 🛠️ refactor code to VectorialHessianFunctions, which are newer than the original code base
  • 🛠️ Refactor is_feasible to work in a more general sense than just for RIPM and (re-)introduce DebugFeasibility()
  • 🛠️ rework KKT vector field, Jacobian and the merit function (incl. documentation)
  • 🛠️ store the merit function and its gradient as the stepsize_objective and for the additional check use centrality_condition
  • 🛠️ debug and refactor Conjugate residual
  • 🔎 carefully check the condensed KKT system
  • 🔎 carefully debug the parameters ρ and σ (Section 8.1 YL) and their update
  • ⚠️ for now the IPNewton performs one or two steps very well, then the gradient seems not be a descent direction (but I am also not yet 100% through with documentation and testing)
  • 📚extend all new documentation strings
  • fix Refine storage of subsolver states #403 on all solvers that have a subsolver.
  • 📈 ensure proper test coverage

mstokkenes and others added 30 commits April 20, 2024 18:56
@mstokkenes
Reworked structure of code
5c7e985 
Now more similar to rest of Manopt
@mstokkenes
Implemented conjugate residual method as abstract solver
820e7dc 
step_solver! for interior point Newton now employs conjugate_gradient!, and interior_point_Newton! converges unless first iterate is not feasible
@kellertuer
add one comment to the new subsolver.
c6726f3
@mstokkenes
Update conjugate_residual.jl
8557459
@mstokkenes
Modified step_solver! for conjugate residual method
51d04a9 
Also changed AbstractManoptProblem inputs to AbstractManoptProblem{<:TangentSpace}
@mstokkenes
Test commit
4269640
@mstokkenes
Combined LHS and RHS function into one function get_Newtons
2007d63 
Also added possibility for plotting results for visualization purposes. Added  comments throughout code.
@mstokkenes
Minor formatting improvements
b4a1270
@kellertuer
Add a few comments from todays meeting.
63a4be9
@mstokkenes
Added subsolver flexibility
4a3adeb
@kellertuer
Add a few comments to discuss tomorrow.
6b396e5
@kellertuer
add a few more comments.
937a907
@kellertuer
A bit of a rework and some comments.
8832bd1
@kellertuer
Add a new idea to the rework to simplify things, that are then less h…
bafac3e 
…irarchical maybe (at least seen from the main solver).
@kellertuer
Start structural rework on constraint objectives.
56eb725
@kellertuer
Sketch the idea of a unified vector function objective.
147cf4e
@mstokkenes
Updating subproblem now works more generically
f56727d
@kellertuer
Rename the VectorialObjective to a VectorGradientFunction and start i…
bde4510 
…mplementing accessors.
@kellertuer
Reduce duplicate text.
40235f4
@kellertuer
Finish most of the desugn and basic documentation work.
c060302
@kellertuer @mateuszbaran
Apply suggestions from code review
88d81f1 
Co-authored-by: Mateusz Baran <mateuszbaran89@gmail.com>
@kellertuer
Merge branch 'master' into kellertuer/structural_rework_constraints
6862c92
@kellertuer
sketch all variants of the (full) access get_gradients and get_gradie…
bd3fec3 
…nts!
@mstokkenes
Made sure code runs before meeting :)
8dcd677
@mateuszbaran
Merge branch 'master' into kellertuer/structural_rework_constraints
879e94a
@mateuszbaran
minor fixes
4b6eac7
@mateuszbaran @github-actions
Update src/plans/constrained_plan.jl
7826cd4 
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@kellertuer
Fix a few typos and improve code a bit.
f4cc60a
@kellertuer
Improve test a bit.
a9d15b6
@kellertuer
Get arguments to full generality.
8c5f0f6
kellertuer and others added 7 commits August 1, 2024 20:01
@kellertuer
Adapt CBM.
2d06668 
its a bit like knitting – one can nearly do that while watching a documentation.
@kellertuer
Just missed Aqua.
d89a75a
@kellertuer
Adapt PBM.
9b963bd
@mateuszbaran
adapt DC
63ee28f
@kellertuer
Adapt ARC.
d9961ab
@kellertuer
Merge branch 'kellertuer/riemannian-interior-point' of github.com:Jul…
b333d6f 
…iaManifolds/Manopt.jl into kellertuer/riemannian-interior-point

# Conflicts:
#	src/solvers/difference_of_convex_algorithm.jl
@kellertuer
fix Aqua DC.
ab0bb15
@kellertuer
Copy link
Member Author

Oh TR looks a bit more involved, the code is quite old and might also benefit from a bit of improvements anyways. Do you want to do that or shall I?

@mateuszbaran
Copy link
Member

I can adapt TR.

@kellertuer
Copy link
Member Author

We have all others and proper code cov.

@kellertuer
Copy link
Member Author

Oh, I introduced an ambiguity? Why did that not happen for the other solvers? I do not directly se how to resolve that.

@mateuszbaran
Copy link
Member

I've adapted TR and resolved the ambiguity.

@kellertuer
Copy link
Member Author

I am not so sure where we lost the 2 lines on TR, but the rest seems to be fine.

@kellertuer
Copy link
Member Author

Yay! I'd call this finished, but would be happy if you could approve it as well.
We can also wait until tomorrow with merging.

@mateuszbaran
Copy link
Member

I would like to take one more look at this PR tomorrow, mostly to check if there are any major performance issues or some small documentation improvements.

@kellertuer
Copy link
Member Author

Sure, If you find something and do a commit, you can also adapt the date in the changelog.
I can check back here tomorrow afternoon then, since I will be on a hike tomorrow (one we can also do when you are here).

mateuszbaran
mateuszbaran approved these changes Aug 2, 2024
View reviewed changes
Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance is not great but to improve that I need to extend PowerManifold a bit first. I think this would be better suited for a separate PR.

mateuszbaran
mateuszbaran reviewed Aug 2, 2024
View reviewed changes
Changelog.md Outdated Show resolved Hide resolved
@kellertuer
Copy link
Member Author

Performance is not great but to improve that I need to extend PowerManifold a bit first. I think this would be better suited for a separate PR.

I think s well, this took quite a while to get done, so working is my first goal here, performance a next one.
I did work through most of the access functions used for constraints and used the single access ones for performance for example.

@kellertuer kellertuer merged commit 0b05c8c into master Aug 2, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateuszbaran mateuszbaran mateuszbaran approved these changes

@github-actions github-actions[bot] github-actions[bot] left review comments

Assignees

@kellertuer kellertuer

Labels
documentation Ready-for-Review A label for pull requests that are feature-ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refine storage of subsolver states
3 participants
@kellertuer @mateuszbaran @mstokkenes