JuliaManifolds / ManifoldsBase.jl

Basic interface for manifolds in Julia
https://juliamanifolds.github.io/ManifoldsBase.jl/
MIT License
83 stars 8 forks source link

Unify `exp` and `retract` #159

Closed kellertuer closed 9 months ago

kellertuer commented 1 year ago

as noted in #158, I saw that currently our default dispatch for exp and retract differ, when it comes to treating the last positional argument t. Here I would like to give a thorough overview about the state and think/discuss about the solution

exp and retract

exp follows the scheme “allocate first”, i.e.both with and without t they allocate and “pass over” to exp!

retract differs as

exp! and retract!

We have that

exp!(M::AbstractManifold, q, p, X, t::Number) = exp!(M, q, p, t * X)

So exp! fuses t and X by default. This is nice for the user since implementing exp!(M, q, p, X) is all they need to do, no need to think about t.

For retractthis is the other way around. retract!(M, q, p, X, m) adds 1.0 as second to last element, i.e. calls retract!(M::AbstractManifold, q, p, X, t::Number, method) – and this passes down to level 2 and 3 – _retract! and retract_method!, which never exist without t.

Summary

Own opinion

I feel that exp! is nice the way it is and an easy entry level. I feel that retract!compared to that is a bit unexpected. So I would prefer to keep exp as is; maybe if there is a way to keep the current state and that also exp!(M, q, p, X, t) alone would suffice, that would be the most neat one

Maybe this would also be the best for retract – but I have no good idea how to accomplish that either of them would suffice. This would at least mean that the “without t”variant has to dispatch through level 2 and 3 (a bit more of boiler plate code).

mateuszbaran commented 1 year ago

Defining variant with t is, for some manifolds, important for performance. So, either the variant with t is the fallback one or for some manifolds we need to define both variants to make everything work. I don't really mind having to implement both but I think it would be good to be aware of the tradeoff.

kellertuer commented 1 year ago

I am aware of the performance effects of the t-variant on some manifolds, it is just not so easy for the beginner to directly understand which function to implement (not yet why, just to find the right one).

Maybe we could to the same as for exp? By default retract_method! with t falls back to the one without? That way it's easy for the user to implement. This would mean that an advanced user has to either implement both variants (if the plain one does not yet exist) or add the one with t. ...unless we find a good way to “switch” which functions dispatches on which.

I also would not mind to implement both to get the performance effects, while just implementing the one without t is easy and works?

mateuszbaran commented 1 year ago

OK, that would be fine.

kellertuer commented 1 year ago

Performance aware people could even do

retract_method!(M, q, p, X) = retract_method!(M, q, p, X, t)

(which per se would introduce a circle but then) implement retract_method!(M, q, p, X, t). I think this one additional line can be expected of experts already caring much about performance, while this switch makes it much easier for beginners :)

On the other hand this decision means, we have to add layer 2 (the _retract dispatches) for all retractions and the none-t variants. That is a bit of boilerpplate code, but I think worth it for usability. I think I now what to do to get this done. But sure this is then breaking and something for 0.15.