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 proxes. #37

Merged
merged 15 commits into from
Dec 8, 2023
Merged

Introduce proxes. #37

merged 15 commits into from
Dec 8, 2023

Conversation

kellertuer
Copy link
Member

@kellertuer kellertuer commented Nov 28, 2023

Introduce the first proximal maps to this collection. I will surely add more docs and testing.

@mateuszbaran is it ok to also define proximal maps for basic functions (like the distance) here? Otherwise I can also defined them in ManoptExamples.jl, but I thought for such “elementary” functions like the distance it might be nice to have them here already.

This PR will be accompanied by two PRs over on ManoptExamples (JuliaManifolds/ManoptExamples.jl#12) and Manopt (JuliaManifolds/Manopt.jl#331) to clean up the later a bit.

Copy link

codecov bot commented Nov 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7fb4aa7) 95.26% compared to head (f1523ba) 95.46%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #37      +/-   ##
==========================================
+ Coverage   95.26%   95.46%   +0.20%     
==========================================
  Files          20       21       +1     
  Lines         359      375      +16     
==========================================
+ Hits          342      358      +16     
  Misses         17       17              

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

@mateuszbaran
Copy link
Member

What is the benefit of having proximal maps here instead of Manopt.jl? Can they be used for anything other than optimization?

@kellertuer
Copy link
Member Author

I am not sure – they should not be defined in Manopt.jl, since that package should focus on optimization and not corresponding functions /cost/grad/...) my alternative would be to defined them in ManopExamples.jl, but I feel the prox is as elementary as the gradient, especially for things like the distance.
According to https://en.wikipedia.org/wiki/Proximal_operator – it seems to mainly be used in optimization, that is true, but I would claim the same for the gradient https://en.wikipedia.org/wiki/Gradient

@mateuszbaran
Copy link
Member

OK, but if we are going to standardize it maybe we should consider a bit more generic interface, maybe more similar to https://github.com/JuliaFirstOrder/ProximalOperators.jl ?

@kellertuer
Copy link
Member Author

That would add a dependency here, but I think that sounds like a good idea.

@kellertuer
Copy link
Member Author

kellertuer commented Nov 28, 2023

Or do you think we should introduce our own generic interface here? Until now we do not do that for gradients of differentials or such, so why do it for proxes?

Until now we just have a naming convention, how to find gradients / differentials / ...

@mateuszbaran
Copy link
Member

I wouldn't necessarily make that a dependency but an interface inspired by that package. We need manifold as an additional argument after all.

Or do you think we should introduce our own generic interface here? Until now we do not do that for gradients of differentials or such, so why do it for proxes?

Until now we just have a naming convention, how to find gradients / differentials / ...

To be honest, I think we should also have such interface for gradients and differentials. I don't like our current convention, I just didn't have time to work on something better.

@kellertuer
Copy link
Member Author

But then maybe let's continue with introducing proxies also using the naming convention still,
but open an issue to have a nicer interface for all these functions (grad/diff/adjoint diff/Hessian/prox) later. We would keep the naming scheme and the implementations but mainly have an interface on top I think?

And sure I also am not that fond of the naming scheme – such an interface like ProximalOperators has would be nice, sure.

@mateuszbaran
Copy link
Member

We would keep the naming scheme and the implementations but mainly have an interface on top I think?

OK, we can postpone it but I'd try implementing all proximal operators as methods of prox! directly.

@kellertuer
Copy link
Member Author

Ah, but then the interface would be breaking (and similarly for grad/diff/...)? Sure that would be fine with me.

@mateuszbaran
Copy link
Member

Yes, it would be a breaking change.

@kellertuer
Copy link
Member Author

Ok, let me do this one without a break – but I can open an issue and write down the design we have in mind here.
Maybe I use that then as an excuse to still not continue with the test suite but do the breaking change once I am done with this group of PRs here.

@kellertuer
Copy link
Member Author

I will need this first in the examples, so this PR has to go first and should be finished by now
(having finished the examples I will update Manopt accordingly).

@kellertuer kellertuer added Ready-for-Review preview docs Add this label if you want to see a PR-preview of the documentation labels Dec 4, 2023
@kellertuer kellertuer marked this pull request as ready for review December 4, 2023 09:29
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 have a few small comments but I'd like to take a look at it again after I understand how exactly you define the proximal map here.

Changelog.md Outdated Show resolved Hide resolved
Changelog.md Outdated Show resolved Hide resolved
docs/make.jl Outdated Show resolved Hide resolved
src/proximal_maps.jl Outdated Show resolved Hide resolved
src/proximal_maps.jl Outdated Show resolved Hide resolved
src/proximal_maps.jl Outdated Show resolved Hide resolved
src/proximal_maps.jl Show resolved Hide resolved
src/proximal_maps.jl Outdated Show resolved Hide resolved
src/proximal_maps.jl Outdated Show resolved Hide resolved
docs/src/library.md Outdated Show resolved Hide resolved
docs/src/library.md Outdated Show resolved Hide resolved
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.

Last batch of comments :). Everything else looks OK.

src/proximal_maps.jl Outdated Show resolved Hide resolved
src/proximal_maps.jl Outdated Show resolved Hide resolved
src/proximal_maps.jl Outdated Show resolved Hide resolved
src/proximal_maps.jl Outdated Show resolved Hide resolved
src/proximal_maps.jl Outdated Show resolved Hide resolved
@mateuszbaran
Copy link
Member

@kellertuer
Copy link
Member Author

Oh my search-and-replace f->p_data was not careful enough there. Thanks for always being precise here :)

@kellertuer kellertuer merged commit b8c3464 into main Dec 8, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview docs Add this label if you want to see a PR-preview of the documentation Ready-for-Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants