Refactor ManifoldsBase to a two-level dispatch design

[kellertuer]

This is a start to tackle #88.

I sketched the idea in the readme but there is a few points open to discuss

• I would like to move the documentation here, since the interface page is quite long and subpages might help the overview and also the explanation of design principles.
• Maybe we could also zenodo this one?
• We should discuss what to do with the current decorator scheme. I feel it is a little too complex. Yes one can change the dispatch by redefining the dispatch functions, but maybe macros that implement a set of functions by calling the ones on another manifold would to the trick as well? I will sketch this in the comings days/weeks
  I think this might be my project for the last week of this year, if time permits. Let‘s also discuss a few details here, for sure.

What needs to be done

• actually implement the lower layer - and maybe also move more Retraction Types and vector transports to base to provide their lower layer fallbacks on the interface level already. This includes
• retractions and inverse retractions
• vector transports
• bases related functions
• For user experience most vector transport types and retraction types should be moved to base to introduce them on layer 3 as well, for example the CaleyRetraction or other vector tranports
• We can introduce a EmbeddedRetraction that performs a certain retraction in the embedding and projects the result (this generalises the projectionretration, which would be the same with exp).
• check documentation / update documentation.
• get test coverage back
• should we use a separate logo for ManifoldsBase? see this comment for the logo proposal.

[codecov]

Codecov Report

Merging #91 (4693976) into master (125e03d) will increase coverage by 0.03%.
The diff coverage is 99.87%.

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
+ Coverage   99.80%   99.84%   +0.03%     
==========================================
  Files          15       17       +2     
  Lines        1556     1919     +363     
==========================================
+ Hits         1553     1916     +363     
  Misses          3        3              

Impacted Files	Coverage Δ
src/exp_log_geo.jl	100.00% <ø> (ø)
src/projections.jl	100.00% <ø> (ø)
src/PowerManifold.jl	99.45% <98.38%> (-0.28%)	⬇️
src/DefaultManifold.jl	100.00% <100.00%> (ø)
src/EmbeddedManifold.jl	100.00% <100.00%> (+1.69%)	⬆️
src/ManifoldsBase.jl	99.09% <100.00%> (-0.08%)	⬇️
src/ValidationManifold.jl	100.00% <100.00%> (ø)
src/bases.jl	100.00% <100.00%> (ø)
src/decorator_trait.jl	100.00% <100.00%> (ø)
src/nested_trait.jl	100.00% <100.00%> (ø)
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 125e03d...4693976. Read the comment docs.

[kellertuer]

...and just Maybe, when anyways writing a complete proper documentation, we can also use this to internally use traits for explicitly implemented features, that is

• Embedded
• Metric
• Quotient
• maybe also group/Lie

might become traits (on a middle layer kind of) in the following sense

What I currently do not like is our Abstract/NonAbstract split in embeddedmanifolds for example. I would like to have this more like a trait, since the abstract (or maybe implicit) case can only be specified for one of these features.

The idea would be that the non-implicit cases (for example metric and quotient combined) would provide default implementations that make things easier.
Sure this is still quite vague, but I hope to get more ideas when starting to write a little about these in some docs :)

[mateuszbaran]

Thanks for starting this! I've left a few comments for discussion.

[kellertuer]

Thanks for already checking grammar and such, this was a first sketch of ideas :) but sure it is good to have them also grammatically correct.

[mateuszbaran]

[ ] I would like to move the documentation here, since the interface page is quite long and subpages might help the overview and also the explanation of design principles.

Hm, why not just split the interface page instead of putting that content in the readme here?

[ ] Maybe we could also zenodo this one?

No idea. I'm a bit biased here because I don't use zenodo citations myself 😉. But feel free to add it if you think it's a good idea.

• We should discuss what to do with the current decorator scheme. I feel it is a little too complex. Yes one can change the dispatch by redefining the dispatch functions, but maybe macros that implement a set of functions by calling the ones on another manifold would to the trick as well? I will sketch this in the comings days/weeks

I'm trying to figure out how a macro-based forwarding for decorators (in the style of #89 ) could look like and I'm not sure if it would be significantly more simple.

actually implement the lower layer - and maybe also move more Retraction Types and vector transports to base to provide their lower layer fallbacks on the interface level already. This includes

What lower layer fallbacks do you have in mind?

I think this might be my project for the last week of this year, if time permits. Let‘s also discuss a few details here, for sure.

Cool, the two-level dispatch would definitely be a nice improvement.

...and just Maybe, when anyways writing a complete proper documentation, we can also use this to internally use traits for explicitly implemented features, that is

• Embedded
• Metric
• Quotient
• maybe also group/Lie

might become traits (on a middle layer kind of) in the following sense

What I currently do not like is our Abstract/NonAbstract split in embeddedmanifolds for example. I would like to have this more like a trait, since the abstract (or maybe implicit) case can only be specified for one of these features.

The idea would be that the non-implicit cases (for example metric and quotient combined) would provide default implementations that make things easier. Sure this is still quite vague, but I hope to get more ideas when starting to write a little about these in some docs :)

Usage patterns of Embedded, AbstractEmbedded, Metric, AbstractGroupManifold, GroupManifold, etc. are quite diverse so I just don't see how it would work. I'm going to wait for some more concrete examples 🙂 .

[kellertuer]

[ ] I would like to move the documentation here, since the interface page is quite long and subpages might help the overview and also the explanation of design principles.

Hm, why not just split the interface page instead of putting that content in the readme here?

Sure, I mean to combine the new notes (and old stuff) from readme into the docs here (and see the Readme as short as the Maniolfds.jl one)

[ ] Maybe we could also zenodo this one?

No idea. I'm a bit biased here because I don't use zenodo citations myself 😉. But feel free to add it if you think it's a good idea.

I am slo not yet 100% sure, since if one uses zenodo then probably the don from Manifolds.jl (and a fitting ManifoldsBase is implicit).

• We should discuss what to do with the current decorator scheme. I feel it is a little too complex. Yes one can change the dispatch by redefining the dispatch functions, but maybe macros that implement a set of functions by calling the ones on another manifold would to the trick as well? I will sketch this in the comings days/weeks

I'm trying to figure out how a macro-based forwarding for decorators (in the style of #89 ) could look like and I'm not sure if it would be significantly more simple.

I am not 100% sure myself yet, but I have an idea, I just have to see where that leads me :)

actually implement the lower layer - and maybe also move more Retraction Types and vector transports to base to provide their lower layer fallbacks on the interface level already. This includes

What lower layer fallbacks do you have in mind?

really just the retract(M, p, X, ::MyFancyMethod) = retract_fancy(M,p,X) things properly documented

Usage patterns of Embedded, AbstractEmbedded, Metric, AbstractGroupManifold, GroupManifold, etc. are quite diverse so I just don't see how it would work. I'm going to wait for some more concrete examples 🙂 .

Yes lets see. My current idea is mainly to use traits instead of the abstract cases and keep the concrete ones.

[mateuszbaran]

actually implement the lower layer - and maybe also move more Retraction Types and vector transports to base to provide their lower layer fallbacks on the interface level already. This includes

What lower layer fallbacks do you have in mind?

really just the retract(M, p, X, ::MyFancyMethod) = retract_fancy(M,p,X) things properly documented

OK, I just wouldn't really call it a fallback, it's the main implementation of that method.

[kellertuer]

Ok, maybe fallback was not the best term to choose here.

[kellertuer]

With the redesign we should even get rid of the PowerRetraction; the product retraction might still be useful (in Manifolds) for different retractions per component.

Edit: Ah no, we don't ;)

[kellertuer]

HM, I copied the docuemneter (nearly) from ManifoldDiffEx which also does not have its own documenter key, yet here it does not seem to use the org one - not yet sure why.

[mateuszbaran]

With the redesign we should even get rid of the PowerRetraction; the product retraction might still be useful (in Manifolds) for different retractions per component.

Edit: Ah no, we don't ;)

I think removing would have to happen in a quite different way, and maybe let's not do too many things at the same time.

HM, I copied the docuemneter (nearly) from ManifoldDiffEx which also does not have its own documenter key, yet here it does not seem to use the org one - not yet sure why.

I'm not sure, maybe the newer Documenter will work?

[kellertuer]

With the redesign we should even get rid of the PowerRetraction; the product retraction might still be useful (in Manifolds) for different retractions per component.
Edit: Ah no, we don't ;)

I think removing would have to happen in a quite different way, and maybe let's not do too many things at the same time.

No and we do not get rid of it, since it would just introduce a lot of ambiguities again (trying to remove it). It should stay.

HM, I copied the docuemneter (nearly) from ManifoldDiffEx which also does not have its own documenter key, yet here it does not seem to use the org one - not yet sure why.

I'm not sure, maybe the newer Documenter will work?

The problem seems to be the push at the end, not the rendering of the docs in general.

[mateuszbaran]

With the redesign we should even get rid of the PowerRetraction; the product retraction might still be useful (in Manifolds) for different retractions per component.
Edit: Ah no, we don't ;)

I think removing would have to happen in a quite different way, and maybe let's not do too many things at the same time.

No and we do not get rid of it, since it would just introduce a lot of ambiguities again (trying to remove it). It should stay.

Good, we're on the same page then :).

[mateuszbaran]

I've updated forwarding of some basis-related functions which fixes some issues with SPD manifold. Some inconsistency related to default_metric_dispatch still makes some SPD tests fail.

[mateuszbaran]

I'm not sure why we had representation_size defined as a tuple for tangent bundle but it is incompatible with the way you do size checking (i.e. ProductRepr points on a tangent bundle don't have a size).

[kellertuer]

We could also overload check size for this case – the default is just a lazy check with the matrix sizes (but I wrote new checks for FixedRank for example)

[mateuszbaran]

Yes, that's a possibility if we actually need representation_size on VectorBundle, which I currently doubt.

[kellertuer]

That is exactly why I currently implemented it (also for circle and positive numbers) to say: Ah - size is fine! when representation size is () ;) kind of a lazy fallback to noch checking sizes. We can overwrite it (as I said I did that for Fixedrank where it was really nice to do) if needed.

[kellertuer]

Can you maybe shortly explain why the fallbacks to the embedding where wrong code? I do not see that directly.

[mateuszbaran]

Can you maybe shortly explain why the fallbacks to the embedding where wrong code? I do not see that directly.

For some functions they are fine, but specifically for representation_size it doesn't work right for HalfPlaneManifold because it uses a different representation. So, falling back to embedding is fine only as long as the embedded manifold doesn't change the representation, which as far as I know isn't expressed in any way in our traits.

[kellertuer]

Well I always thought of them as reasonable defaults, so then the one test case / manifold does not fit one can always implement the non-decorator case for this manifold specifically and keep them as general fallbacks? This way most manifolds would not need an implementation (for the embedded ones) since the cases you deleted act as their default implementation using the embedding.

[mateuszbaran]

OK, this makes sense. I'll patch representation_size accordingly then.

[kellertuer]

Great thanks. Maybe we should document this motivation / modelling more.

[mateuszbaran]

Good idea, I'm sometimes not sure how things are supposed to work myself.

[kellertuer]

Don'T worry me as well in some places, just here I put a lot of thought into these defaults myself, so I remember those and why I put them there (and what to do for the non default case).

[mateuszbaran]

I've done the forwarding of representation_size, does it look right?

[mateuszbaran]

BTW, I've updated a few manifolds in the Manifolds.jl branch.

[kellertuer]

Yes that looks perfectly fine.

I noticed that bot having the fallback for get_coordinates/get_vector night be ok, since not really many manifolds will have the same coordinates/vectors as their embeddings, so we can also keep the current state as is there.

[kellertuer]

I started with hyperbolic and removed quite some forwards to .valuesince we have nice macros for that now. Then I noticed that we might have to adapt our scheme a little (or I am missing something, still). When checking a PoincareBallPoint, our new is:_point function thinks - ah! Embedded, let me check! but embedding the ball point is still in R3, the embedding (of arrays / HyperboloidPOints) is R3 so the test fails since the check is called on the embedding.

So either the get_embedding has to know which representation we are talking about (as an optional parameter the point should be enough) or the embedding itself (IsEmbeddedManifold) has to “know” which embedding we are talking about.

These seem to be the main remaiing errors with Hyperbolic.

[mateuszbaran]

I started with hyperbolic and removed quite some forwards to .valuesince we have nice macros for that now. Then I noticed that we might have to adapt our scheme a little (or I am missing something, still). When checking a PoincareBallPoint, our new is:_point function thinks - ah! Embedded, let me check! but embedding the ball point is still in R3, the embedding (of arrays / HyperboloidPOints) is R3 so the test fails since the check is called on the embedding.

I don't really understand it. A manifold is supposed to have a single embedding, and whatever representation of points is used, a point in the same embedding should be returned, right? And Hyperbolic(N) is supposed to be embedded in Lorentz(N+1)? At least that's what decorated_manifold returns.

[kellertuer]

I would say that was a flaw in the old system, is mildly one in the system now as well.

When using arrays or HyperboloidPont you are correct.
When using one of the PoincarePointds the embedding is R2, but I am too lazy to specify the metric.
So for different representations of points on one manifold we usually have different embeddings (but I would say get_embedding should return the default one).
But I also only noticed when working on Hyperbolic now that the checks are a little more rigorous. Adding the p worked fine, I seem to just Miss to fix a final get basis function that does not yet do what I hoped it would do now.

[mateuszbaran]

OK, making embedding dependent on the point is fine.

[kellertuer]

We are slowly getting there. SkewHermitian has still a but but I fixed quite a few os the smaller manifolds (Spectrahedron etc) and Hyperbolic as well.
The only larger part will be Lie groups I think.

[mateuszbaran]

Yes, Lie groups seem to be the only remaining big issue.

[mateuszbaran]

Sorry for accidentally merging this. I think we need a new PR to continue 🙁 .

[kellertuer]

Yes this was maybe a little early, though I think this might actually nearly be finished, so it might not be long anymore.

[mateuszbaran]

I'm still unsure whether the trait system is going to be expressive enough for Lie groups but other than that it looks fine.