Atlases and charts

[mateuszbaran]

This is supposed to provide minimal support for atlases and charts, mainly aimed at solving out current problems with metric manifold being inconsistent about it. This should solve #6 .

One point where I'm not sure what to do is flat and sharp. We don't have any specific representation of cotangent vectors. They can be represented by either functions, functors or coordinates in a basis of the cotangent space, and

Manifolds.jl/src/manifolds/MetricManifold.jl

Lines 336 to 345 in 77aecb4

	@decorator_transparent_fallback function flat!(
	M::MetricManifold,
	ξ::CoTFVector,
	p,
	X::TFVector,
	)
	g = local_metric(M, p)
	copyto!(ξ.data, g * X.data)
	return ξ
	end

works only in one very specific case. The question here is: how do we know which basis? My first idea is to store the basis in FVector but I'd like to hear your opinion.

[kellertuer]

Concerning flat!, I think we could do something similar to what we do with MPoint and TVector, i.e. if you just provide an array we treat it as an default cotangent representation, which is an array (as we do with vector) and I would introduce a type for the linear function representation (maybe similar to the Dual type?).

[mateuszbaran]

Well, I'm currently leaning towards making functions/functors the default cotangent vector representation (see #261 ), and arrays of coefficients as an alternative. Then flat and sharp have many possible variants, depending on received and target representation of tangent and cotangent vectors. Some problems would be solved by storing the basis in FVector, i.e. we would have the same number of arguments in all variants.

[kellertuer]

That sounds reasonable, too. Looking forward to seeing that sketch :)

[mateuszbaran]

flat and sharp work more or less as intended now on Circle, what do you think about this approach?

[kellertuer]

flat and sharp work more or less as intended now on Circle, what do you think about this approach?

Looks nice to me; just a question that came to my mind: Is there or could we introduce a way to automatically convert array<->Vector? In all other parts of the package we do not constrain tangent vectors, but here we do. Maybe that's necessary, I am not yet 100% sure, but it would be nice if those could work easily, i.e. with auto conversion.

[mateuszbaran]

There are a few separate issues.

Is there or could we introduce a way to automatically convert array<->Vector?

What do you mean by array <-> vector conversion? Conversion between the default representation of a tangent vector and a vector of its coefficients in a basis? Any such conversion requires manifold and a point which are always stored separately.

In all other parts of the package we do not constrain tangent vectors, but here we do.

I'd say that all other parts of the package don't really support FVectors, and I think FVectors are primarily for dealing with vector representation in a basis. Currently you can give nothing as a basis and then it is supposed to mean the default representation but maybe that's more confusing than helpful. I don't see here any constraining other than special-casing FVectors and changing default cotangent vector representation.

[kellertuer]

Currently we often have X just as an array (say a vector for the sphere), but for flatland sharpwe need them specifically asTFVectors. Ah I see for flat you do also accept arrays. My main goal here is, that if you stumble upon using sharp and flat, it would be nice if for start you can just plug in what you know already, i.e. your normal X` (for flat that is). I am not sure whether something that easy is possible for sharp, when or cotangent vectors are functions by default and not vectors.

But due to the duality pairing I sometimes think the same array representation (i.e. not yet a coordinate vector in a basis) as for tangents should be possible for cotangents. And it would be nice to have an array typed function for them as well? It maps to some default of course, as does the flat with an array X (maybe a default that even returns an array?).

Again this is just to help users enter our regime of the sharp/flat world, which I really like to have, since I think there is no package yet doing that rigorously.

[mateuszbaran]

Currently we often have X just as an array (say a vector for the sphere), but for flatland sharpwe need them specifically asTFVectors. Ah I see for flat you do also accept arrays. My main goal here is, that if you stumble upon using sharp and flat, it would be nice if for start you can just plug in what you know already, i.e. your normal X` (for flat that is). I am not sure whether something that easy is possible for sharp, when or cotangent vectors are functions by default and not vectors.

Yes, you can just put an array in flat and by default you will get a RieszRepresenterCotangentVector. The other way, i.e. being able to give any function to sharp should be possible but it's not as easy, so for now only RieszRepresenterCotangentVector (or CoTFVector) are accepted. I think sharping an arbitrary function is beyond the scope of this PR.

But due to the duality pairing I sometimes think the same array representation (i.e. not yet a coordinate vector in a basis) as for tangents should be possible for cotangents. And it would be nice to have an array typed function for them as well? It maps to some default of course, as does the flat with an array X (maybe a default that even returns an array?).

Could you give an example of what for example ξ[i] would be for the array representation of cotagnents, for example on S^2?

[kellertuer]

Thanks, that's already great. For sharp I was thinking of the following: If you have X in some TpM on S2, then you can take (in the embedding, R3), x -> dot(X,x) as a Dual, but this can also be uniquely represented by X (using the default ONB). so setting ξ=X would be reasonable and an array representation of the cotangent vector. Sure, such a default has to be documented.

So if that such a default is not too dangerous, I think using that (similar to the flat case) would be nice for users stating here.

[mateuszbaran]

For now I've removed size and indexing into FVector. I've also removed the nothing basis thing for FVector so that FVector is only for coefficients in a basis. I think the interface is cleaner now.

[kellertuer]

Furthermore for

local_metric(M::MetricManifold{𝔽<::AbstractManifold,EuclideanMetric}, p, B::InducedBasis{𝔽,TangentSpaceType,&lt;:RetractionAtlas})

and the corresponding inverse local metric you should add a test.
The InducedBasis is also currently not exported.

[kellertuer]

Finally induced_basis should also be covered (both variants) with a test?

edit: det_local_metric is now untested in power and product manifold, and I have not yet seen why. I think that's the main tree points I miss (local_metric above, induced_basis and said det_local_metrics).

[kellertuer]

I hopefully finished this nearly – see the two remarks at the code.
I have one further (hopefully really final) question.

Can we call get_point_coordinates also just get_coordinates? That would be neatly symmetric:

• get_coordinates/get_vector have the same signature (M, p, X, B)/(M,p,a,B) when acting on TpM
• get_coordinates/get_point have the same signature (M, A, i, p)/(M,A,i,a)

So similar to project I think get_coordinates should be clear from the signature, which coordinates one wants to obtain (first case: M,p and a basis so its a tangent vector, second case M,A,i indicates the chart, so its a point).

[mateuszbaran]

previously entered check_tangent_vector, since the base point check was only done inside. Now this is all done in a central place (good!) so in order to actually enter check_vector we have to use a correct point (dimension and values), for example is_vector(M, [0.0, 0.0], 0.0, true) triggers the right error that X is of wrong size.

I think that might be the reason for the other missing tests, too.

Yes, this makes sense 👍 .

Furthermore for

local_metric(M::MetricManifold{𝔽<::AbstractManifold,EuclideanMetric}, p, B::InducedBasis{𝔽,TangentSpaceType,&lt;:RetractionAtlas})

and the corresponding inverse local metric you should add a test.
The InducedBasis is also currently not exported.

I haven't exported InducedBasis yet because I'm not sure it's the right user-facing abstraction (maybe we should rely more on DefaultOrthogonalBasis than on InducedBasis? I'm not sure). I'll take a look at that method.

Finally induced_basis should also be covered (both variants) with a test?

edit: det_local_metric is now untested in power and product manifold, and I have not yet seen why. I think that's the main tree points I miss (local_metric above, induced_basis and said det_local_metrics).

For power and product we should have many more metric-related tests and functionality anyway. I could delete them for now.

I hopefully finished this nearly – see the two remarks at the code.
I have one further (hopefully really final) question.

Thanks for your help! I'll take a look at your remarks.

Can we call get_point_coordinates also just get_coordinates? That would be neatly symmetric:

• get_coordinates/get_vector have the same signature (M, p, X, B)/(M,p,a,B) when acting on TpM
• get_coordinates/get_point have the same signature (M, A, i, p)/(M,A,i,a)

So similar to project I think get_coordinates should be clear from the signature, which coordinates one wants to obtain (first case: M,p and a basis so its a tangent vector, second case M,A,i indicates the chart, so its a point).

I'm afraid of method ambiguities this may introduce so I'd prefer to keep get_point_coordinates and get_coordinates separate. project is more simple because methods for points and vectors differ by the number of arguments.

[kellertuer]

Oh I think I broke something else.

I think concerning test coverage just the power/product issues are left, I hope I got the rest.

Concerning the function names – uff – that‘s a difficult decision, because currently it is really asymmetric with get_coordinates and get_point_coordinates, I really dislike that, but I see your point, too, for sure.

[mateuszbaran]

We could also rename get_coordinates to get_vector_coordinates to be symmetric 😄 .

Which would affect base and also is not a nice name, hm? I am really unsure what to do there.

[kellertuer]

The InducedBasis is also currently not exported.

Well now, since it has a test, it is.

[mateuszbaran]

The InducedBasis is also currently not exported.

Well now, since it has a test, it is.

Ah, that's OK 🙂 .

[mateuszbaran]

Oh no, it looks like metric functions have wrong propagation:

julia> local_metric(M1, [1.0, 0.0, 0.0], B)
ERROR: MethodError: no method matching local_metric(::Euclidean{Tuple{3}, ℝ}, ::Vector{Float64}, ::DefaultOrthonormalBasis{ℝ, ManifoldsBase.TangentSpaceType})
Closest candidates are:
  local_metric(::Euclidean, ::Any, ::InducedBasis{𝔽, ManifoldsBase.TangentSpaceType, var"#s116", TI} where {var"#s116"<:RetractionAtlas, TI}) where 𝔽 at /home/mateusz/.julia/dev/Manifolds/src/manifolds/Euclidean.jl:298
  local_metric(::AbstractDecoratorManifold, ::Any, ::AbstractBasis; kwargs...) at /home/mateusz/.julia/dev/ManifoldsBase/src/DecoratorManifold.jl:358
  local_metric(::MetricManifold{𝔽, var"#s116", EuclideanMetric} where var"#s116"<:AbstractManifold, ::Any, ::InducedBasis{𝔽, ManifoldsBase.TangentSpaceType, var"#s115", TI} where {var"#s115"<:RetractionAtlas, TI}) where 𝔽 at /home/mateusz/.julia/dev/Manifolds/src/manifolds/Euclidean.jl:291
  ...
Stacktrace:
 [1] local_metric__transparent(M::Sphere{2, ℝ}, p::Vector{Float64}, B::DefaultOrthonormalBasis{ℝ, ManifoldsBase.TangentSpaceType}; kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ Manifolds ~/.julia/dev/ManifoldsBase/src/DecoratorManifold.jl:377
 [2] local_metric__transparent(M::Sphere{2, ℝ}, p::Vector{Float64}, B::DefaultOrthonormalBasis{ℝ, ManifoldsBase.TangentSpaceType})
   @ Manifolds ~/.julia/dev/ManifoldsBase/src/DecoratorManifold.jl:377
 [3] local_metric(M::Sphere{2, ℝ}, p::Vector{Float64}, B::DefaultOrthonormalBasis{ℝ, ManifoldsBase.TangentSpaceType}; kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ Manifolds ~/.julia/dev/ManifoldsBase/src/DecoratorManifold.jl:363
 [4] local_metric(M::Sphere{2, ℝ}, p::Vector{Float64}, B::DefaultOrthonormalBasis{ℝ, ManifoldsBase.TangentSpaceType})
   @ Manifolds ~/.julia/dev/ManifoldsBase/src/DecoratorManifold.jl:359
 [5] top-level scope
   @ REPL[33]:1

[mateuszbaran]

Anyway, maybe we should just return 1 as det_local_metric for AbstractOrthonormalBasis (see https://mathoverflow.net/questions/353860/taylor-expansion-of-determinant-of-riemannian-metric-in-normal-coordinates-up-to )

[mateuszbaran]

I've removed these two methods for now, they are not that easy to fix and we can always add them back later.

[kellertuer]

Ok, that‘s also fine with me.

[kellertuer]

Ok, fine. Since I am still unsure about the coordinate naming, let‘s for now just keep it as it is.

So then, as far as I see we could merge this?

[mateuszbaran]

Yes, I think we can merge this 🎉 .