More random randomness on random (no actually all) Lie groups

[kellertuer]

This small fix could even reduce other Lie groups random implementations

• [x] adds rand to UnitaryMatrices
• [x] adds. a “passthrough” for points on Lie groups (to ask the manifold) and tangent vectors to generate tangent vectors at the identity
• [x] text coverage
• [x] check whether other Lie algebras implicitly do that already and now explicitly could use this generic default instead.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (e46a864) 99.57% compared to head (bb03bb0) 99.57%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #706 +/- ## ======================================= Coverage 99.57% 99.57% ======================================= Files 113 113 Lines 10961 10986 +25 ======================================= + Hits 10914 10939 +25 Misses 47 47 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[kellertuer]

Hmm, this turns out to be much more involved than I thought.

a) Interaction with power groups is still quite strange (I would have hoped them passing on to PowerManifold would just be fine b) Introducing a generic fallback has broken a few existing implementations (or fallbacks) as well? This is also very stange. Probably means I have some misunderstanding (again?) on how the group decorator is supposed to work. My hope was that basically no group needs a rand function after this (unless a much more efficient variant is known on said group compared to the manifold one).

Currently I am not able to resolve these two. So I will try again the next few days somewhere. Sorry, this was meant to be a “Saturday-morning-30-minute-PR” but might be a “maybe I understand this in a few weeks” PR then.

Previously, somehow usually on power groups with element manifold M rand(M) was used while now its rand!(rng, M, pX) which (and I have no clue why) does not work for special orthogonal (though even that should pass on by the new definition to orthogonal matrices, but it never evaluates its traits but directly says they are all empty).

Our traits are really cool but in situations like these horrible to debug.

With

Mr = SpecialOrthogonal(3)
rand(Mr) # works
p = zeros(3,3)
rand!(Mr, p)# Does work as well
rand!(Random.default_rng(), Mr, p) # never gets its traits only the empty one and hence errors,
active_traits(rand!,Mr) # but the traits are correctly defined, just never asked for.

I fear this would need a traits macro where one can specify which argument the manifold is at (that defaults to 1)

Now I am even more confused, since that does exist

https://github.com/JuliaManifolds/ManifoldsBase.jl/blob/b7665ae67e1b0f7d3a9c93bed17907f93ddb19a5/src/decorator_trait.jl#L622

but then I have no clue left who the rand! call above gets no traits (and hence does not work...

edit 7 (or so): It seems this line

https://github.com/JuliaManifolds/ManifoldsBase.jl/blob/b7665ae67e1b0f7d3a9c93bed17907f93ddb19a5/src/nested_trait.jl#L306

becomes a call like ManifoldsBase.trait(rng, rand!, Mr, pX) which does now work since the traits are only correctly returned for ManifoldsBase.trait(rand!, Mr, pX) (but this also only happens if the manifold is not first). @mateuszbaran can we fix this somehow? I mean sure one could omit all before manifold or put them at the end – does either of that make sense and is doable?

Since from the general idea of the order of arguments, the ones before the manifold should either be types or a function call or the rng – I think these can be omitted in the trait call.

[mateuszbaran]

Our traits are really cool but in situations like these horrible to debug.

I usually just don't use traits for things where don't work well. Like here. Playing with trait dispatch is rarely a 30-minute thing. If you delete the trait dispatches, everything else should work fine.

[kellertuer]

The hint you propose, I do not understand. I can not define rand to work on the identity for any decorated manifold (for tangents) – and also not just for GroupManifolds since the whole idea is to also provide it for the general unitary group definition.

[mateuszbaran]

The only way to make rand work for identity is checking for it in the implementation of rand for group (using if vector_at isa Identity). You can't do that through dispatch.

[kellertuer]

Oh that part is not my problem ;)

I want to map random tangents to those at the identity = Lie algebra (instead of in TpM what the manifold case would be)

[kellertuer]

I think my best solution (but I struggle with your macro) is if line 306 above would omit all parameters before the manifold (so none if manifold is first but for example rng or a type) when calling trait. That would resolve all my problems basically.

Without that – we can not do this PR, and I could only try to implement more single Lie group rand!s

[kellertuer]

A (boooooring) alternative is of course to define active_traits(rng, M, rand!, ...) to map to activate_traits(M, rand!,...);)

edit: hm, though boring it might be easier than the other ideas above

[mateuszbaran]

I'm not sure why can't you just dispatch on GroupManifold and forget about traits? It's boring but works fine.

[mateuszbaran]

It wouldn't be as general as traits but much easier to do.

[mateuszbaran]

I'm simply just tired of fighting with dispatching on traits and don't want to debug any new code that relies on it.

[kellertuer]

I'm not sure why can't you just dispatch on GroupManifold and forget about traits? It's boring but works fine

because some are (just) implicit group manifold by having the trait

I'm simply just tired of fighting with dispatching on traits and don't want to debug any new code that relies on it.

Yes, me as well, as I said before, when it works it‘s great to reuse code, but a bit hard to debug. Maybe the boring variant should work fine enough, since that can remove the rng even for any manifold in the active traits arguments. Will check that tomorrow.

[kellertuer]

Hm the “boring” approach really seems to be the better one, it also restricts this “removing a parameter” to just RNGs – and was a 3 line fix – and me being stupid for an hour. So let's see how much this now breaks. I would still expect this to not work, though the new approach limits the effect to rand – it probably has side effects somewhere.

[mateuszbaran]

I think the boring solution shouldn't cause any issues.

[kellertuer]

I think by now the boring one is even the better one, since it does not (further) complicate the macro and localises the “ignore something before the manifold” to really just the one case where we have that (the seed), so we do not do that “by accident” when we should not.

[kellertuer]

I noticed that for https://github.com/JuliaManifolds/Manopt.jl/discussions/349 having rand and project would be nice for unitary so I adapted those from Stiefel (which only differs in tangent vector representation).

[mateuszbaran]

It seems like there was an old projection that was working, and your new one was incorrect?

[kellertuer]

That is strange, because yesterday locally that worked fine for me? But maybe I confused pX and X as representers somewhere between UnitaryMatrices and Unitary?

[kellertuer]

At least I added concrete tests for Unitary random by now.

That is the project I also will return to after this – better testing in Manifolds.jl – that is my next thing to continue here in this package.

[mateuszbaran]

Tests on CI stopped failing after I removed that project! so I guess it wasn't correct.

That is the project I also will return to after this – better testing in Manifolds.jl – that is my next thing to continue here in this package.

That's a hard problem but maybe you will figure something out.

[kellertuer]

Well I started that in the other pr and have an idea, though I am not yet sure how much the complete rework will be.

But I noticed that we have single groups with currently 10k tests, so reducing that a bit might also be nice.

[kellertuer]

Tests seem a bit strange here, as if codecov had a hiccup – bit besides that I would consider this finished ;)