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

Fix ChambollePock TangentBundle methods #138

Merged
merged 5 commits into from
Jul 27, 2022

Conversation

mateuszbaran
Copy link
Member

I'm fixing an issue introduced by JuliaManifolds/Manifolds.jl#509 , that is TangentBundle no longer implementing exp, log, distance and parallel_transpot. Most issues are fixed by just having separate options for tangent bundle in options but one remaining problem is that DebugEntryChange requires distance to be implemented, which in turn requires log. My idea to solve that is to introduce inverse retraction-dependent distance approximations in ManifoldsBase.jl, that is something like distance(M::AbstractManifold, p, q, m::AbstractInverseRetractionMethod) = norm(M, p, inverse_retract(M, p, q, m)). What do you think about it?

@codecov
Copy link

codecov bot commented Jul 25, 2022

Codecov Report

Merging #138 (47c8bc3) into master (066b998) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 47c8bc3 differs from pull request most recent head 32d33a9. Consider uploading reports for the commit 32d33a9 to get more accurate results

@@           Coverage Diff           @@
##           master     #138   +/-   ##
=======================================
  Coverage   98.70%   98.70%           
=======================================
  Files          51       51           
  Lines        3322     3322           
=======================================
  Hits         3279     3279           
  Misses         43       43           
Impacted Files Coverage Δ
src/solvers/ChambollePock.jl 100.00% <ø> (ø)
src/plans/primal_dual_plan.jl 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@mateuszbaran mateuszbaran added the Ready-for-Review A label for pull requests that are feature-ready label Jul 25, 2022
@kellertuer
Copy link
Member

I think overall this looks fine and thanks for the fix, I am just a little insecure about the naming

inverse_retraction_method_tb but have not yet had a good idea how to name it instead. I feel _tb is not descriptive enough but writing tangentbundle is too long as an overall variable name.

@mateuszbaran
Copy link
Member Author

I was also thinking about calling it inverse_retraction_method_m and inverse_retraction_method_n, maybe that would be fine?

@kellertuer
Copy link
Member

That would be an idea, but a breaking change.

@kellertuer
Copy link
Member

What about retraction_method_dual and ìnvere_retraction_method_dual` since it refers to the n (N) one for the dual variable?

@mateuszbaran
Copy link
Member Author

That sounds fine 👍

Copy link
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.

We could shortly check that the examples still run fine (but the tests still do, so probably they'll do as well), but otherwise this looks nice.

@kellertuer kellertuer removed the Ready-for-Review A label for pull requests that are feature-ready label Jul 27, 2022
@kellertuer kellertuer merged commit cfeb7e1 into master Jul 27, 2022
@mateuszbaran
Copy link
Member Author

I really doubt this change can break any examples.

@kellertuer kellertuer deleted the mbaran/fixChambollePockDefaults branch October 18, 2022 18:13
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.

None yet

2 participants