Introduce rand(M) and rand!(M, p) to the API

[kellertuer]

This PR adapts the existing methods from Manifolds.jl to ManifoldsBase.jl.

Things to check / discuss

• [x] should we make both functions aware of decorators? Especially for the rand(rng, M) variants this might be challenging, but I thing that might be cool to have.
• [x] Should we also provide a default implementation for the Power manifold?
• [x] ~Should we also provide an implementation in the embedding (rand in embedding & project)?~

[codecov[bot]]

Codecov Report

Merging #141 (946df35) into master (1bca296) will increase coverage by 0.08%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #141      +/-   ##
==========================================
+ Coverage   99.70%   99.79%   +0.08%     
==========================================
  Files          18       18              
  Lines        2397     2451      +54     
==========================================
+ Hits         2390     2446      +56     
+ Misses          7        5       -2     

Impacted Files	Coverage Δ
src/decorator_trait.jl	100.00% <ø> (ø)
src/DefaultManifold.jl	100.00% <100.00%> (ø)
src/ManifoldsBase.jl	99.55% <100.00%> (+0.02%)	:arrow_up:
src/PowerManifold.jl	99.62% <100.00%> (+0.42%)	:arrow_up:
src/nested_trait.jl	100.00% <100.00%> (ø)

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

[mateuszbaran]

Nice, I think this is a good idea.

• should we make both functions aware of decorators? Especially for the rand(rng, M) variants this might be challenging, but I thing that might be cool to have.

I think this can wait, especially for the variant with rng. That would be quite complicated to support.

• Should we also provide a default implementation for the Power manifold?

Sure, you can copy it from Manifolds.jl.

• Should we also provide an implementation in the embedding (rand in embedding & project)?

That would require making rand aware of decorators, which would be quite a lot of work.

[kellertuer]

Ok, we can add the power part here as well;

For the decorator-aware part – for the one starting with M that should be simple, for the ones starting with the rin, that is more work, I think, but also worth it if you think of the SPDs or something similar with a lot of metrics? Randomness – at least in my mind – is independent of the metric.

[kellertuer]

Uff, I took. a short look at the Power rands – that is a hundred lines of code! Some inheriting from Random (which we have here already) some from Distributions (which we don't) and probably also for several different power styles. I will have to read this hundred lines very very very very carefully to check which ones we can copy here and which we can't.

[mateuszbaran]

For the decorator-aware part – for the one starting with M that should be simple, for the ones starting with the rin, that is more work, I think, but also worth it if you think of the SPDs or something similar with a lot of metrics? Randomness – at least in my mind – is independent of the metric.

Randomness isn't entirely independent of the metric (if you care about probability density) but I get your point. My only concern is that it may be complicated.

Uff, I took. a short look at the Power rands – that is a hundred lines of code! Some inheriting from Random (which we have here already) some from Distributions (which we don't) and probably also for several different power styles. I will have to read this hundred lines very very very very carefully to check which ones we can copy here and which we can't.

I am somewhat familiar with that code so I can handle that.

[kellertuer]

For the M-upfront variant it should be our normal decorator macro, but you are right, for the other it might be more complicated.

[kellertuer]

Would it be a solution to (by default) pass rand(ring, M, ...) to a rand_rngdeco(M, rng...) that is decorated with the nested trait and its fall back is to call rand(rng, M_undeco) on the undecorated manifold again? That would be a little trick, but maybe the easiest to do? That way one can still implement metric-specific variants for example.

[mateuszbaran]

Hm, I've taken a look and it seems that the new trait system shouldn't care that manifold is not the first argument so I'd try decorating without permuting arguments.

[kellertuer]

Oh, are you sure? That would be neat, if that just worked. Can you try? You know that system better than me.

[kellertuer]

It does not seem to be so easy, just adding

@trait_function rand(M::AbstractDecoratorManifold; kwargs...)
@trait_function rand!(M::AbstractDecoratorManifold, p; kwargs...)
@trait_function rand(rng::AbstractRNG, M::AbstractDecoratorManifold; kwargs...)
@trait_function rand!(rng::AbstractRNG, M::AbstractDecoratorManifold, p; kwargs...)

Even brass the DefaultManifold, and I am not sure why, since it does report

MethodError: no method matching rand!(::DefaultManifold{Tuple{3}, ℝ}, ::Vector{Float64})

but actually that is implemented in line 135 in DefaultManifold (besides the kwargs not seem to be carried on though I used them above)?

So – probably I am again missing something in our magic decorator.

[kellertuer]

Ha! I forgot the prefixes and to import rand! – now the tests are running but I am not sure we have coverage yet nor am I sure how to test the decorated ones.

[mateuszbaran]

Ah, nice, you've added the dispatch. I will try adding a test using the Pass-through trait.

[mateuszbaran]

I had to fix our decorator system a bit but now rand! and rand should work properly.

[kellertuer]

Nice!

[mateuszbaran]

Are you working on adapting Manifolds.jl to this small change (primarily removal of the rand methods for PowerManifold)? If not I can do that.

[kellertuer]

That would be great, I‘ll be a little more busy with semester preparations starting tomorrow.