search

JuliaManifolds / ManifoldsBase.jl

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

Refactor ManifoldsBase to a two-level dispatch design #91

Closed kellertuer closed 2 years ago

kellertuer commented 2 years ago

This is a start to tackle #88.

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

What needs to be done

codecov[bot] commented 2 years ago

Codecov Report

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

Impacted file tree graph

@@            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%) :arrow_down:
src/DefaultManifold.jl 100.00% <100.00%> (ø)
src/EmbeddedManifold.jl 100.00% <100.00%> (+1.69%) :arrow_up:
src/ManifoldsBase.jl 99.09% <100.00%> (-0.08%) :arrow_down:
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 commented 2 years ago

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

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

kellertuer commented 2 years ago

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 commented 2 years ago

[ ] 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 :wink:. 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 :slightly_smiling_face: .

kellertuer commented 2 years ago

[ ] 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 commented 2 years ago

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 commented 2 years ago

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

kellertuer commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

kellertuer commented 2 years ago

I think I will first refactor the decorator pattern, since this two-level approach (already for retractions) introduced a lot more signatures that had to be made decorated - now for vector transport this increases tremendously. So before getting to basis (or figuring out the remaining vector transport errors, most of them are ambiguities that would be resolved by the first part of this comment) ...

I will sketch the decorator-using-traits idea

mateuszbaran commented 2 years ago

Hm, I'm actually not sure that decorator change would help here. We have an independent problem here, that is whether dispatch-on-retraction-type or dispatch-on-manifold-decorator should take precedence. If there is no universal preference, we need a full disambiguation thing for each pair of decorator and retraction type anyway. If there is a universal preference, we can solve it by adding a third level of dispatch (for example resolve decorators -> resolve method -> resolve manifold). Would that be a viable thing? I don't see any obvious problems with that.

kellertuer commented 2 years ago

Hm, I do not see how in the current decorator this can be fixed but I have an idea in a new system (maybe that is what you describe as a third layer basically).

In general we have a scheme, we resolve retraction first, and only then the manifold.

A decorator would be before that for sure so the order you describe is what I have in mind, sure. It would mean, however, that we would always pass on e.g. all retractions (or none) through a decorator, but I think that is ok.

The reason I do not see that I can do it with the current system is, that resolving retractions in the decorator nearly took more lines then we added for the lower level in the retractions themselves. So if we need more lines of code in the decorator than in the actual code, I do not see how the decorator would be useful actually (in its current form).

mateuszbaran commented 2 years ago

Very likely current form of decorators won't be particularly helpful. Anyway, here is a sketch of what I had in mind:

# a very rough idea of decorator unwrapping -- note that we don't need to write it separately for each method because that is handled by a function on a lower level
# this still needs variants for different method signatures (with `t` or without, mutating or not etc.) but much fewer of them
retract!(M::AbstractManifold, q, p, X, m::AbstractRetractionMethod) = retract_undecorated!(M, q, p, X, m)
retract!(M::AbstractGroup, q, p, X, m::AbstractRetractionMethod) = retract!(base_manifold(M), q, p, X, m)
retract!(M::MetricManifold, q, p, X, m::AbstractRetractionMethod) = retract_metric!(M, q, p, X, m)

# that's what you've already done
retract_undecorated!(M::AbstractManifold, q, p, X, m::ExponentialRetraction) = retract_exp!(M, q, p, X)
retract_undecorated!(M::AbstractManifold, q, p, X, m::PolarRetraction) = retract_polar!(M, q, p, X)

Hmm... this, plus automatic generation of the forwards would be basically your idea of the new decorator system, right?

kellertuer commented 2 years ago

Oh thats nearly my idea just that I would omit

 retract!(M::AbstractManifold, q, p, X, m::AbstractRetractionMethod) = retract_undecorated!(M, q, p, X, m)

(this one is the default and already done, retract is the high level

And write the other two with traits but their short form might be

retract!(M::AbstractGroup, q, p, X, m::AbstractRetractionMethod) = retract!(base_manifold(M), q, p, X, m)
retract!(M::MetricManifold, q, p, X, m::AbstractRetractionMethod) = retract!(base_manifold(M), q, p, X, m)

(note that retractions are independent of the metric, since they just need the differential in their def)

Hmm... this, plus automatic generation of the forwards would be basically your idea of the new decorator system, right?

A mix of auto-generation/macros and traits – thats where I still have to spend time on. But this is far shorter than the current system (which in the current intermediate state just has a lot errors/ambiguities). So I would like to have more commits in between that pass the tests (just because I am not a fan of fixing tests for days), thats why I want to go for the decorator next.

mateuszbaran commented 2 years ago

Oh thats nearly my idea just that I would omit

 retract!(M::AbstractManifold, q, p, X, m::AbstractRetractionMethod) = retract_undecorated!(M, q, p, X, m)

(this one is the default and already done, retract is the high level

I think this part is actually essential to reduce ambiguities. Note that this way retract! doesn't have to deal with method-related ambiguities. Similarly, retract_undecorated! doesn't have to deal with decorators.

(note that retractions are independent of the metric, since they just need the differential in their def)

What do you mean by retractions being independent of metric? MetricManifold decorator often changes the result of, for example, the exponential retraction.

kellertuer commented 2 years ago

Uff, but that would basically be a complete third layer and I already felt now like quite stupid generating all these functions for the lower layer. Not much thinking just coding carefully not to forget a case and such. Hm, have to think about that then. Maybe really with quite some macros (but I find them not that easy to code).

Concerning the Metric Manifolds – well – exp is the only retraction it does change, actually, but right that one it does. Hm, then – both these things make my ideas more complicated and complex.

mateuszbaran commented 2 years ago

Uff, but that would basically be a complete third layer and I already felt now like quite stupid generating all these functions for the lower layer. Not much thinking just coding carefully not to forget a case and such. Hm, have to think about that then. Maybe really with quite some macros (but I find them not that easy to code).

Yes, that's a third layer and it introduces additional forwarding but I think forwarding is more simple than fighting with ambiguities ;).

Concerning the Metric Manifolds – well – exp is the only retraction it does change, actually, but right that one it does. Hm, then – both these things make my ideas more complicated and complex.

Actually there is also the ODE-based retraction (which is technically different) to make it even more complicated.

kellertuer commented 2 years ago

Forwarding is simpler, but the middle layer would change name again, and I think I would like to really have some layer macros then.

Oh yeah I forgot about the ODE one, that is also metric dependent, for sure.

mateuszbaran commented 2 years ago

I can help with macros but at the moment I don't quite see a clear pattern here. Feel free to post here even a rough sketch of how it could work to help me figure it out :slightly_smiling_face: .

kellertuer commented 2 years ago

Thinking about it for a while, the one thing I am not much in favour of yet is that with the third layer we have so many function names, that for any user it might be complicated to find the right one to overwrite. Especially if they want to enter on the high level (but undecorated already) level, your proposed function name retract_undecorated is not a good experience for a user.

Maybe here is a short idea. We have 3 layers

1) Manifold layer – this is where we organise the decorators, it the area where „the actual manifold a function is dispatched to“ is figured out. You implement here high level dispatch like for embeddings / quotients / ... maybe with traits we will see ;)

2) This moves from a fixed manifold and “sorts out” dispatch onto “subtypes” of functions (dispatch onto specific retractions ...) - maybe a kind of „middle layer“

3) when both are fixed we reach the low level, where one actually does the implementation - the already „low level“ called one.

I would call 1) The high level - here new manifolds are „introduced and designed“, i.e. it also dispatched onto itself until the manifold is figured out where the actual implementation will be asked/looked for but none other than the manifold should be strongly typed. 2) This interims layer should usually not be touched by and end user, unless one introduces new retractions (i.e. I will get most retractions over to base as well, so most are available - Caley for example) 3) implement your fun out here.

So with that the actual new one is layer 2, and we could emphasise the internal nature by calling all functions on this layer _retract / _retract what do you think? They are the signatures with explicit ::ParallelTransport and passing on to parallel_transport.

kellertuer commented 2 years ago

Here is my idea (with the 3 layers) and traits

1) Layer 1 with traits https://github.com/JuliaManifolds/ManifoldsBase.jl/pull/91/files#diff-781aa41b427ec28b133baaa8322416aa38ebc2f6324dc2d7a2cd46fda72f5491

2) Layer 2 now with _ as isn line 274 https://github.com/JuliaManifolds/ManifoldsBase.jl/pull/91/files#diff-f5785906db07385d6161a45ba0c67152cb6854c110f0c981f96af26b3e8279d8

(have not yet done the rest, but I think this scheme might be worth it).

My next step would be to write down above scheme of 3 layers with much more detail in the design.md - independent of how the layer 1 is realised.

If you agree on the traits, then I would continue on that. We still have an AbstractDecorator, but We might get rid of the other Abstract types if we want to (and I think we want to) since the traits allow for a manifold that in the old system would habe both <: AbstractEmbeded and <: AbstractGroup (not possible) since here we can just activate them using the is_... functions. Note that we still need the AbstractDecorator, unless we define the default functions (inner in ManifoldsBase.jl for example) as {<: AbstractManifold; !IsIsometricallyEmbeddedManifold(X)} (which would be possible but maybe clumsy).

What do you think?

mateuszbaran commented 2 years ago

Thinking about it for a while, the one thing I am not much in favour of yet is that with the third layer we have so many function names, that for any user it might be complicated to find the right one to overwrite. Especially if they want to enter on the high level (but undecorated already) level, your proposed function name retract_undecorated is not a good experience for a user.

Maybe here is a short idea. We have 3 layers

  1. Manifold layer – this is where we organise the decorators, it the area where „the actual manifold a function is dispatched to“ is figured out. You implement here high level dispatch like for embeddings / quotients / ... maybe with traits we will see ;)
  2. This moves from a fixed manifold and “sorts out” dispatch onto “subtypes” of functions (dispatch onto specific retractions ...) - maybe a kind of „middle layer“
  3. when both are fixed we reach the low level, where one actually does the implementation - the already „low level“ called one.

I would call

  1. The high level - here new manifolds are „introduced and designed“, i.e. it also dispatched onto itself until the manifold is figured out where the actual implementation will be asked/looked for but none other than the manifold should be strongly typed.
  2. This interims layer should usually not be touched by and end user, unless one introduces new retractions (i.e. I will get most retractions over to base as well, so most are available - Caley for example)
  3. implement your fun out here.

So with that the actual new one is layer 2, and we could emphasise the internal nature by calling all functions on this layer _retract / _retract what do you think? They are the signatures with explicit ::ParallelTransport and passing on to parallel_transport.

This definitely makes sense and we should be careful to introduce as little confusion as possible. retract_undecorated is just a not-well-thought-out name, and _retract seems better. People should rarely have the need to use it anyway. I think this is a good plan.

  1. Layer 1 with traits https://github.com/JuliaManifolds/ManifoldsBase.jl/pull/91/files#diff-781aa41b427ec28b133baaa8322416aa38ebc2f6324dc2d7a2cd46fda72f5491

IIRC last time I checked SimpleTraits there was some problem with it but I've just checked them again (mostly the macro expansion side) and it seems fine-ish.

2. Layer 2 now with _ as isn line 274 https://github.com/JuliaManifolds/ManifoldsBase.jl/pull/91/files#diff-f5785906db07385d6161a45ba0c67152cb6854c110f0c981f96af26b3e8279d8

This looks fine with one minor point: do we want exp to be high-level or low-level?

My next step would be to write down above scheme of 3 layers with much more detail in the design.md - independent of how the layer 1 is realised.

If you agree on the traits, then I would continue on that. We still have an AbstractDecorator, but We might get rid of the other Abstract types if we want to (and I think we want to) since the traits allow for a manifold that in the old system would habe both <: AbstractEmbeded and <: AbstractGroup (not possible) since here we can just activate them using the is_... functions. Note that we still need the AbstractDecorator, unless we define the default functions (inner in ManifoldsBase.jl for example) as {<: AbstractManifold; !IsIsometricallyEmbeddedManifold(X)} (which would be possible but maybe clumsy).

This is a big change and I'll have to think a bit about it. Getting rid of abstract decorators in favour of traits intuitively makes sense. Maybe even SimpleTraits would be the right solution for traits but we probably won't know until we try it. Keeping AbstractDecorator is fine.

Generally I think you are on the right track.

kellertuer commented 2 years ago

This definitely makes sense and we should be careful to introduce as little confusion as possible. retract_undecorated is just a not-well-thought-out name, and _retract seems better. People should rarely have the need to use it anyway. I think this is a good plan.

I would document it as that: This layer should not be touched unless one introduces a new retraction for example - one further reason to provide most retraction types already here in ManifoldsBase

  1. Layer 1 with traits https://github.com/JuliaManifolds/ManifoldsBase.jl/pull/91/files#diff-781aa41b427ec28b133baaa8322416aa38ebc2f6324dc2d7a2cd46fda72f5491

IIRC last time I checked SimpleTraits there was some problem with it but I've just checked them again (mostly the macro expansion side) and it seems fine-ish.

I will check. The good thing is, that we do not need combinations like an EmbeddedGroupManifold (I think group manifold was something like that at least for a while) - sure we have to check it once I finished retractions and we actually can check. We definetly need one AbstractDecoratorManifold still, but we can more easily combine/mix decorators with the Traits approach.

  1. Layer 2 now with _ as isn line 274 https://github.com/JuliaManifolds/ManifoldsBase.jl/pull/91/files#diff-f5785906db07385d6161a45ba0c67152cb6854c110f0c981f96af26b3e8279d8

This looks fine with one minor point: do we want exp to be high-level or low-level?

Perfect. Since Exp does not have the necessity to resolve a parameter I think it is both. It is the high level function people should use (though it is a special case of retract as well), but it is also the low level one people should implement. Maybe it is a little bit more low level (since on the high level it would also be ExponentialRetraction). Similarly for parallel_transport, where I also like this approach, because it brings back that function name – which I find nice :)

My next step would be to write down above scheme of 3 layers with much more detail in the design.md - independent of how the layer 1 is realised. If you agree on the traits, then I would continue on that. We still have an AbstractDecorator, but We might get rid of the other Abstract types if we want to (and I think we want to) since the traits allow for a manifold that in the old system would habe both <: AbstractEmbeded and <: AbstractGroup (not possible) since here we can just activate them using the is_... functions. Note that we still need the AbstractDecorator, unless we define the default functions (inner in ManifoldsBase.jl for example) as {<: AbstractManifold; !IsIsometricallyEmbeddedManifold(X)} (which would be possible but maybe clumsy).

This is a big change and I'll have to think a bit about it. Getting rid of abstract decorators in favour of traits intuitively makes sense. Maybe even SimpleTraits would be the right solution for traits but we probably won't know until we try it. Keeping AbstractDecorator is fine.

Generally I think you are on the right track.

Perfect, then I will continue this way (wanted to get this feedback first), of course I am not 100% sure this will work out completely, we will know later. But to me (now that I sketched the overall picture) it feels better than the previous approach, mainly due to the strict layers and that we fixed what should happen when and where. To me this order was not always clear in the previous modelling.

Tusen takk :)

mateuszbaran commented 2 years ago

This definitely makes sense and we should be careful to introduce as little confusion as possible. retract_undecorated is just a not-well-thought-out name, and _retract seems better. People should rarely have the need to use it anyway. I think this is a good plan.

I would document it as that: This layer should not be touched unless one introduces a new retraction for example - one further reason to provide most retraction types already here in ManifoldsBase

Yes, this is a good explanation. I'd just split "touching" into "calling" and "extending". Extending is, as you wrote, for introducing new retraction types etc. while direct calling should only occur from high-level decorator dispatch functions.

  1. Layer 1 with traits https://github.com/JuliaManifolds/ManifoldsBase.jl/pull/91/files#diff-781aa41b427ec28b133baaa8322416aa38ebc2f6324dc2d7a2cd46fda72f5491

IIRC last time I checked SimpleTraits there was some problem with it but I've just checked them again (mostly the macro expansion side) and it seems fine-ish.

I will check. The good thing is, that we do not need combinations like an EmbeddedGroupManifold (I think group manifold was something like that at least for a while) - sure we have to check it once I finished retractions and we actually can check. We definetly need one AbstractDecoratorManifold still, but we can more easily combine/mix decorators with the Traits approach.

For example SpecialOrthogonal is both embedded and group at the same time but that wouldn't be a problem I think. IIRC abstract group manifold was an abstract embedded for some time but it was not a good idea.

3. Layer 2 now with _ as isn line 274 https://github.com/JuliaManifolds/ManifoldsBase.jl/pull/91/files#diff-f5785906db07385d6161a45ba0c67152cb6854c110f0c981f96af26b3e8279d8

This looks fine with one minor point: do we want exp to be high-level or low-level?

Perfect. Since Exp does not have the necessity to resolve a parameter I think it is both. It is the high level function people should use (though it is a special case of retract as well), but it is also the low level one people should implement. Maybe it is a little bit more low level (since on the high level it would also be ExponentialRetraction). Similarly for parallel_transport, where I also like this approach, because it brings back that function name – which I find nice :)

OK, it should be fine to extend high-level functions for particular manifolds if no method/variant dispatch needs to happen.

Perfect, then I will continue this way (wanted to get this feedback first), of course I am not 100% sure this will work out completely, we will know later. But to me (now that I sketched the overall picture) it feels better than the previous approach, mainly due to the strict layers and that we fixed what should happen when and where. To me this order was not always clear in the previous modelling.

Tusen takk :)

Yes, I'm sure the plan will have to be tweaked a little as we go but it looks good.

Værsågod :slightly_smiling_face:

kellertuer commented 2 years ago

I would document it as that: This layer should not be touched unless one introduces a new retraction for example - one further reason to provide most retraction types already here in ManifoldsBase

Yes, this is a good explanation. I'd just split "touching" into "calling" and "extending". Extending is, as you wrote, for introducing new retraction types etc. while direct calling should only occur from high-level decorator dispatch functions.

Sure, touching was too informal, it will be documented more precisely.

For example SpecialOrthogonal is both embedded and group at the same time but that wouldn't be a problem I think. IIRC abstract group manifold was an abstract embedded for some time but it was not a good idea.

That was what I was looking for. This is a good point here, since it will easily be possible here with traits setting two of them to true. (The concrete cases of an EmbeddedManifold and a GroupdManifold will stay for sure, but the traits enable to “mix” the previous abstract types easily).

OK, it should be fine to extend high-level functions for particular manifolds if no method/variant dispatch needs to happen.

Thats well phrased, I will document this as well in this way.

Yes, I'm sure the plan will have to be tweaked a little as we go but it looks good.

Sure, we will see how this works out.

kellertuer commented 2 years ago

Just a small update today (did not get as far as I wanted): the retractions and the vector functions (basis, coordinates, vector) have now their additional (middle) Layer II, the docs are updated to reflect the discussed design. The tests are not yet updated.

Next part is to actually implement a few traits and get back to tests with the Decorator.

kellertuer commented 2 years ago

I got a little stuck today on bases functions. One problem is that number_of_coordinates is not called at a point where we have a basis, but on layer 3 we only have the number system of the basis left. Somehow sometimes the allocation function ids not hit and if the number of coordinates does not work and I can't get it to work (just tried for 90 minutes). I think I am mixing up the type of the NumberSystem and the actual instance itself. But its tricky.

These are (up to an ambituity that I just introduced in trying to fix this) all functions with parameters have their Layer 3 - so if the tests run again I start with the Traits.

mateuszbaran commented 2 years ago

Just a small update today (did not get as far as I wanted): the retractions and the vector functions (basis, coordinates, vector) have now their additional (middle) Layer II, the docs are updated to reflect the discussed design. The tests are not yet updated.

Next part is to actually implement a few traits and get back to tests with the Decorator.

Cool, I'll take a look at it.

I got a little stuck today on bases functions. One problem is that number_of_coordinates is not called at a point where we have a basis, but on layer 3 we only have the number system of the basis left. Somehow sometimes the allocation function ids not hit and if the number of coordinates does not work and I can't get it to work (just tried for 90 minutes). I think I am mixing up the type of the NumberSystem and the actual instance itself. But its tricky.

I'll take a look at this. number_of_coordinates can't be called without a basis (it doesn't even make sense to do so), so this looks weird.

mateuszbaran commented 2 years ago

I'll push some fixes once they are done.

mateuszbaran commented 2 years ago

I'm actually not sure about leaving only the number field for the lower level functions. It feels a bit forced in some cases, for example the diagonalizing basis.

kellertuer commented 2 years ago

I felt for the simple once it should be ok, for Cached that is not enough and we should still use the Basis itself, and I think for the Diagonalizing as well, you are right.

I think I fixed ValidationManifold so far. The new scheme (though I have to get a little used to implementing the allocating variant here and there) feels a little better/intuitive is my impression for now. Lets see how that continues through the Power Manifold and the decorators.

kellertuer commented 2 years ago

Found the culprit - the macro has to import stuff before redefining it. Now there are only a few tricky ones in power left and then the tests are running.

When not coding bugs, PowerTransportMethod could really be avoided, since we do not have that ambiguity anymore now, which is really nice since we could actually introduce the same defaults now as for normal Manifolds.

So up to the 4 remaining errors and this one point on fallbacks, we are finally at the decorator itself. I already have a few sketches on paper how to do this. Will maybe do a first session tomorrow.

kellertuer commented 2 years ago

So there is jus a get_vector left which seems to be tricky, since once it does not hit the right function and once it returns the wrong value, and I am for both not yet sure why.

edit: Found it and generalised it to _read_coordinatesand its write, this also avoids the artificialv_index` that we had.

kellertuer commented 2 years ago

The new (traits based) approach is sketched and implemented (without ambiguities) - it actually took far less lines of code than the old scheme. It is not yet tested and only half documented. I hope the current documentation at least gets the idea across. I will do both documentation and testing next.

Currently

I will also test this implementation by extending the example on how to implement a manifold. That way users can also directly see how to use the abstract decorator. Maybe I do a second one to also illustrate the sub manifold approach (maybe using symmetric manifolds or such).

kellertuer commented 2 years ago

I reintroduced EmbeddedManifold Tests and it feels quite nice to do these. Maybe we have to introduce a few more invokes, for the case where a Decorator does not have a certain trait. For now I just did these for checks; but they might be required for all other functions as well.

Before I do that, could you check what you think of this approach? It is done in its main features by now.

kellertuer commented 2 years ago

I updated the initial post to keep track of the todos. The tests are currently running but coverage is still a little low.

I encountered one thing that might be challenging with Traits – but also only appears since the current design in principle makes it possible for an AbstractDecorator to be two decorators at once, see

https://discourse.julialang.org/t/combining-simpletraits-jl-traits-on-one-type/73644

for the topic. I like the idea if that would be possible, if not, we‘d have to use AbstractEmbeddedManifold and such again. Maybe we can also use our own THTT instead and come up with a scheme that allows for such a combination, but then we have to solve the problem of precedence, i.e. of two Traits (abstract decorators) are active for the same function (as in the Thread above with the and case), which one wins.

The good news: Adapting the tests for EmbeddedManifold was deleting all dispatch tests (for symbols :transparent and changing all ErrorMessages to actual MethodErrors (which I really like) - ok and fixing a few bugs on the way. So the new model overall still feels better than the old one

mateuszbaran commented 2 years ago

Nice, great progress :slightly_smiling_face: . I've taken a short break for Christmas but I'll look at it now.

kellertuer commented 2 years ago

Don‘t worry, I did not expect any response over Christmas for sure. I also took some time off, when I was confused by how to create function names within a macro, but for basis functions this is now already done (retractions / vector transports still have to be done).

mateuszbaran commented 2 years ago

SimpleTraits.jl doesn't seem to support such complex trait patterns, see: https://github.com/mauro3/SimpleTraits.jl/pull/2 . I'm not sure what to do about it. See also: https://discourse.julialang.org/t/announcing-traits-jl-a-revival-of-julia-traits/35683/4?u=schlichtanders . But WhereTraits.jl (successor to Traits.jl) is quite a heavy dependency so I'd prefer to avoid it.

mateuszbaran commented 2 years ago

After thinking a bit about it I'm not actually sure if traits are what we need here. At least not that kind of traits. What do you think about this:

abstract type AbstractTrait end

struct EmptyTrait <: AbstractTrait end

struct NestedTrait{T1<:AbstractTrait, T2<:AbstractTrait} <: AbstractTrait
    t1::T1
    t2::T2
end

struct IsCool <: AbstractTrait end
struct IsNice <: AbstractTrait end

abstract type AbstractA end
abstract type DecoA <: AbstractA end
# A few concrete types
struct A <: AbstractA end
struct A1 <: DecoA end
struct A2 <: DecoA end
struct A3 <: DecoA end
struct A4 <: DecoA end

# just some function
f(::AbstractA, x) = x+2
g(::DecoA, x, y) = x+y

trait(args...) = EmptyTrait()
trait(::A1, ::Any) = NestedTrait(IsNice(), EmptyTrait())
trait(::A2, ::Any) = NestedTrait(IsCool(), EmptyTrait())
trait(::A3, ::Any) = NestedTrait(IsCool(), NestedTrait(IsNice(), EmptyTrait()))

_f(a, b) = __f(trait(a, b), a, b)
__f(::NestedTrait{IsNice}, a, b) = g(a, b, 3)
__f(::NestedTrait{IsCool}, a, b) = g(a, b, 5)
__f(t::NestedTrait, a, b) = __f(t.t2, a, b)
__f(::EmptyTrait, a, b) = f(a, b)

print("normal:$(_f(A(),0))") # prints: normal:2
println("cool $(_f(A2(),0))") #prints: cool: 5
println("both nice and cool $(_f(A3(),0))") # works
println("neither nice nor cool $(_f(A4(),0))") # works
println("nice $(_f(A1(),0))") # works

That's basically nested traits where we explicitly define the order in which they are supposed to be handled, and the order can be manifold-dependent and can be overwritten for a specific manifold or even other parameters. The trait function does the magic here. It should be fast thanks to constant propagation and inlining (we may need to add some @inline though). It may be possible to make the code a bit nicer but first I wanted to sketch my idea.

kellertuer commented 2 years ago

Thanks for the solution, that looks quite complicated, I have to check how this might work in practice, i.e. for 50+ functions and a few decorators. How would that work for more than 2 decorators? For me it looks like exponential growths in number of function definitions per decorator?

mateuszbaran commented 2 years ago

No, this actually explicitly avoids exponential growth. It's approximately linear with the number of traits for __f and at least linear for trait (depending on what you actually need it to do). If you describe the situation you want to be modeled in my scheme, you can describe it and I'll translate it to code.

kellertuer commented 2 years ago

I am not yet at the concrete level (for now my 3 traits are kind of hierarchical, the old embedding types that is), but the next step would be to combine this with other old abstract decorators, and then I would like to see how that works.

The problem here is that in ManifoldsBase we only have one decorator (embedded manifold) basically; but I also have to first understand your approach a little closer (and for example _f conflicts in naming maybe with our layer 2? Or is it layer 2 and with the trait upfront we avoid ambiguities? Still _f - via __f - calls first layer again, so it is maybe not really layer 2).

mateuszbaran commented 2 years ago

The problem here is that in ManifoldsBase we only have one decorator (embedded manifold) basically;

There is also ValidationManifold, right?

ut I also have to first understand your approach a little closer (and for example _f conflicts in naming maybe with our layer 2? Or is it layer 2 and with the trait upfront we avoid ambiguities? Still _f - via __f - calls first layer again, so it is maybe not really layer 2).

The naming here needs some work, this was just an example. Just using f should also be fine:

# generic code

abstract type AbstractTrait end

struct EmptyTrait <: AbstractTrait end

struct NestedTrait{T1<:AbstractTrait, T2<:AbstractTrait} <: AbstractTrait
    t1::T1
    t2::T2
end

@inline base_trait(args...) = EmptyTrait()

@inline merge_traits() = EmptyTrait()
@inline merge_traits(t::EmptyTrait) = t
@inline merge_traits(t::NestedTrait) = t
@inline merge_traits(t::AbstractTrait) = NestedTrait(t, EmptyTrait())
@inline merge_traits(t1::EmptyTrait, t2::EmptyTrait) = t1
@inline merge_traits(t1::EmptyTrait, t2::AbstractTrait) = t2
@inline merge_traits(t1::AbstractTrait, t2::EmptyTrait) = NestedTrait(t1, t2)
@inline merge_traits(t1::AbstractTrait, t2::AbstractTrait) = NestedTrait(t1, NestedTrait(t2, EmptyTrait()))
@inline merge_traits(t1::NestedTrait, t2::EmptyTrait) = t1
@inline merge_traits(t1::NestedTrait, t2::AbstractTrait) = NestedTrait(t1.t1, merge_traits(t1.t2, t2))
@inline merge_traits(t1::AbstractTrait, t2::AbstractTrait, t3::AbstractTrait, trest::AbstractTrait...) = merge_traits(merge_traits(t1, t2), t3, trest...)

@inline parent_trait(::AbstractTrait) = EmptyTrait()

@inline function trait(args...)
    bt = base_trait(args...)
    return expand_trait(bt)
end

@inline expand_trait(e::EmptyTrait) = e
@inline expand_trait(t::AbstractTrait) = _expand_trait(t, parent_trait(t))
@inline _expand_trait(t1::AbstractTrait, ::EmptyTrait) = t1
@inline _expand_trait(t1::NestedTrait, ::EmptyTrait) = t1
@inline function _expand_trait(t1::NestedTrait, t2::AbstractTrait)
    et1 = expand_trait(t1.t1)
    et2 = expand_trait(t1.t2)
    return merge_traits(et1, et2, t2)
end
@inline function expand_trait(t::NestedTrait)
    et1 = expand_trait(t.t1)
    et2 = expand_trait(t.t2)
    return merge_traits(et1, et2)
end

# Example usage

struct IsCool <: AbstractTrait end
struct IsNice <: AbstractTrait end

abstract type AbstractA end
abstract type DecoA <: AbstractA end
# A few concrete types
struct A <: AbstractA end
struct A1 <: DecoA end
struct A2 <: DecoA end
struct A3 <: DecoA end
struct A4 <: DecoA end
struct A5 <: DecoA end

# just some function
f(::AbstractA, x) = x+2
g(::DecoA, x, y) = x+y

struct IsGreat <: AbstractTrait end # a special case of IsNice
parent_trait(::NestedTrait{IsGreat}) = IsNice()

base_trait(::A1, ::Any) = merge_traits(IsNice())
base_trait(::A2, ::Any) = merge_traits(IsCool())
base_trait(::A3, ::Any) = merge_traits(IsCool(), IsNice())
base_trait(::A5, ::Any) = merge_traits(IsGreat())

f(a::DecoA, b) = f(trait(a, b), a, b)

f(::NestedTrait{IsNice}, a, b) = g(a, b, 3)
f(::NestedTrait{IsCool}, a, b) = g(a, b, 5)

# generic forward to the next trait to be looked at
f(t::NestedTrait, a, b) = f(t.t2, a, b)
# generic fallback when no traits are defined
f(::EmptyTrait, a, b) = invoke(f, Tuple{AbstractA, typeof(b)}, a, b)

print("normal:$(f(A(),0))") # prints: normal:2
println("cool $(f(A2(),0))") #prints: cool: 5
println("both nice and cool $(f(A3(),0))") # works
println("neither nice nor cool $(f(A4(),0))") # works
println("nice $(f(A1(),0))") # works

println("great $(f(A5(),890))") # fallback
f(::NestedTrait{IsGreat}, a, b) = g(a, b, 54)
println("great $(f(A5(),890))") # new method

I've extended it to support hierarchical traits (parent_trait defines the parent; you can even do multiple inheritance). I can make the syntax for definitions a bit nicer if you want to follow this approach (mostly NestedTrait(IsCool(), NestedTrait(IsNice(), EmptyTrait())) could be something like [IsCool(), IsNice()]).

mateuszbaran commented 2 years ago

The idea is that f(a::DecoA, b) = f(trait(a, b), a, b) computes trait hierarchy for the current method call through trait(a, b) and proceeds with the dispatched hierarchy. trait returns essentially a list of traits (each node is a NestedTrait), and when overloading methods for a trait you just look at the head of the list (though you can look further if you really need). If head doesn't match any specializations, then it is cut off by f(t::NestedTrait, a, b) = f(t.t2, a, b). If the entire list of traits is exhausted, the fallback case is called by f(::EmptyTrait, a, b) = invoke(f, Tuple{AbstractA, typeof(b)}, a, b). Most of the first part of my last code is about properly flattening multiple hierarchies of traits into a single list.

kellertuer commented 2 years ago

Supernice! I like the modelling and the idea, also the updated naming, I think that is the scheme we want to go for. Thanks! (Might take a few days until I test that on the real code, since I am at the Chaos Communication Congress (remotely) these days).

mateuszbaran commented 2 years ago

I've improved my example a bit. I may still have to fight a bit to make it actually inline and constprop properly, Julia inliner doesn't like recursive methods.

I'm not sure if we would need function-dependent traits but it would also be easily possible (f(a::DecoA, b) = f(trait(a, b), a, b) can be changed to f(a::DecoA, b) = f(trait(f, a, b), a, b)).