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

Adding support for Manifolds to Nelder-Mead #872

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

danspielman
Copy link

I did this in the obvious way: retract! is applied to every vector created by Nelder-Mead. I have found this very useful for doing optimization over complicated manifolds. I used to handle optimizations of this sort by using an objective function of the form x -> obj( retract (x)). But, this causes Nelder-Mead to operate in the wrong space. Using the manifold approach has sped up some of my code a lot, and has allowed it to converge in situations that failed before.

@codecov
Copy link

codecov bot commented Oct 2, 2020

Codecov Report

Merging #872 into master will increase coverage by 0.07%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #872      +/-   ##
==========================================
+ Coverage   82.07%   82.15%   +0.07%     
==========================================
  Files          43       43              
  Lines        2778     2790      +12     
==========================================
+ Hits         2280     2292      +12     
  Misses        498      498              
Impacted Files Coverage Δ
...c/multivariate/solvers/zeroth_order/nelder_mead.jl 85.52% <93.33%> (+1.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19c74be...1e00f2e. Read the comment docs.

@pkofod
Copy link
Member

pkofod commented Dec 16, 2020

Hi, thanks! And sorry for the very late response. I've been finishing up my thesis in the fall and have had little time for these projects. We could move the retraction inside of the centroid call and just have the centroid call accept the method, but this solution also looks fine. HAve you run into any issues since October? If not, I'll merge this. Again, apologies for the late response.

@danspielman
Copy link
Author

danspielman commented Dec 21, 2020 via email

@pkofod
Copy link
Member

pkofod commented Dec 22, 2020

BTW, if you like I would be happy to add support for the manifold of PSD matrices if someone else hasn’t done it yet. I use them often.

Are you in the #manifolds channel on the julia slack? I asked a question about this earlier today. It was my impression that PSD matrices do not live on a manifold, only PD matrices.

Edit: Oh, looking at your comment time stamp you mentioned this before I asked on slack. Interesting coincidence.

@danspielman
Copy link
Author

danspielman commented Dec 22, 2020 via email

@kellertuer
Copy link

Cool!. Alternatively you could check out the symmetric positive definite manifold from Manifolds https://juliamanifolds.github.io/Manifolds.jl/latest/manifolds/symmetricpositivedefinite.html and use nelder mead with that manifold in Manopt.jl, see https://manoptjl.org/stable/solvers/NelderMead.html

Or check the symmetric positive semidefinite manifold of fixed rank matrices https://juliamanifolds.github.io/Manifolds.jl/latest/manifolds/symmetricpsdfixedrank.html – I haven't checked plain semidefinite symmetric matrices yet.

@pkofod
Copy link
Member

pkofod commented Dec 22, 2020

Or check the symmetric positive semidefinite manifold of fixed rank matrices https://juliamanifolds.github.io/Manifolds.jl/latest/manifolds/symmetricpsdfixedrank.html – I haven't checked plain semidefinite symmetric matrices yet.

Are you stealing my users on a PR in my own package? 😱

🤣

@kellertuer
Copy link

Maybe ;)

One could also take the manifold mentioned as a starting point to add it here.

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.

3 participants