Delete Euclidean

[kellertuer]

This PR deletes the Euclidean Manifold and adds a (more minimal) Default Manifold, that is as short as possible, such that a new user can get started.

I also extended the tests to also cover geodesics.

I noticed, that the non mutating exp, log,... face the challenge, that similar_result will not work for the Point/TVector subtypes (see extended tests for the empty_manifold. Any ideas how we could unify array and typed approaches to manifolds with respect to the similar_result function?

[mateuszbaran]

Nice work!

I noticed, that the non mutating exp, log,... face the challenge, that similar_result will not work for the Point/TVector subtypes (see extended tests for the empty_manifold. Any ideas how we could unify array and typed approaches to manifolds with respect to the similar_result function?

similar_result should work as long as MPoint/TVector subtypes have eltype and similar defined (which I think is a good idea anyway).

[kellertuer]

similar_result should work as long as MPoint/TVector subtypes have eltype and similar defined (which I think is a good idea anyway).
Ah, yes, you're right, at least if one encapsulates properly, I think, so

struct MyMPoint <: MPoint
    data::Vector{Float64}
end

woudl require and additional line eltype(::MyPoint) = Float64 for example, and similar would return a similar MyPoint then, right? That is a good idea, meybe to be even included into the DefaultManifold as an illustration.

[mateuszbaran]

Yes, a full example would be something like

struct MyMPoint{T} <: MPoint
    data::Vector{T}
end
Base.eltype(p::MyMPoint) = eltype(p.data)
Base.similar(p::MyMPoint, ::Type{EltypeNew}) where EltypeNew = MyMPoint(similar(p.data, EltypeNew))
Base.similar(p::MyMPoint) = MyMPoint(similar(p.data))

It could definitely be included in DefaultManifold.

[codecov]

Codecov Report

Merging #8 into master will increase coverage by 6.99%.
The diff coverage is 97.47%.

@@            Coverage Diff            @@
##           master      #8      +/-   ##
=========================================
+ Coverage    91.5%   98.5%   +6.99%     
=========================================
  Files           2       3       +1     
  Lines         106     201      +95     
=========================================
+ Hits           97     198     +101     
+ Misses          9       3       -6

Impacted Files	Coverage Δ
src/ManifoldsBase.jl	100% <100%> (+7.86%)	⬆️
src/DefaultManifold.jl	100% <100%> (ø)
src/ArrayManifold.jl	96.66% <96.66%> (ø)

---

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 fe36f66...611a04b. Read the comment docs.

[kellertuer]

We could maybe still extend the example on the new Types by test cases. And elaborate on that in the documentation later on, how such types are meant to be used. I only fear that the inplace methods might not work, since .= does not work for such types? Or can that be “passed down” to the .data field?

[kellertuer]

I think when a few tests for the typed data, are finished, we can merge this.

[mateuszbaran]

We could maybe still extend the example on the new Types by test cases. And elaborate on that in the documentation later on, how such types are meant to be used. I only fear that the inplace methods might not work, since .= does not work for such types? Or can that be “passed down” to the .data field?

This is a bit complicated. On one hand, broadcasting can be passed into the .data field without much work in typical cases. The ArrayAndChar example from documentation shows how to do it. On the other hand, sometimes this won't work (when the internal representation is more complex). Another thing is that Zygote compatibility JuliaManifolds/Manifolds.jl#42 essentially requires a separate implementation of non-mutating operations.

I think when a few tests for the typed data, are finished, we can merge this.

Sure. So, you want to add broadcasting support for typed data? I think we can't generally expect that all typed data will be broadcastable. copyto! should usually work though. You can also take a look at ProductArray and ProductRepr from Manifolds.jl: https://github.com/JuliaNLSolvers/Manifolds.jl/blob/master/src/ProductRepresentations.jl

[kellertuer]

For wuite some cases I am fine with another implementation. One thing I want to try is, provide types for SPD, that store U,S,V from the SVD and check whether that can be faster. So with that I am not saying we should in general pass down broadcasting (and I haven't read much about Zygote yet), I was just wondering, what the nicest way is to show the user of this interface in the DefaultManifold how the typed data should be used.

I am not sure whether I want to add broadcasting for type data in general, though. I was just thinking about – as I said – what the nicest way is to equip the small example here with typed functions (exp,log,distance,norm,inner, and vector_transport I think?); and shortly document that Idea (in the readme as long as our doc is not that long).

edit: Ah I think I understood how it works and it might be easier to write the aforementioned functions shortly again for fixed types, compared to the necessarxy things for all three types to be able to broadcast and stuff (including the problem that log anyways has to “know” the type of tangent vector). I will propose something, hopefully tomorrow, to finish this PR.

[mateuszbaran]

I'm not sure if there is one correct way of using typed points and vectors. Manifolds.jl has ArrayManifold, FVector, ProductRepr and ProductArray, each one addresses a different problem. Maybe it would be enough to refer readers to these examples and describe their design? I could make such description. Probably ArrayManifold is the closes to what you want for DefaultManifold?

One thing I want to try is, provide types for SPD, that store U,S,V from the SVD and check whether that can be faster.

This will heavily depend on the size of matrices. Up to 3x3 I wouldn't expect much improvement in most cases since StaticArrays has really fast eigendecomposition of such small matrices. For larger matrices is seems like a good idea though.

[kellertuer]

I might have been a little tired tomorrow; actually what I had in mind for MatrixManifoldis, what ArrayManifold already does and DefaultManifold just would have to subtype from that. So then … do we move ArrayManifold here or do we keep it in Manifolds? I am not yet completely sure how much the interface should take over at that point.

Concerning SPDs – yes for 3x3 I also don't expect much of a gain.

[mateuszbaran]

So then … do we move ArrayManifold here or do we keep it in Manifolds? I am not yet completely sure how much the interface should take over at that point.

I think we can keep ArrayManifold in Manifolds.jl then. At least for now.

[kellertuer]

I just shortly checked locally – extending DefaultManifold to be similar to an ArrayManifold means copying a lot of stuff from that while copying ArrayManifold reduces the DefaultManifold even further (no need for the point/tangent/cotangent nor the similar/copy functions). That's why I would opt for transferring ArrayManifold but I'll wait until we have discussed that in #10.

Despite this change (both variants of which I now can easily make, though the first is a little more copying) this PR is hence done.

[kellertuer]

With the addition of ArrayManifold to illustrate semantic types (MPoint, TVector, and CoTVector) I think this PR is finished.

[mateuszbaran]

Great, I think this PR looks fine except ArrayManifold should be updated to the new vector transport interface.

[kellertuer]

Good observation, added the method, added the along variant and noticed the non-mutating one could be omitted since it should work directly with the original interface.

[kellertuer]

I carefully implemented more tests for the ArrayManifold but now I really think this PR is finished and I would like to merge, let's say tomorrow.