Reduce Ambiguities by a two level scheme for decoratored functions

[kellertuer]

We currently have a few functions that are decorated, where we fight ambiguity errors. Since I did that yesterday again, I thought today a while about a solution. The problem occurs when we dispatch on the optional arguments, since that introduces ambiguities due to the scheme that

• Abstract Decorator + Abstract optional argument is used in decorator checks
• Abstract Manifold + concrete optional type is used for default implementations
• Concrete Manifold + concrete type (sometimes also abstract here) is used for implementations.

I think only functions where we dispatch on the optional argument are a challenge currently, i.e. exp(M,, p, X, t=1)is not a problem but retraction and functions involving a basis are. Luckily on the abstract/high Level these arguments we dispatch on have a default (the retraction type, the basis could also be more used with a default).

I have some ideas for a solution

A Two Layer idea

We introduce two layers. The upper (more semantic / user interface) layer uses keyword arguments for these optional/positional arguments we dispatch on later. This way you can not dispatch on these on the high level, but they are easy to use. We basically replace the last positional argument with w keyword argument.
this high level is decorated and acts transparent/parent/intransparent

Examples:

• get_vector(M, p, c; basis=DefaultBasis())– where we could use the same technique we have for vector transport and retractions actually and introduce a default_basis(M) function, too.

The second layer would not be transparent and the keyword(s) (bet lots hope we never reach a plural) would become the last positional argument, i.e.

• get_vector(M, p, c, basis), where basis does not have a default, i.e. is not optional but also the function is not transparent.

I think this can even be done in the same function name, since the first signature is shorter and we would hunky introduce transparency on the first layer. I think this might even remove the necessity for the invoke maker that we currently have for the basis dispatch for example.

Affected Functions

All affected functions would have the abstract documentation string to mention this. Currently I think this would affect

• retractions / inverse retractions
• vector transports
• all functions involving bases

Another good thing – I think – is, that generic functions can more clearly happen before resolving transparency (kwargs/AbstractDecorators) or after resolving transparency (no abstract decorator types, positional arguments).

I think this problem should not affect points/tangent vectors, since they are never typed on a level with transparency (that is they are ::Any until we really dispatch on the representations).

Summary

We mainly change the signature of transparent functions to not include optional arguments but put them (during transparency) into keyword arguments. After resolving the decorator, the function is the same as before, just not transparent anymore.

I hope this is only “mildly breaking”, i.e. in higher level functions (when implementing on ::AbstractManifold-level) we have to put retraction type/vectortransporttype/basis into kwargs.

Let me know what you think.

[kellertuer]

I just noticed that my above idea misses one point:

There is vector transport methods where we do dispatch on the manifold level (Pole & Schilds ladder) and that is currently – I think – not possible in the above idea.

[mateuszbaran]

I think this is a generally a good idea but I have a few comments.

I think this can even be done in the same function name, since the first signature is shorter and we would hunky introduce transparency on the first layer.

I think high-level and low-level functions should have different names to give us more flexibility with dispatch. Note that get_vector and get_coordinates have problems because there is no separation into many functions. The set of rules should be more or less (for get_vector):

1. Remove transparent decorators on manifold.
2. If it's a default basis-like thing, it should go to something _default_basis_get_vector. This avoids mixing with cached bases.
3. If it's a cached basis, it should go to to something like _cached_get_vector, which only has methods for the generic case, product and power manifolds.
4. If it's a ProjectedOrthonormalBasis, we should cache the basis first anyway.

I think a good way to see where the problems are is searching for uses of @invoke_maker in our codebase.

• retractions / inverse retractions
• vector transports
• all functions involving bases

That's right, and I think we could also improve injectivity_radius. It's main problem is that its two-argument variant has two different meanings, depending on types of arguments.

There is vector transport methods where we do dispatch on the manifold level (Pole & Schilds ladder) and that is currently – I think – not possible in the above idea.

It's definitely possible if we split vector transports into multiple functions: at the highest level we check for Pole & Schilds and forward to functions specific to them.

I hope this is only “mildly breaking”, i.e. in higher level functions (when implementing on ::AbstractManifold-level) we have to put retraction type/vectortransporttype/basis into kwargs.

I'm a currently unsure about the keyword argument idea. It is not key to fixing these problems (I think having more functions with fewer methods is). There is definitely a benefit to using keyword arguments when we have more than one argument with a default value (for example retractions) but this needs to be carefully considered. For example if a high-level function would just turn a keyword argument into a normal argument, that's doesn't introduce much value, I think there needs to be some additional logic involved.

[kellertuer]

Ok, then lets not follow the keyword idea but more like you said the inner functions with different names idea.

[kellertuer]

I just had a small idea – what if the internal functions differ mainly in having a different order (and less type ambiguities) lie

• retract(M, p, X, m) is the public one (with m having a default and such) also for the schild ladder and such, so the high level variant
• retract(m,M,p,X) (or retract(M,m,p,X)?) is the low level one, which does not allow for optional arguments and such?

[mateuszbaran]

Maybe it would work but I feel like it would only cause more confusion. I definitely prefer to have different functions at these two levels.

[kellertuer]

Ok, I was thinking about that a little bit yesterday and I do have a first idea but I am not yet sure how usable that will be.
Will try once we finish the other PR.

[mateuszbaran]

Great, I can definitely help with this. I agree that we should finish #89 first.

[kellertuer]

So the idea without permitting the arguments would have one disadvantage (that maybe can be solved with a macro)

We have to manually create the “link” from the high level to the lower level, for example
retract(M, p, X, ::PolarRetraction) = retract_polar(M, p, X)
(on the abstract manifold level)

but then having one (low level) function per retraction-type would resolve a lot of ambiguities.

The same holds for

• retract! (if we do both on the higher level we have the advantage that it will be easier to implement two low Level ones, such that we might also tacke the AD thingies)
• inverse_retract and inverse_retract!(where I would postfix the type like above)
• vector_transport_X(!) similarly just that for the lower level names I would maybe omit the vector_ and just use parallel_transport_X(!) or projection_transport_X(!)...
• for the basis related thing the lower level ones could be
  get_cached_vector, get_default_vector, get_default_onb_vector ?...

I hope this way packages like Manopt/RoMe would not be affected by this breaking change, only Manifolds.jl will.

Finally for the transparency and decorators I also spent some minutes this morning. Until now we never needed to change the transparency on runtime, such that all the transparency-with-lookup-and-dispatch is maybe a little over-engineered, and we could introduce the transparency similar to the manifold_elemend_forward macro, maybe in groups like

• checks
• inner and friends
• retractions and friends
• exp/log

That would be far more static since the macros would define fallbacks onto a field in the manifold, but I think it would be nicer both to program and with the groups even to use.
One could still join the groups into a macro that says TransparentEmbedding which sets a certain set of groups as we did until now.

[mateuszbaran]

We have to manually create the “link” from the high level to the lower level, for example retract(M, p, X, ::PolarRetraction) = retract_polar(M, p, X) (on the abstract manifold level)

but then having one (low level) function per retraction-type would resolve a lot of ambiguities.

Yes, I think that's a good approach.

The same holds for

• retract! (if we do both on the higher level we have the advantage that it will be easier to implement two low Level ones, such that we might also tacke the AD thingies)
• inverse_retract and inverse_retract!(where I would postfix the type like above)
• vector_transport_X(!) similarly just that for the lower level names I would maybe omit the vector_ and just use parallel_transport_X(!) or projection_transport_X(!)...
• for the basis related thing the lower level ones could be
  get_cached_vector, get_default_vector, get_default_onb_vector ?...

Yes, I think this is correct. One thing to keep in mind would be that default behaviors should be implemented on the higher level, thus freeing the lower level from having to do ambiguity resolution that is currently necessary. I'm not entirely sure to what degree it will make things easier for manifolds where the default behavior needs to be overwritten for some reason. Anyway I'm sure it would be nicer than what we have now.

I hope this way packages like Manopt/RoMe would not be affected by this breaking change, only Manifolds.jl will.

This will be massively breaking for Manifolds.jl but for Manopt and RoMe it should be nearly or entirely non-breaking.

Finally for the transparency and decorators I also spent some minutes this morning. Until now we never needed to change the transparency on runtime, such that all the transparency-with-lookup-and-dispatch is maybe a little over-engineered, and we could introduce the transparency similar to the manifold_elemend_forward macro, maybe in groups like

• checks
• inner and friends
• retractions and friends
• exp/log

That would be far more static since the macros would define fallbacks onto a field in the manifold, but I think it would be nicer both to program and with the groups even to use. One could still join the groups into a macro that says TransparentEmbedding which sets a certain set of groups as we did until now.

This also makes sense but I have to think about it more, making this a macro makes the system less extensible. Maybe it would be fine though, it's hard to tell at the moment.

[kellertuer]

To the last point – I feel we never needed the flexibility we currently build with the _transparentdispatch functions, that is why I am proposing this. The system is simpler (though less flexible/extensible, maybe).

[kellertuer]

Fixed in #92 🎉 .