Skip to content

Add support for Riemanian objective through JuMP#448

Merged
kellertuer merged 14 commits intoJuliaManifolds:masterfrom
blegat:riemann_obj
Apr 18, 2025
Merged

Add support for Riemanian objective through JuMP#448
kellertuer merged 14 commits intoJuliaManifolds:masterfrom
blegat:riemann_obj

Conversation

@blegat
Copy link
Copy Markdown
Collaborator

@blegat blegat commented Apr 10, 2025

Closes #273

Comment thread ext/ManoptJuMPExt.jl Outdated
@kellertuer
Copy link
Copy Markdown
Member

I just finished #449 which does what you need (hopefully) at least for all objectives that (only) need cost/grad/hess – but it could in principle also be done for the non smooth prox ones.

So if you merge master into yours, you have even an -obj that creates the ScaledManifoldObjective wrapper :)

@kellertuer
Copy link
Copy Markdown
Member

Thanks for continuing here!

Over easter I can take a look at the failing tests and try to maybe even write a more generic tutorial that uses this new mode.

Would it make sense to also take a look at the non-array-manifolds here already or would it be better to check that on a next PR?

@blegat
Copy link
Copy Markdown
Collaborator Author

blegat commented Apr 17, 2025

I prefer smaller PRs whenever possible

@kellertuer
Copy link
Copy Markdown
Member

Also perfect with me. Then we should fix the tests and maybe check with a small start of a tutorial, that all the new code works as expected.

Copy link
Copy Markdown
Member

@kellertuer kellertuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The objective is an optimization part so it is part of Manopt not ManifoldsBase.
At the end we also need a Changelog entry for the new patch version (that is one of the tests failing).

I had hoped I can edit this locally and contribute a tutorial to this PR as well, that seems to not be the case I can only propose changes here (but maybe accept them to see how far CI gets?)

Comment thread ext/ManoptJuMPExt.jl Outdated
Comment thread ext/ManoptJuMPExt.jl Outdated
@blegat
Copy link
Copy Markdown
Collaborator Author

blegat commented Apr 17, 2025

Weird, "Allow edits and access to secrets by maintainers" is checked

@kellertuer
Copy link
Copy Markdown
Member

Sure I can pull and I can accept these edit comments here. But doing those feels a bit like hand-craftedly sending postcards to GitHub ;) I can not push from my locally checked out branch.

Whatever, then we do this PR shorter – you like that anyways :) - and just do the feature improvement. I can try locally already to get a tutorial working and that will be a next PR.

For not what is missing is

  • Documenter is missing MathOptInterface.set which should go to the extension page docs/src/extensions.md in the right section
  • I could not figure out the CI failure and conversion errors the new code still does.

@kellertuer
Copy link
Copy Markdown
Member

Ah and the Changelog.md in the main folder needs a short comment for the new version that the interface was generalised.

@blegat
Copy link
Copy Markdown
Collaborator Author

blegat commented Apr 17, 2025

What can we do if model.objective is nothing. Is there an easy way to get a zero objective to be used as second argument to decorate_objective! ?

@kellertuer
Copy link
Copy Markdown
Member

What would that be used for? I currently do not have in mind what a “zero objective” mightt be.

The easiest “empty” objective is probably https://manoptjl.org/stable/plans/objective/#Manopt.ManifoldCostObjective so something like

obj = ManifoldCostObjective( (M,p) -> -Inf )

(or any other constant that -Inf)

Comment thread ext/ManoptJuMPExt.jl Outdated
Comment thread Changelog.md Outdated
@blegat
Copy link
Copy Markdown
Collaborator Author

blegat commented Apr 17, 2025

It seems there is an issue with scaled objective because e98ea6f fixed the tests

@blegat
Copy link
Copy Markdown
Collaborator Author

blegat commented Apr 17, 2025

With the scaled objective that you have added, the point does not move, very weird

@blegat
Copy link
Copy Markdown
Collaborator Author

blegat commented Apr 17, 2025

The tests are passing locally, I won't have time to do more today but feel free to merge if ci is green in order to iterate on a follow up PR

@blegat
Copy link
Copy Markdown
Collaborator Author

blegat commented Apr 17, 2025

The issue with the current approach is that the flipping of the objective sense don't happen if a riemannian objective is set

@kellertuer
Copy link
Copy Markdown
Member

The tests are passing locally, I won't have time to do more today but feel free to merge if ci is green in order to iterate on a follow up PR

Nice! For me passing tests is fine enough, because we also have a test for the extension so it still works as expected. And I can try to write a short quarto-based tutorial and link to it from the extensions page as well in a next PR

The issue with the current approach is that the flipping of the objective sense don't happen if a riemannian objective is set

That I do not fully understand but I will check the code.

Thanks for all your work, would be super cool to maybe also get the other open point with non-array things done somewhen then – I can also first check how much I understand of the things that have to be changed then.

@blegat blegat mentioned this pull request Apr 18, 2025
@kellertuer kellertuer merged commit 181cd63 into JuliaManifolds:master Apr 18, 2025
11 checks passed
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.88%. Comparing base (6430b31) to head (79b7bf9).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #448   +/-   ##
=======================================
  Coverage   99.88%   99.88%           
=======================================
  Files          83       83           
  Lines        8742     8747    +5     
=======================================
+ Hits         8732     8737    +5     
  Misses         10       10           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Riemannian black box function with JuMP/MOI

2 participants