Adapt Manifolds to new Decorator type

[kellertuer]

...that was introduced in ManifoldsBase 0.12.0

This PR also

• Reduces code lines in MetricManifold
• Reduces code lines in Group Manifold
• checks/updates docs

[kellertuer]

The main rework is done, however there is more tests failing than I thought, so I might have missed something, hopefully just for the group manifold, since there we do not have explicit checks for transparency.

[kellertuer]

The group manifold has some little strange transparency tests that I do not yet understand, maybe @sethaxen you wrote them?
Why are there tests checking for a function with parameters (

Manifolds.jl/test/groups/groups_general.jl

Lines 33 to 41 in 8a5d654

	@test Manifolds.decorator_transparent_dispatch(compose, G, x, x, x) ===
	Val{:intransparent}()
	@test Manifolds.decorator_transparent_dispatch(compose!, G, x, x, x) ===
	Val{:intransparent}()
	@test Manifolds.decorator_transparent_dispatch(group_exp, G, x, x) ===
	Val{:intransparent}()
	@test Manifolds.decorator_transparent_dispatch(group_log, G, x, x) ===
	Val{:intransparent}()
	@test Manifolds.decorator_transparent_dispatch(

) that I understand and that are currently covered (or I hope they are) and some that are without parameters (

Manifolds.jl/test/groups/groups_general.jl

Lines 141 to 156 in 8a5d654

	for f in [compose, compose!, translate_diff!, translate_diff]
	@test Manifolds.decorator_transparent_dispatch(f, G) === Val{:transparent}()
	end
	for f in [translate, translate!]
	@test Manifolds.decorator_transparent_dispatch(f, G) === Val{:intransparent}()
	end
	for f in [inverse_translate_diff!, inverse_translate_diff]
	@test Manifolds.decorator_transparent_dispatch(f, G) === Val{:transparent}()
	end
	for f in [group_exp!, group_exp, group_log, group_log!]
	@test Manifolds.decorator_transparent_dispatch(f, G, x, x) ===
	Val{:intransparent}()
	end
	for f in [get_vector, get_coordinates]
	@test Manifolds.decorator_transparent_dispatch(f, G) === Val{:parent}()
	end

) these I do not understand, especially since they are different to the first ones...

Edit: Since I seem to have last touched (git blame) the second part of lines, I might propose now to maybe delete those since I do not know what they are supposed to test.

[mateuszbaran]

Edit: Since I seem to have last touched (git blame) the second part of lines, I might propose now to maybe delete those since I do not know what they are supposed to test.

IIRC such tests are there to just notify that transparency changed. If the change is deliberate, you can adjust the test. If it's not, It'd be better to fix the code.

[kellertuer]

The main problem is, that the two contradict, so with parameters is different form without, and I think that is not only strange but also that without parameters the testing is not even well defined nor useful?

[mateuszbaran]

Ah, OK, so you can just remove these tests then I think.

[kellertuer]

Hm, seems that I have introduced some side effects that I did not intent to; I will have to check closer what is going wrong currently. Sorry that it will take a while, since it seems the CompatHelper still gives us an update every day ( I thought that was fixed?)

[mateuszbaran]

Hm, seems that I have introduced some side effects that I did not intent to; I will have to check closer what is going wrong currently.

No problem. I will continue my work on connections after this is done but I have plenty of other things to do 🙂 . Including writing of better documentation for groups (it's very basic right now).

it seems the CompatHelper still gives us an update every day ( I thought that was fixed?)

Maybe 0.12.1 release triggered it again? We could easily check 🙂

[kellertuer]

I hope I finally fixed the tests.

• There were two kinds of transparencies used. Instead of having a Hierarchy that (as currently) goes first to the decorator then the transparency type, we could maybe do it the other way around, that would reduce code by a few lines (i.e. Having a TransparentDecoratorType as a super type for TransparentEmbedding and TransparentGroup).
• one function had to be reimplemented: base_manifold.

[codecov]

Codecov Report

Merging #388 (d631822) into master (991569f) will decrease coverage by 0.19%.
The diff coverage is 60.31%.

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
- Coverage   97.92%   97.72%   -0.20%     
==========================================
  Files          76       76              
  Lines        5914     5841      -73     
==========================================
- Hits         5791     5708      -83     
- Misses        123      133      +10     

Impacted Files	Coverage Δ
src/Manifolds.jl	100.00% <ø> (ø)
src/groups/group.jl	90.76% <50.00%> (-3.52%)	⬇️
src/groups/group_action.jl	87.50% <55.55%> (-4.17%)	⬇️
src/groups/general_linear.jl	100.00% <100.00%> (ø)
src/groups/special_linear.jl	100.00% <100.00%> (ø)
src/manifolds/HyperbolicHyperboloid.jl	100.00% <100.00%> (ø)
src/manifolds/MetricManifold.jl	97.41% <100.00%> (-0.92%)	⬇️
src/manifolds/Rotations.jl	95.81% <0.00%> (-0.02%)	⬇️

---

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 991569f...d631822. Read the comment docs.

[kellertuer]

The 13 lines we miss are due to macros not being checked correctly (I shortened code there by replacing a function definition and a @signature with the direct @function. I will look at the lins closely when investigating the documentation.

[kellertuer]

The 13 lines we still miss might be due to an issue like JuliaLang/julia#36283

[mateuszbaran]

Yes, all of these failures look like false positives.

[mateuszbaran]

OK, I think this is fine now 🙂

[kellertuer]

Nice! I will wait for the tests and merge maybe in the evening or tomorrow morning.