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

Integration into Optim #35

Open
antoine-levitt opened this issue Sep 11, 2019 · 25 comments
Open

Integration into Optim #35

antoine-levitt opened this issue Sep 11, 2019 · 25 comments
Labels
question Further information is requested

Comments

@antoine-levitt
Copy link
Collaborator

Hi,

I just checked back, it seems there has been lots of activity on this project, great! Is it ready for prime time? In particular, can we replace Optim's implementation of manifolds with this package? We can think about a deeper integration sometime in the future, but right now I think just supporting manifolds embedded in R^n using retraction and projection of a vector onto the tangent space for the Stiefel and sphere manifolds (the basic things implemented in https://github.com/JuliaNLSolvers/Optim.jl/blob/master/src/Manifolds.jl) with a stable-ish API will be enough.

@pkofod: or should we wait until the next-gen optim?

@mateuszbaran
Copy link
Member

Thanks for asking!

I think Manifolds.jl isn't quite ready yet. A few things we need to consider before using it in things like Optim.jl:

  1. Stiefel manifold isn't done yet (but it shouldn't be problematic), also the power manifold is a work in progress. I want to do it right (that's why I'm working on hybrid dynamic-static arrays in this branch: https://github.com/JuliaNLSolvers/Manifolds.jl/tree/mbaran/power-manifold , I'll put them in a separate package soon).
  2. Documentation isn't deployed yet and we need a better README.
  3. Manifolds.jl, at least in its current shape, is a pretty heavy dependency for something as simple as projection and retraction. A large part of this is caused by manifold-valued distributions (which could be moved into a separate package) and the metric manifold.
  4. Our API for basic operations (including retraction) looks pretty stable. Projection onto the tangent space might receive a new argument argument that indicates the method to be used (like for retractions) but API should be backward-compatible with the current one.

I think issues 1-3 should be solved before Manifolds.jl can be safely used in Optim.jl (or the new Optim). I need a few weeks to solve (1). Issue (3) needs some discussion, I'm not sure how important a low number of dependencies is for you.

@pkofod
Copy link
Contributor

pkofod commented Sep 11, 2019

@pkofod: or should we wait until the next-gen optim?

I think it's fine to try it out in a system we know :)

@pkofod
Copy link
Contributor

pkofod commented Sep 11, 2019

I'm not sure how important a low number of dependencies is for you.

Not important if it's "our own"

@kellertuer
Copy link
Member

Thanks for this topic, I was not able to continue working the last month (sadly), I will continue to work on the SPDs hopefully next week – but I am on @mateuszbaran s side considering the basic API, but I would love to have at least a proper, complete documentation as well as maybe a few more manifolds before integrating somewhere.

@pkofod
Copy link
Contributor

pkofod commented Sep 19, 2019

I think issues 1-3 should be solved before Manifolds.jl can be safely used in Optim.jl (or the new Optim). I need a few weeks to solve (1). Issue (3) needs some discussion, I'm not sure how important a low number of dependencies is for you.

I've been reading the code in this package and thinking about using it in Optim going forwards. I'm super impressed, and the work seems super thorough. I still don't see an immediate need to split this package into separate packages; if you're opting in to one manifold, let's just provide all the batteries you could ever have thought of! But, and let me just undermine my own statement one sentence later, I think we may want to create just one additional package that is the "ManifoldsBase.jl".

What is ManifoldsBase.jl? ManifoldsBase.jl has all the stubs, the generic interface, and so on. And quite importantly, it also has the Euclidian manifold. I am saying this because I obviously want numerical analysis packages to take advantage of all this good work, but I don't think many packages will want to include Manifolds in their Project.jl file, because it is indeed a bit heavy. How long does it take to load for you guys? It's 16 seconds on my windows machine, haven't tested on linux. Is that OrdinaryDiffEq? Anyway, Optim et al should just be allowed to include the Euclidian manifold and the full API, and then people can implement their own manifolds or using Manifolds to get the full suite. Optim cannot take on a dependency that takes 16 seconds to load, especially because Optim itself is a dependency of many packages in the ecosystem, and few of those packages use manifods, so it would be a shame to push 10 seconds of addition using times onto them :)

@antoine-levitt
Copy link
Collaborator Author

Possibly another option would be to load ODE lazily if required?

Also speaking of that, something that came up with @jagot is that he wanted to do a crazy manifold where he keeps some columns of a matrix orthogonal and some others normalized, the splitting being non-contiguous. What that illustrates is that it's important that users be able to define a manifold with just the two operations Optim needs, and not necessarily all the operations supported by this package.

@mateuszbaran
Copy link
Member

Thanks, we are trying to make this package as good as we can 🙂 .

On my computer it takes 24 seconds to load Manifolds, that's way too much for a dependency. OrdinaryDiffEq takes 18 seconds. What ManifoldsBase.jl would have to include without redesign of our interface:

  • Distributions: 1.3-2 seconds (which could perhaps be conditionally loaded using Requires?)
  • SimpleTraits: about 0.8 seconds (although only a small part of the interface relies on this),
  • and a few standard things like LinearAlgebra

This should take the loading time down to perhaps 3 seconds, maybe a bit less. That's about as much as using Optim takes for me. Do you think it'd be OK to require SimpleTraits unconditionally?

What that illustrates is that it's important that users be able to define a manifold with just the two operations Optim needs, and not necessarily all the operations supported by this package.

Yes, no one has to implement the full interface if they don't need it. Even many of our manifolds don't implement everything we have in our generic interface tests. Extending our functions would be totally fine IMHO.

@pkofod
Copy link
Contributor

pkofod commented Sep 20, 2019

Why is Distributions needed?

edit: let me clarify, I'm not in doubt about what it's used for, I'm just curious why it would be required in defining the stubs.

@mateuszbaran
Copy link
Member

We extend their VariateForm and ValueSupport as well as derive from Distribution: https://github.com/JuliaNLSolvers/Manifolds.jl/blob/master/src/DistributionsBase.jl .

@pkofod
Copy link
Contributor

pkofod commented Sep 20, 2019

Okay, so it is technically possible to just only support sampling if using Manifolds, right?

@mateuszbaran
Copy link
Member

Right, that sounds reasonable. I'll add separating ManifoldsBase.jl to my TODO list 🙂 .

@sethaxen
Copy link
Member

I also think ManifoldsBase.jl is a good idea and should definitely include Euclidean.

I think issues 1-3 should be solved before Manifolds.jl can be safely used in Optim.jl (or the new Optim). I need a few weeks to solve (1).

Unfortunately, I've not had the time to continue work on #25 and probably won't in the immediate future. Defining defaults for Lie groups is tricky.

Possibly another option would be to load ODE lazily if required?

This is probably wise even for Manifolds.jl.

This was referenced Oct 15, 2019
@mateuszbaran
Copy link
Member

I've looked at this again and essentially the last missing piece is gradient calculation. Optim.jl currently just projects Euclidean gradient to the tangent space but it doesn't work for some manifolds (see for example JuliaManifolds/ManifoldDiff.jl#27).

@antoine-levitt
Copy link
Collaborator Author

Optim uses manifolds as an opportunity feature - it takes a few lines of code to implement so why not. I think it's fine to keep Optim as a "poor man's manifold optimization" since the focus is on unconstrained optimization. Manopt.jl is there for more serious manifoldization. Unless there is a will to merge both packages?

@kellertuer
Copy link
Member

kellertuer commented Dec 28, 2019

I think that's a good approach, since Manopt.jl– as I think we already discussed – will focus on Optimization on Manifolds including constrained and nonsmooth (theses especially) problems.
Further I would like to stay in the area of manopt and pymanopt, where we currently join forces across languages.

@kellertuer
Copy link
Member

This has been inactive quite a while; on the other hand our API (ManifoldsBase.jl) is really stable by now – so besides the differences in the notion of a retraction, ManifoldsBase.jl could be used in Optim maybe (still staying on the opportunity/“poor mans variant” within Optim)?

@antoine-levitt
Copy link
Collaborator Author

Yeah I think the status has been "oh yeah, somebody should really do it at some point" for a few years now :-)

@kellertuer
Copy link
Member

That is great to know – and don't worry, I know that status just too well. Then let's leave this issue open until this really actually happens – maybe I find the time as well, but I am not so experienced with the inner organisation of the Optim.jl code

@mateuszbaran
Copy link
Member

A small update: I've made a script that allows for using Manifolds.jl manifolds in current Optim.jl: https://gist.github.com/mateuszbaran/0354c0edfb9cdf25e084a2b915816a09 . Changing Optim.jl to using Manifolds.jl manifolds without a wrapper would be a breaking change with minimal benefits over using that wrapper. IMHO the best short and medium-term path forward is putting the glue code in some package (either Optim.jl or Manifolds.jl) as a package extension (a new Julia 1.9 feature). What do you think?

@kellertuer
Copy link
Member

Just a question for understanding – with this wrapper Optim would do “addition in the embedding and projection” in its solvers?

I like the glue code idea especially since it is quite short. Maybe we can do an Issue in Optim to do the breaking change for the next larger update (that is when they have a breaking change anyways)?

I would maybe put the glue stuff it into Optim, though I do not have a very good argumen, just a feeling – since the code extends the manifold-capabilities of Optim.

@mateuszbaran
Copy link
Member

Just a question for understanding – with this wrapper Optim would do “addition in the embedding and projection” in its solvers?

Yes, that's what Optim.jl does. And also projecting tangent vectors.

I would maybe put the glue stuff it into Optim, though I do not have a very good argumen, just a feeling – since the code extends the manifold-capabilities of Optim.

Putting it in Optim.jl would make sense IMO but let's wait for an opinion of Optim maintainers.

@kellertuer
Copy link
Member

Does it maybe make sense to post this idea also at JuliaNLSolvers/Optim.jl#448 ?

@mateuszbaran
Copy link
Member

I think that topic is more about Optim.jl doing more than just retract and project_tangent and that's a different issue.

@kellertuer
Copy link
Member

ah, I see, right.

@pkofod
Copy link
Contributor

pkofod commented Mar 16, 2023

I think it would make sense. I do not have too much bandwidth, but I'd love to help if I can. Optim.jl is fine for me. If we need to break, let's discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants