Custom inverse retraction for StopWhenChangeLess

[mateuszbaran]

One small change so that StopWhenChangeLess can be used on, for example, tangent bundles.

[codecov]

Codecov Report

Merging #144 (a0947a1) into master (fbdb608) will decrease coverage by 0.26%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
- Coverage   98.98%   98.71%   -0.27%     
==========================================
  Files          51       51              
  Lines        3335     3335              
==========================================
- Hits         3301     3292       -9     
- Misses         34       43       +9     

Impacted Files	Coverage Δ
src/plans/stopping_criterion.jl	96.87% <100.00%> (ø)
src/plans/stepsize.jl	91.27% <0.00%> (-5.24%)	⬇️

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

[kellertuer]

Does distance always accept a retraction? I was not yet aware of that. For me that seems a little strange since I thought distance always refers to exp/log (as a fallback with norm). Anything else would be only an approximate distance?

[kellertuer]

Ah, I see, somehow I missed that distance was introduced with a retraction; I just wonder why for exp, log, parallel_transport we use new names for the approximate versions while for distance we don't?

[mateuszbaran]

Distance accepting an inverse retraction is a new addition from when I was fixing Chambolle-Pock. For StopWhenChangeLess sometimes an approximation to the exact distance would be enough I think. Here is the relevant PR: JuliaManifolds/ManifoldsBase.jl#119 .

[kellertuer]

Oh I am not complaining about the possibility to have an approximate distance with the help of a retraction, I am just confused about the naming (and must have missed that point in the linked PR, I should have asked it there).

[kellertuer]

Maybe we could resolve this even more general, that one (alternatively) can specify the field (defaulting to :x for now, later maybe to :Iterate, see the Frank Wolfe branch for first ideas to decouple :x from the :Iterate) and a measure for distance (which default “just” to (p,q) -> distance(M,p,q) but that way you could pass in your favourite distance-like-function yourself?

[mateuszbaran]

Sure, this stop condition could be improved further but maybe let's stop at having a better default in this PR?

[kellertuer]

If you think this is too much work, sure; the only disadvantage I see is, that this introduces a generality that the future variant might not (again be able to) cover, so the more general change might be breaking

[mateuszbaran]

I think whatever future design is going to be it should have the behavior from this PR as a default so the more general change should take it into account. This way users can just use this condition for tangent bundles without thinking that they don't have log implemented. Using custom distances should just be additional possibility on top of this, though maybe the default will be implemented slightly differently later.

[kellertuer]

Hm,
my thinking is the following:
I would prefer to rename the retractoin-based-distance-approximation (calling it distance might be misleading) - then I would be fine using that name as a default here.
And the more general thing is just then not necessary here.

But you are right that might even be a breaking change in Manifolds and not necessarily here. So let‘s stay with your idea for now here.

[kellertuer]

...and yes I should have noticed this naming problem in the linked PR but I did not sorry for that.

[mateuszbaran]

Ah, OK, so I think we can add that renaming issue to JuliaManifolds/Manifolds.jl#438 to keep track of it?

[kellertuer]

Yes that would be a good idea, a problem why I stay a little vague is, that I do not have a good idea for that thing yet (retraction_distance sounds a little strange to me for example).