fix tangent vector allocations (nearly)

[kellertuer]

We still have one problem:

I tried to define allocate_result(M::AbstractManifold, ::typeof(rand), p) (commented out for now) but that is ambigous to the decorator path of allocations. The decorator path however would always pass for FixedRank to the Embedding. So we would need the specific allocation anyways?

[codecov[bot]]

Codecov Report

Merging #168 (ba52e7a) into master (b3d14c5) will decrease coverage by 0.12%. The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
- Coverage   99.96%   99.84%   -0.12%     
==========================================
  Files          20       20              
  Lines        2558     2562       +4     
==========================================
+ Hits         2557     2558       +1     
- Misses          1        4       +3     

Files Changed	Coverage Δ
src/ManifoldsBase.jl	98.43% <100.00%> (-1.18%)	:arrow_down:

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

[kellertuer]

Ah I think I found a way, it was mainly TangentSpaceAtPoint where I will define in a next PR on Maniflds.jl

allocate_result(M::TangentSpaceAtPoint, ::typeof(rand)) = zero_vector(M.fiber.manifold, M.point)

and then we have to check whether the allocation that include p would be nice to have

allocate_result(M::AbstracManifold, ::typeof(rand), p) = zero_vector(M, p)

but that one (though nice) includes a fight of ambiguities that at least I am not able to resolve (with line 91 in decorator_trait.jl)

[mateuszbaran]

Yes, allocate_result(M::AbstracManifold, ::typeof(rand), p) = zero_vector(M, p) would lead to a lot of ambiguities. The only sane solution I can think of is adding an if f === rand to allocate_result(M::AbstractManifold, f, x...), so that there is no dispatch. But it's ugly so not great either.

[kellertuer]

Most will not notice, because currently that is dispatched to the embedding and a default p of representation size allocated there. But this is the one thing failing here; for now defining that in FixedRank (and TangentSpaceAtPoint) does resolve this as well.

[kellertuer]

The three lines added here are the same as on the other PR, where it seems a bug on 1.1 was fixed so our new method for mid point does no longer seem necessary.

[mateuszbaran]

I would worry about these lines, soon we will just drop Julia 1.0 and those lines anyway.

[kellertuer]

yes sure it will be fixed with the other still open PR.

[kellertuer]

Just ran the Manifold tests completely and what does fail are PositiveNumbers, some interplay between rand! and the rug it seems? Not sure. I fear I again do not fully understand why that fails over on Manifolds.

The question is then, do we check and fix that here or do we adopt PositiveNumbers?

edit: besides that other types cases fail (SPDPoint), but that is expected since their tangent vectors before did not work at all and now those have to be fixed as well. Just for positive numbers I am not sure, why they fail.

edit 2: Since it is really just positive numbers but for example not the circle, I would assume that is a bug in Positive Numbers and we could merge / register this PR.