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.

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

Closed
wants to merge 6 commits into from
Closed

Conversation

mateuszbaran
Copy link

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

@ChrisRackauckas
Copy link
Member

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.

@mateuszbaran
Copy link
Author

OK, I will keep the manifold there. I was also thinking about putting it in OptimizationFunction because some manifolds could be automatically converted into equality constraints for some solvers but that's probably not worth the effort.

I will add documentation and more solvers soon.

@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #437 (129aefc) into master (461d002) will increase coverage by 6.20%.
The diff coverage is n/a.

@@            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.

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.

@mateuszbaran
Copy link
Author

A small status update: I've decided to make some changes to Manopt.jl to better support this integration before finishing this PR.

@Vaibhavdixit02
Copy link
Member

Hey @mateuszbaran, curious if you think it will be a good idea to revive this now?

@mateuszbaran
Copy link
Author

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.

@kellertuer
Copy link

kellertuer commented Sep 18, 2023

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)

@Vaibhavdixit02
Copy link
Member

but we also have the chance to introduce an API here and hope it gets adapted

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.

@mateuszbaran
Copy link
Author

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.

@Vaibhavdixit02
Copy link
Member

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 😅)

@Vaibhavdixit02
Copy link
Member

Also, this doesn't necessarily need to live here, it could live in your org/repo if you want

@kellertuer
Copy link

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.

@mateuszbaran
Copy link
Author

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 .

@Vaibhavdixit02
Copy link
Member

Thanks both for the update 😄

@Vaibhavdixit02 Vaibhavdixit02 mentioned this pull request Mar 5, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants