search

JuliaManifolds / ManifoldsBase.jl

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

Define Meta Manifolds already in ManifoldsBase #169

Closed kellertuer closed 9 months ago

kellertuer commented 9 months ago

This allows the user to even use a TangentSpaceAtPoint when only depending on ManifoldsBase. I would like to have this for defining proper/nice/generic subproblems in tangent spaces in Manopt.jl (e.g. for the current trust region rework).

This is breaking (hence also for 0.15) in the sense that with this version otherwise the VectorBundle is defined twice (which kills precompiation).

To do

codecov[bot] commented 9 months ago

Codecov Report

Merging #169 (ef36dc7) into master (ba4a2ff) will increase coverage by 0.00%. The diff coverage is 99.84%.

:exclamation: Current head ef36dc7 differs from pull request most recent head fb37b01. Consider uploading reports for the commit fb37b01 to get more accurate results

@@           Coverage Diff            @@
##           master     #169    +/-   ##
========================================
  Coverage   99.96%   99.96%            
========================================
  Files          20       26     +6     
  Lines        2562     3160   +598     
========================================
+ Hits         2561     3159   +598     
  Misses          1        1
Files Coverage Δ
...rayToolsExt/ManifoldsBaseRecursiveArrayToolsExt.jl 100.00% <100.00%> (ø)
...yToolsExt/ProductManifoldRecursiveArrayToolsExt.jl 100.00% <100.00%> (ø)
src/DefaultManifold.jl 100.00% <100.00%> (ø)
src/Fiber.jl 100.00% <100.00%> (ø)
src/ManifoldsBase.jl 100.00% <100.00%> (+0.39%) :arrow_up:
src/PowerManifold.jl 100.00% <100.00%> (ø)
src/TangentSpace.jl 100.00% <100.00%> (ø)
src/VectorFiber.jl 100.00% <100.00%> (ø)
src/bases.jl 100.00% <100.00%> (ø)
src/point_vector_fallbacks.jl 100.00% <100.00%> (ø)
... and 4 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

mateuszbaran commented 9 months ago

Hmm... since it's breaking, maybe let's move the implementation of fiber bundles from https://github.com/JuliaManifolds/Manifolds.jl/pull/642 here? And Product manifolds as well.

kellertuer commented 9 months ago

Sure, why not. I was not aware you worked on the Fibre Bundle in that PR.

mateuszbaran commented 9 months ago

It's in the PR's title :smile:

mateuszbaran commented 9 months ago

Will you make the update or prefer me to do that?

kellertuer commented 9 months ago

Since those are your changes (and it is harder for me to track them down in your PR), maybe you could add them here? Feel free to add product and power as much as possible here, too, I'd love to have them here as well.

mateuszbaran commented 9 months ago

OK, sure, I will do it then.

kellertuer commented 9 months ago

What do you think about the extension here? Or should the few functions using ArrayPartitions stay in Manifolds instead, where we have that dependency anyways?

mateuszbaran commented 9 months ago

I prefer to have that extension here.

kellertuer commented 9 months ago

Nice, I do not have a strong preference, but I agree, that this puts all VectorBundle-Fun in one place. Then Iwe should add the Revise dependency here as well (that's what's missing still).

kellertuer commented 9 months ago

I just spend quite a bit of time with the docs. Citations should be fine already – but the docs still have 8 doctorings that we never included somewhere. I only fixed about a few and these are still missing. Test coverage – I started with that but I am not yet sure how far I got.

kellertuer commented 9 months ago

Maybe we should also rethink the docs a bit when fixing the remaining ones – the manifolds section is by now very long – and might be better to be restructured.

mateuszbaran commented 9 months ago

Maybe let's update docs when new things are ready? I'm in the middle of moving metamanifolds here.

kellertuer commented 9 months ago

Sure, that's one of the reasons I waited with fixing :)

kellertuer commented 9 months ago

It seems for now that flat and sharp were defined in Manifolds.jl only, shall we move them over as well?

edit1: Same for flat and the RiesRepresenterCotangent stuff...

edit2: I am confused of the errors Documenter throws. I now defined sharp/flat like all other interface functions and properly exported them but Documenter still throws about a million (ok maybe I am exxagarating) errors like

┌ Error: @autodocs (src/bases.md:23-27) encountered a bad docstring binding 'ManifoldsBase.sharp'
│ ```@autodocs
│ Modules = [ManifoldsBase]
│ Pages = ["bases.jl"]
│ Order = [:type, :function]
│ ```
│ This is likely due to a bug in the Julia docsystem relating to the handling of
│ docstrings attached to methods of callable objects. See:
│ 
│   https://github.com/JuliaLang/julia/issues/45174
│ 
│ As a workaround, the docstrings for the functor methods could be included in the docstring
│ of the type definition. This error can also be ignored by disabling strict checking for
│ :autodocs_block in the makedocs call with e.g.
│ 
│   strict = Documenter.except(:autodocs_block)
│ 
│ However, the relevant docstrings will then not be included by the @autodocs block.
│   exception = UndefVarError: `sharp` not defined
└ @ Documenter ~/.julia/packages/Documenter/9kOxY/src/utilities/utilities.jl:44

at me but sharpnow is defined! I can try to spent another (second) hour on this, but maybe that is not so useful? Especially all this follows now the same way our other definitions are done and they do not cause errors. Very strange.

mateuszbaran commented 9 months ago

It seems for now that flat and sharp were defined in Manifolds.jl only, shall we move them over as well?

edit1: Same for flat and the RiesRepresenterCotangent stuff...

Yes, my mistake, I've moved too much here. flat, sharp and cotangent stuff should not be here.

kellertuer commented 9 months ago

I would not mind having them here, but sure we can also move them back, since they are only needed for the cotangent parts and those are not yet so mature I think, that they should be moved here. Could you move that back?

mateuszbaran commented 9 months ago

I've moved them back to Manifolds.jl

mateuszbaran commented 9 months ago

List of documentation issues looks sane now :slightly_smiling_face:

kellertuer commented 9 months ago

Yes, and I also have a idea (that I started as well) how to restructure the docs – and I will check the docs we moved whether they are detailed enough; hopefully will find a bit of time tomorrow for that.

Thanks :)

mateuszbaran commented 9 months ago

Cool, I'll check that when it's ready :slightly_smiling_face:

kellertuer commented 9 months ago

Woah! We made it to all-lines-covered? That's awesome.

kellertuer commented 9 months ago

I reworked retractions already, so that all generic dispatch and types are (as before) in the retractions file; I will do the same for the inverse retractions and the vector transport.

There are two missing Docs Errors that I could not yet narrow down and it seems the Fiber.jl docs are included twice somewhere and I can not see where that happens? Will have to check for that as well.

When there are no errors left I would still like to check those docs that are really short and provide a bit more details there, For example 


"""
    TensorProductType(spaces::VectorSpaceType...)

Vector space type corresponding to the tensor product of given vector space
types.
"""
struct TensorProductType{TS<:Tuple} <: VectorSpaceType
    spaces::TS
end

I have no clue what this type is for and that is does, the first line is not the type but the constructor and the sentence in the docs does not help me to understand this either.

mateuszbaran commented 9 months ago

Sounds good. TensorProductType is still experimental so I will move it back to Manifolds.jl.

mateuszbaran commented 9 months ago

Sorry for so many comments but I am a bit lost between fiber bundles, bundle fibres, vector bundles and vector bundle fibers and their correspoding types... it also seems you have either switched order of these words somewhen or added Fiber before bundle or so at some point?

Hm, I can try to explain but I'm not sure how much you understand the old VectorBundle design? The only change is that I made VectorBundle a special case of a more general FiberBundle, for cases where fibers are not vector spaces.

kellertuer commented 9 months ago

I think there is two things I miss a bit in the docs. Let's take vector bundle as an example

1) The docs should have a generic description of what that is and how it can be used (e.g. how the constructor works) I think that is missing currently 2) The docs should describe how that is technically (programmatically) realised (maybe a centre part between the generic description and the constructor), that part I think I understand to some extend – besides a few bit strange mixups in names?

Maybe I also just got lost for now in vector bundle, fiber bundle, bundle fiber, and all these types with an added Type which for tangent space is again consted to something without type. That is really where I got lost.

kellertuer commented 9 months ago

I carefully read the code for the Fiber bundle (to not be to special already when looking at the tangent bundle) and tried extending the docs. I ran into a few questions, since I feel we store a few tings 2-3 times

  1. Why do we need to store FiberBundle.type is it not enough to have that as a parameter of the fiber bundle? For dispatch we would just need it as a parameter.
  2. Why do we beed to store a single fiber (I would prefer to rename BundleFibers to just Fiber)
  3. Why does the fiber again store the manifold and the type? Is that not double?

If we remove the fiber field from FiberBundle the fiber bundle would still contain the same information – it knows its type and its manifold. Or in other words, the only thing that 3 does is introduce redundancy (no inconsistency, since you took the same bound type).

Also in the fiber bundle the fiber type is in the field type while in the BundleFiber it is the field fiber ?

So I would maybe do 

struct FiberBundle{
    𝔽,
    TF<:FiberType,
    TM<:AbstractManifold{𝔽},
    #TVT<:FiberBundleProductVectorTransport,
} <: AbstractManifold{𝔽}
    manifold::TM
    #vector_transport::TVT
end

and 

struct Fiber{TF<:FiberType,TM<:AbstractManifold, P}
    manifold::TM
    p::P
end

But here even further – isn't Fiber (like TangentSpace) also a manifold? And should it therefore not also have a 𝔽 field upfront?

I am still not sure why the fiber bundle has to store its vector transport (we also do not do that for other manifolds). So we could maybe even leave that out as well? any vector transport call would either use the default (where I would store the one you currently want to store in the fiber bundle) or specify another one.

I hope this is reasonable – and if so, I am happy to rework this and write the docs, because if my concerns here are right, I think I understand the concept :)

edit: Hm, now I am confused again. Why do we have FiberBundle (the large thing) and BundleFibers (which I thought would be a single one?) but also FiberAtPoint? I am now again lost what this “middle type” is for. To me BundleFibers seems to be just a FiberBundle without the vector transport? But it is then also contained in the FiberBundle? Why? Russian Matroska design pattern?

mateuszbaran commented 9 months ago

If we remove the fiber field from FiberBundle the fiber bundle would still contain the same information – it knows its type and its manifold. Or in other words, the only thing that 3 does is introduce redundancy (no inconsistency, since you took the same bound type).

IIRC some redundancy is there because it was easy to just do M.fiber rather than having a code that reconstructs the object and worry about performance of said reconstruction. Maybe it's no longer an issue, I will investigate.

I am still not sure why the fiber bundle has to store its vector transport (we also do not do that for other manifolds).

This is actually a bit underdeveloped part. Essentially, we want some structure on a fiber bundle -- at least a connection. This "vector transport" is actually about specifying what connection should be used. It's usually restricted to principal and vector bundles (see https://ncatlab.org/nlab/show/connection+on+a+bundle but please don't scroll too far down :sweat_smile: ) but there may be valid examples of connections outside of these two types.

edit: Hm, now I am confused again. Why do we have FiberBundle (the large thing) and BundleFibers (which I thought would be a single one?) but also FiberAtPoint? I am now again lost what this “middle type” is for. T

I will try to simplify this part -- I don't like it either.

kellertuer commented 9 months ago

If we remove the fiber field from FiberBundle the fiber bundle would still contain the same information – it knows its type and its manifold. Or in other words, the only thing that 3 does is introduce redundancy (no inconsistency, since you took the same bound type).

IIRC some redundancy is there because it was easy to just do M.fiber rather than having a code that reconstructs the object and worry about performance of said reconstruction. Maybe it's no longer an issue, I will investigate.

I am happy to help with docs, but for that I would like to know why and when redundancy is necessary, to me it seems like the above idea with 2 types is not only clearer and closer to there manifolds, but also simpler in many places.

I am still not sure why the fiber bundle has to store its vector transport (we also do not do that for other manifolds).

This is actually a bit underdeveloped part. Essentially, we want some structure on a fiber bundle -- at least a connection. This "vector transport" is actually about specifying what connection should be used. It's usually restricted to principal and vector bundles (see https://ncatlab.org/nlab/show/connection+on+a+bundle but please don't scroll too far down 😅 ) but there may be valid examples of connections outside of these two types.

Hm but manifolds we also equip with a metric a connection a vector transport and so on … all of that implicitly or with the default_ functions. Why is that then not possible for fiber bundles? Or phrased differently – is there a good reason to deviate from the default design pattern all of our manifolds and meta manifolds follow until now?

edit: Hm, now I am confused again. Why do we have FiberBundle (the large thing) and BundleFibers (which I thought would be a single one?) but also FiberAtPoint? I am now again lost what this “middle type” is for. T

I will try to simplify this part -- I don't like it either.

Then let‘s not do it and remove this „middle type“

mateuszbaran commented 9 months ago

Hm but manifolds we also equip with a metric a connection a vector transport and so on … all of that implicitly or with the default_ functions. Why is that then not possible for fiber bundles? Or phrased differently – is there a good reason to deviate from the default design pattern all of our manifolds and meta manifolds follow until now?

One way to look at it is that it's mostly to not bother with a decorator struct for changing the connection. I'm really not excited about having to design another decorator just to be able to change the connection on fibers when there is a very simple alternative.

kellertuer commented 9 months ago

Hm, because with the two types I sketched above I think I could easily implement most functions that are there in the new form.

On the other hand, the connection decorator will be needed at some point in time (analogously to the metric decorator) anyways. And then there is my German-ness that likes to stick to design rules and ideas – and with these your idea of “this is the easy way to do this for now” conflicts a bit. But I do agree there is a bit of a tradeoff.

kellertuer commented 9 months ago

I feel my approach might be minimalistic (though sticking more to the other manifolds in struct design).

If you feel this is too much work on your side, I could reduce the current PR to what I think should suffice (including docs and testing) and we go from there add what you think is missing.

mateuszbaran commented 9 months ago

If you want it done quickly, then perhaps the best way to go about it is to keep fiber and vector bundles in Manifolds.jl for now? Fibers and tangent spaces are non-controversial so if that would be enough for Manopt then it might be a good compromise. And of course this PR also brings ProductManifold to ManifoldsBase.jl.

kellertuer commented 9 months ago

I am not super in a hurry, just that we have PRs open that depend on this. But sure I would even be fine just having the tangent space here (though also having tangent bundle then would be nice) and we could keep the fibers in Manifolds.jl for now?

Though that would also split a few things I think. But since the design seems to not be finished, maybe really leaving out fibers here is a good idea – what I actually need is the tangent space – and it would be nice to just have it as TangentSpace(M,p) then.

mateuszbaran commented 9 months ago

But sure I would even be fine just having the tangent space here (though also having tangent bundle then would be nice) and we could keep the fibers in Manifolds.jl for now?

It would be great if we could have FiberAtPoint (or just call it Fiber) here, and make TangentSpace a special case of Fiber. This would save us another rework when fiber bundles are finished. BundleFibers and FiberBundle wouldn't be included here.

Though that would also split a few things I think. But since the design seems to not be finished, maybe really leaving out fibers here is a good idea – what I actually need is the tangent space – and it would be nice to just have it as TangentSpace(M,p) then.

OK, TangentSpace name is free now so we can use it instead of TangentSpaceAtPoint.

kellertuer commented 9 months ago

Ok, that we can do, but then also TangentBundle would not yet be moved here (since it is a special case of the VectorBundle)? That would also be fine with me then.

mateuszbaran commented 9 months ago

Yes, TangentBundle would remain in Manifolds.jl for now.

kellertuer commented 9 months ago

That's fine with me. I will rework this PR – hopefully tomorrow evening.

Before then registering 0.15, should we also fix #135 (maybe also the parts of https://github.com/JuliaManifolds/Manifolds.jl/issues/630 that can be resolved here)?

mateuszbaran commented 9 months ago

OK. I will do a bit of work on this PR today too.

135 sounds like an easy change so it would be a good idea. I'd postpone https://github.com/JuliaManifolds/Manifolds.jl/issues/630 though because figuring out the right tolerances can be tricky.

kellertuer commented 9 months ago

Ok, fair enough :)

edit: I will not find much time today.

mateuszbaran commented 9 months ago

I did the rework. I've kept fiber_type in Fiber because it doesn't take any memory when it's an empty object anyway, and it's easier to pass it somewhere.

mateuszbaran commented 9 months ago

I haven't properly updated docs and show methods yet though.

kellertuer commented 9 months ago

I did the rework. I've kept fiber_type in Fiber because it doesn't take any memory when it's an empty object anyway, and it's easier to pass it somewhere.

I want to check, whether I like all those places ;) But sure, if that is reasonable, we can keep it as a field, though I do like it as only a parameter ,maybe a bit more.

I haven't properly updated docs and show methods yet though.

Sure, I can work on those tomorrow :)

kellertuer commented 9 months ago

...and it is even non-breaking? That's nice, though I forgot what the braking one is waiting for (besides maybe also resolving the one issue we mentioned already above).

I think a few variables could be unified and will check more documentation hopefully later today.

mateuszbaran commented 9 months ago

This could maybe be considered technically non-breaking but new features here require breaking changes in Manifolds.jl to be usable and we drop Julia 1.0 here. I'd prefer to release this as a part of v0.15.

kellertuer commented 9 months ago

hm oh, but you just changed the version from 15.0 to 14.x?

mateuszbaran commented 9 months ago

Sorry, didn't mean to push it :/ . It was meant to be local to make resolve work in my environment.

kellertuer commented 9 months ago

That is fine, then you just confused me with that version number – I am of course also fine with a 15.0 version here.

kellertuer commented 9 months ago

I worked a bit further on the docs, but I am again lost on quite a last reference left to [TangentBundle](@ref) for the SasakiRetraction I am unsure how to rephrase this.

It also seems we introduce the CotangentVectorSpaceType but no CotangentSpace is that intentional? I think we could also introduce the cotangent space if we have the vector space type for the fiber already?

Those are the last two errors left before the docs render. Then I will check the strings further and which to add maybe

mateuszbaran commented 9 months ago

I worked a bit further on the docs, but I am again lost on quite a last reference left to [TangentBundle](@ref) for the SasakiRetraction I am unsure how to rephrase this.

Either remove the reference or link to Manifolds.jl docs?

It also seems we introduce the CotangentVectorSpaceType but no CotangentSpace is that intentional? I think we could also introduce the cotangent space if we have the vector space type for the fiber already?

CotangentSpaceType has been here for some time but I actually don't need CotangentSpace so I didn't think about it. Feel free to add it.