-
-
Notifications
You must be signed in to change notification settings - Fork 83
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.
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
Manopt.jl integration #437
Conversation
That looks like the best place to put it, since it's algorithm specific (the other optimizers won't support the argument). This is missing docs though: each of the solver sets has a doc page. Should also be added to the table in the front of the docs. |
OK, I will keep the manifold there. I was also thinking about putting it in I will add documentation and more solvers soon. |
Codecov Report
@@ Coverage Diff @@
## master #437 +/- ##
=========================================
+ Coverage 2.85% 9.06% +6.20%
=========================================
Files 40 40
Lines 2694 2704 +10
=========================================
+ Hits 77 245 +168
+ Misses 2617 2459 -158 see 7 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
||
Note that currently `AutoForwardDiff` can't correctly compute the required Riemannian gradient for optimization. Riemannian optimizers require Riemannian gradients while `ForwardDiff.jl` returns normal Euclidean ones. Conversion from Euclidean to Riemannian gradients can be performed using the [`project`](https://juliamanifolds.github.io/ManifoldsBase.jl/stable/projections.html#Projections) function and (for certain manifolds) [`change_representer`](https://juliamanifolds.github.io/Manifolds.jl/stable/manifolds/metric.html#Manifolds.change_representer-Tuple{AbstractManifold,%20AbstractMetric,%20Any,%20Any}). | ||
|
||
Note that the constraint is correctly preserved and the convergence is quite fast. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...actually ( ;) ) the convergence is the same local quadratic convergence as for the Euclidean case.
A small status update: I've decided to make some changes to Manopt.jl to better support this integration before finishing this PR. |
Hey @mateuszbaran, curious if you think it will be a good idea to revive this now? |
Hi! I've stopped working on this PR mainly because there there was some hope to have a more centralized way to specify manifold constraints and their API among nonlinear solvers (instead of each one doing its own thing) but so far no one seems to care enough to make a breaking release. It would make it easier for Optimization.jl because it wouldn't have to care about translating manifold constraints for each solver. Maybe that's the way to handle this, or maybe it's just too niche for Optimization.jl. There is also currently some ongoing work on Manopt.jl-JuMP.jl integration (JuliaManifolds/Manopt.jl#264). The interface isn't quite finalized yet though. |
I think it might be quite niche, but we also have the chance to introduce an API here and hope it gets adapted? (edit: But I am also super-biased as I am working on Manopt.jl and Manifolds.jl so for me this is not so niche) |
I am completely onboard with this if you want to move ahead. Might even turn out to be a good thing, since the point of Optimization.jl has been to provide unified APIs for a variety of solvers and problem types, defining an API here and then asking people to adopt it might be an easier route and very much inline with the aim behind this package. |
OK, sure, I can go back to this integration if you think this is a good idea 🙂 . I'm currently in the middle of making some important changes to Manifolds.jl (JuliaManifolds/Manifolds.jl#642), so I will continue here after that change is finished. |
Hey @mateuszbaran gentle bump, would love to see this go in. Do you have an estimate of how much more work is left here? Let me know if you want me to handle some parts (it's on a branch of your fork so can't do much right now 😅) |
Also, this doesn't necessarily need to live here, it could live in your org/repo if you want |
Oh this will happen for sure. Just that people also have these jobs that earn money and sometimes this means that open source takes a step back (including myself the last few days, otherwise would step in). No worries, this is still planned and not forgotten - but in between also required a few fixes in Manopt.jl. |
Hi! I'd also love to finish this PR but other, more urgent work keeps going my way. There are different levels of completion this PR could achieve, with the most ambitious being support of Riemannian and mixed Riemannian+nonlinear constraints for all solvers, with automatic conversion of Riemannian constraints to nonlinear ones. This is still quite far away and realistically won't have the time to finish it before summer 2024, at the earliest. The next piece of functionality this requires is conversion of manifold constraints to equality constraints (for level set manifolds), which I'm going to put in JuliaManifolds. Some manifolds are defined using inequality constraints but with the notable exception of SPD matrices, this seems less important to me and could be left as not implemented, at least initially. This PR could be merged in a more limited state but I'm afraid it would just lead to poor user experience. Thank you for the offer to help but I think I will need it later, when the conversion to equality constraints is ready and it would just have to be plugged into Optimization.jl. Also somewhat relevant is that JuMP/Manopt.jl integration has recently been merged: JuliaManifolds/Manopt.jl#264 . |
Thanks both for the update 😄 |
Hi! I've started working on integration with Manopt.jl. I will add more solvers but first I have a design question: where should I put the manifold constraint specification? I've put them in optimizer for now because it was easiest but Optimization.jl already has some constraints in optimization function and optimization problem so adding a third place for constraints might not be ideal.
cc @kellertuer