Bases of vector spaces

[mateuszbaran]

Some work on adding bases of cotangent spaces. This is supposed to solve problems with flat! and sharp! from this PR: JuliaManifolds/Manifolds.jl#325 .

[codecov]

Codecov Report

Merging #59 (03094a5) into master (e1babe4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #59   +/-   ##
=======================================
  Coverage   99.69%   99.70%           
=======================================
  Files          10       11    +1     
  Lines        1320     1342   +22     
=======================================
+ Hits         1316     1338   +22     
  Misses          4        4           

Impacted Files	Coverage Δ
src/DecoratorManifold.jl	100.00% <100.00%> (ø)
src/DefaultManifold.jl	100.00% <100.00%> (ø)
src/EmbeddedManifold.jl	99.30% <100.00%> (-0.02%)	⬇️
src/ManifoldsBase.jl	99.43% <100.00%> (+0.03%)	⬆️
src/PowerManifold.jl	99.14% <100.00%> (-0.02%)	⬇️
src/ValidationManifold.jl	100.00% <100.00%> (ø)
src/bases.jl	100.00% <100.00%> (ø)
src/numbers.jl	100.00% <100.00%> (ø)
src/vector_spaces.jl	100.00% <100.00%> (ø)
src/vector_transport.jl	100.00% <100.00%> (ø)
... and 1 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 e1babe4...03094a5. Read the comment docs.

[kellertuer]

So this basically introduces a second parameter to the basis indicating what the basis is for? Looks ok to me, I just noticed a minor rephrasing of an error message we could do.

[kellertuer]

“this function for a default” – you are referring to a default type? Otherwise one could also write „a default for this function“.

[mateuszbaran]

This text was there before. Now I'm not quite sure what was it supposed to say.

[kellertuer]

Ah, I did not notice that; we could still rephrase it :)

[kellertuer]

Sorry I did not want to approve this, since tests are still missing,

[mateuszbaran]

So this basically introduces a second parameter to the basis indicating what the basis is for? Looks ok to me, I just noticed a minor rephrasing of an error message we could do.

Yes, it's essentialy this, as well as moving FVector to ManifoldsBase.jl and adding a field in it for the basis. The new parameter for basis is not entirely necessary but it looked weird to have a basis and not knowing what kind of space it is a basis of.

[kellertuer]

Since it is backwards compatible, I think it's fine to add the parameter.

[mateuszbaran]

I just noticed that the vector space is indeed needed in the basis, to be able to correctly interpret coordinates in get_vector.

[kellertuer]

That's why I carefully asked – we can surely keep in that order and yes, ambiguity fighting is not nice, for sure. Thanks for the explanations.

Yes, that's equivalent, I can use this instead of <:Any.

Great. I think that looks a little nicer

[mateuszbaran]

Non-implemented error messages are quite unhelpful when debugging wrong invokes. I've removed one, maybe we should remove all of them?

[kellertuer]

We discussed that anyways following Lyndons notes, I would say it's reasonable to remove them, yes.

[mateuszbaran]

I think this is ready now. The one new coverage failure looks like a false positive.

[mateuszbaran]

While improving documentation I noticed that vector_space_dimension is referenced in ManifoldsBase.jl but defined in Manifolds.jl. I think we could change its signature to make it take separately manifold and vector space type and move it to ManifoldsBase.jl as well.

[mateuszbaran]

OK. I think we don't need both is_manifold_point and is_point so I think we should pick one of them. is_vector is needed anyway and one remaining issue here is whether is_vector should call is_tangent_vector or the other way round. I think it will be extremely rare to overload is_vector (or zero_vector) for manifolds, for vector spaces other than the tangent space.

[kellertuer]

OK. I think we don't need both is_manifold_point and is_point so I think we should pick one of them. is_vector is needed anyway and one remaining issue here is whether is_vector should call is_tangent_vector or the other way round. I think it will be extremely rare to overload is_vector (or zero_vector) for manifolds, for vector spaces other than the tangent space.

We overload/define zero(_tangent)_vector! quite often, I think; will that get obsolete, so will there be a good default?

So, I think if we drop backwards compatibility, then we should get rid of is_tangent_vector and is_manifold_point and just use is_point and is_vector, though both names are not completely self-descriptive (you have to have in mind point->manifold, vector->tangent space or any other fibre).

[mateuszbaran]

We overload/define zero(_tangent)_vector! quite often, I think; will that get obsolete, so will there be a good default?

Yes, we have some overloads for tangent vectors. What I mean is that for any other fibre there would usually be decent defaults. For tangent vectors it won't be obsolete.

So, I think if we drop backwards compatibility, then we should get rid of is_tangent_vector and is_manifold_point and just use is_point and is_vector, though both names are not completely self-descriptive (you have to have in mind point->manifold, vector->tangent space or any other fibre).

This patch already breaks quite a bit so I wouldn't worry about backwards compatibility. I'll get rid of is_tangent_vector and is_manifold_point then.

[kellertuer]

Ok, let's just make sure, that @sethaxen at some point agrees, too; but I imagine he is also quite busy currently.

[sethaxen]

Sorry, there's a lot of discussion here. Can someone summarize the current proposal/disagreement?

[kellertuer]

I try to do summarise in order of renaming, since that was a trail of thoughts all this followed. The first is function names

1. zero_tangent_vector is renamed to zero_vector for consistency with Julia
2. is_tangent_vector is renamed to is_vector for consistency with 1.

Here my criticism is, that now the function name bears a little less information about what the function is doing, but if you have the full signature is_vector(M, p, X) it's fine. Even more these functions can now be used for any fibres – for the first maybe with an optional parameter, for the second depending on the type of X. So especially for co tangent vectors, these also should work.

3. is_manifold_point is renamed to is_point since that is then consistent with 2. – for both one now needs the implicit knowledge either that points are on manifolds and vectors are in tangent spaces or the full signature is_point(M,p). For this I am ok, since I think I Walt to move to using p ∈ M anyways

Then we came across types

4. MPoint is renamed to AbstractManifoldPoint, since that is more informative and the Abstract is more consistent with the rest of our naming scheme
5. TVector and CoTVector get an abstract super type AbstractFibreVector, and while I writing this summary, we should actually call them AbstractTangentVector and AbstractCoTangentVector, respectively.

Finally since it is breaking everything anyways already here

6. Manifold is renamed to AbstractManifold to be more consistent.

4.-6. were actually names I introduced, I think, when I started Manopt.jl and when I was not completely aware of the Julia naming scheme, so I think, since this is breaking anyways and renames a lot, this is a good time to introduce this consistency.

That‘s the proposal and I think currently there is no disagreement really – I think there is a small tradeoff, that is_vector bears a little too less information (without the rest of the signature), but it is good that it then can be used on arbitrary fibers.

But for sure, we would like to know your opinion, too.

[mateuszbaran]

That's a good summary 🙂 . I'd just add that I want to do some generalization here for arbitrary fibers because PR JuliaManifolds/Manifolds.jl#325 finally makes it possible to handle them well.

[kellertuer]

I just noticed, since we are renaming everything to shorten names, what about changing check_base_point to check_point (and document that a little better in check_tangent_vector. What do you think?

[mateuszbaran]

Sure, that would be consistent with the other changes.

[mateuszbaran]

I've formatted code locally using JuliaFormatter 0.13.10. Here JuliaFormatter 0.14 was used and it crashed on parsing our files 😕 .

[kellertuer]

don‘t we have check_base mentioned here somewhere? I think at least is_vector (or check vector) should mention it in its doctoring. I will investigate the Julia<formatter issue tomorrow (can reproduce it even on master locally here).

[mateuszbaran]

I've submitted an issue: domluna/JuliaFormatter.jl#410 .

[mateuszbaran]

What do you mean by check_base?

[kellertuer]

Cool, thanks for submitting that.

Ah yes, I mean check_base_pointwhich we might want to call check_point? That should be mentioned maybe in both doc strings even.

[mateuszbaran]

OK, I see, that check_base_point. I think check_base_point is still fine? A little verbosity there doesn't look bad and the name is still correct.

[kellertuer]

Ok, we can also leave it, it would still be good to mention it in the docs I think.

[mateuszbaran]

I'm also thinking that we probably should just implement something like _check_vector for each manifold, and do something like

function check_vector(M::Manifold, p, X; check_base_point=true, kwargs...)
    if check_base_point
        mpe = check_point(M, p; kwargs...)
        mpe === nothing || return mpe
    end
    return _check_vector(M, p, X; kwargs...)
end

so that check_vector wouldn't have to duplicate the logic for checking the base point for each manifold.

[kellertuer]

Maybe the easier solution is to only have check_base(_point) for is_vecor (i.e. the high level interface) and not for check_*(i.e. the lower one)?

[mateuszbaran]

Yes, that's a good idea. I'll do that.

[kellertuer]

Cool, one can see on Manifolds.jl how much redundancy this removes; great!