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

Unify exp and retract #159

Closed
kellertuer opened this issue Jul 2, 2023 · 4 comments · Fixed by #167
Closed

Unify exp and retract #159

kellertuer opened this issue Jul 2, 2023 · 4 comments · Fixed by #167
Labels
Milestone

Comments

@kellertuer
Copy link
Member

kellertuer commented Jul 2, 2023

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

  • retract(M, p, X, m) calls retract(M, p, X, 1.0, m)
  • retractM, p, t, m) passes to _retract (level 2) which dispatches on the method and retract_method allocates and calls retract_method!– but they do not exist without t.

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

  • For exp it is necessary to implement exp!(M, q, p, X) and nice (maybe speedup?) to implement exp!(M, q, p, X, t), but this method alone is not enough.
  • For retract it is necessary to implement retract_method!(M, q, p, X, t), implementing the one without t is not possible.

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).

@kellertuer kellertuer added this to the 0.15.0 milestone Jul 2, 2023
@mateuszbaran
Copy link
Member

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
Copy link
Member Author

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

OK, that would be fine.

@kellertuer
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants