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

Introduce an EmbeddedObjective #286

Merged
merged 26 commits into from
Sep 2, 2023
Merged

Conversation

kellertuer
Copy link
Member

@kellertuer kellertuer commented Aug 15, 2023

This PR resolves #217 by proposing an EmbeddedManifoldObjective.

Idea

  • the new objective is a wrapper
  • before calling the cost of the original objective it does an embed(M,p)
  • ...and similarly before calling get_gradient – but then also calls riemannian_gradient on the gradient the original objective returns

Interface

since we have the decorate_objective! chain already available, just introducing the objective_type=:Euclidean keyword there makes it possible for any objective to be wrapped. Currently also :Embedded is equivalent, since it actually works more general then just Euclidean – but :Euclidean is easier to understand for most users.

Internals

The EmbeddedManifoldObjective has two (optionally used) fields p and X for a point and tangent vector in the embedding. That way one can use embed! or get_gradient! in the embedding. This should reduce allocations tremendously.

ToDo

  • get_cost
  • get_gradient / get_gradient!
  • get_hessian / get_hessian!
  • get_grad_equality_constraint / get_grad_equality_constraint! (the the plural)
  • get_grad_inequality_constraint / get_grad_inequality_constraint! (and the plural)
  • More documentation
  • Testing
  • A Tutorial

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #286 (b0d2d7a) into master (2b3c35f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master     #286    +/-   ##
========================================
  Coverage   99.73%   99.74%            
========================================
  Files          76       77     +1     
  Lines        6892     7017   +125     
========================================
+ Hits         6874     6999   +125     
  Misses         18       18            
Files Changed Coverage Δ
src/Manopt.jl 87.50% <ø> (ø)
src/plans/constrained_plan.jl 100.00% <ø> (ø)
src/plans/plan.jl 100.00% <ø> (ø)
src/plans/embedded_objective.jl 100.00% <100.00%> (ø)
src/plans/objective.jl 100.00% <100.00%> (ø)
src/solvers/FrankWolfe.jl 100.00% <100.00%> (ø)
src/solvers/adaptive_regularization_with_cubics.jl 100.00% <100.00%> (ø)
src/solvers/augmented_Lagrangian_method.jl 100.00% <100.00%> (ø)
src/solvers/difference-of-convex-proximal-point.jl 100.00% <100.00%> (ø)
src/solvers/difference_of_convex_algorithm.jl 100.00% <100.00%> (ø)
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

I think the general idea looks good, I think just a few minor comments.

This PR resolves #217 by proposing an EmbeddedObjective– maybe one could also better all it EmbeddedManifoldObjective?

I think EmbeddedManifoldObjective would be more consistent with the other names?

which functions can we extend this to? Hessian for sure, maybe also proximal maps.

Is there actually a generic way to convert a Euclidean proximal map to a Riemannian one?

src/plans/objective.jl Outdated Show resolved Hide resolved
src/plans/objective.jl Outdated Show resolved Hide resolved
src/plans/objective.jl Outdated Show resolved Hide resolved
src/solvers/solver.jl Outdated Show resolved Hide resolved
Co-authored-by: Mateusz Baran <mateuszbaran89@gmail.com>
@kellertuer
Copy link
Member Author

I think the general idea looks good, I think just a few minor comments.

This PR resolves #217 by proposing an EmbeddedObjective– maybe one could also better all it EmbeddedManifoldObjective?

I think EmbeddedManifoldObjective would be more consistent with the other names?

That's what I thought as well when already writing this PR ;)

which functions can we extend this to? Hessian for sure, maybe also proximal maps.

Is there actually a generic way to convert a Euclidean proximal map to a Riemannian one?

You might be right, that that is not the case – but the constraints and their gradients, the differentials, they should be possible as well.

@kellertuer
Copy link
Member Author

kellertuer commented Aug 17, 2023

I think I now also sketched the hessian, before doing that for the constraints – since that is quite some work (though more in writing cases than in much thinking) I would like to discuss/check whether

  • the get_gradient cases can maybe be simplified
  • whether it might be reasonable to have a second embedded vector in the objective for the Euclidean Hessian (so that the Euclidean Hessian and the embedded direction can be computed without allocations).

edit: also this one for now has to wait for JuliaManifolds/ManifoldDiff.jl#30

@kellertuer
Copy link
Member Author

In order to have a nice tutorial, this probably also should wait for JuliaManifolds/Manifolds.jl#644

@kellertuer kellertuer added enhancement WIP Work in Progress (for a pull request) and removed discussion labels Aug 17, 2023
@kellertuer
Copy link
Member Author

This is just missing a bit of testing, but the tutorial is at least basically finished – it needs JuliaManifolds/Manifolds.jl#644, and I have to check while for now the Hessian conversion on Grassmann is not (yet) correct.

@kellertuer kellertuer added Ready-for-Review A label for pull requests that are feature-ready and removed WIP Work in Progress (for a pull request) labels Aug 30, 2023
@kellertuer
Copy link
Member Author

Sorry for the tag a bit too early, I missed the “automatic conversion to embedded objectives” for sub solvers (and was wondering why embedded worked so poorly with ARC).

@kellertuer
Copy link
Member Author

One thing I am a bit confused about it, that on https://manoptjl.org/previews/PR286/tutorials/EmbeddingObjectives/ the second run (with the euclidean ones converted) sometimes does work (yesterday evening) sometimes not (today) – on my machine that consistently needs 7 iterations and not 40 (which is the ARC default max iter).

Not sure what the CI is doing there.

@kellertuer
Copy link
Member Author

Ah, the main point is that I am stupid. Quarto renders on the last registered version. So sure that is the old one and hence fails. I will trick that a bit for this tutorial now.

@kellertuer kellertuer merged commit b574950 into master Sep 2, 2023
13 checks passed
@kellertuer kellertuer deleted the kellertuer/EmbeddedObjective branch November 20, 2023 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Ready-for-Review A label for pull requests that are feature-ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement a ManifoldEuclideanGradientObjective
2 participants