search

JuliaManifolds / Manifolds.jl

Manifolds.jl provides a library of manifolds aiming for an easy-to-use and fast implementation.
https://juliamanifolds.github.io/Manifolds.jl
MIT License
368 stars 53 forks source link

Fix `apply_diff_group` for some actions #673

Closed mateuszbaran closed 10 months ago

mateuszbaran commented 10 months ago

I've also added differential of matrix inversion.

TODO:

codecov[bot] commented 10 months ago

Codecov Report

Merging #673 (0ad3fad) into master (f9a6b19) will increase coverage by 0.16%. The diff coverage is 100.00%.

:exclamation: Current head 0ad3fad differs from pull request most recent head 6714cff. Consider uploading reports for the commit 6714cff to get more accurate results

@@            Coverage Diff             @@
##           master     #673      +/-   ##
==========================================
+ Coverage   99.41%   99.57%   +0.16%     
==========================================
  Files         108      108              
  Lines       10600    10675      +75     
==========================================
+ Hits        10538    10630      +92     
+ Misses         62       45      -17
Files Coverage Δ
ext/ManifoldsTestExt/tests_general.jl 99.15% <ø> (+3.23%) :arrow_up:
ext/ManifoldsTestExt/tests_group.jl 100.00% <100.00%> (+0.26%) :arrow_up:
src/Manifolds.jl 86.66% <ø> (ø)
src/groups/addition_operation.jl 100.00% <100.00%> (ø)
src/groups/general_unitary_groups.jl 100.00% <100.00%> (ø)
src/groups/group.jl 100.00% <100.00%> (ø)
src/groups/group_operation_action.jl 100.00% <100.00%> (ø)
src/groups/multiplication_operation.jl 100.00% <100.00%> (ø)
src/groups/power_group.jl 100.00% <100.00%> (ø)
src/groups/product_group.jl 100.00% <100.00%> (ø)
... and 1 more

:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

mateuszbaran commented 10 months ago

Sorry that it takes a while but it's fairly convoluted. @olivierverdier if I'm on the right track, it looks like our differentials are incorrect exactly up to sign when differentiating wrt. something taken with an inverse.

olivierverdier commented 10 months ago

Sorry that it takes a while but it's fairly convoluted. @olivierverdier if I'm on the right track, it looks like our differentials are incorrect exactly up to sign when differentiating wrt. something taken with an inverse.

Do you think it is a math mistake or an implementation mistake? 🤔

mateuszbaran commented 10 months ago

There isn't enough math comments left from the time I've added left-backward and right-forward actions to be sure. I'm just re-doing the math and checking it against implementation.

mateuszbaran commented 10 months ago

Could you check if apply_diff_group gives the correct result in this PR for group operation actions on SO(3) when a and p are not identity? I've made some mistakes in my math. The current state of implementation was derived from the Giles' paper I've cited but all these inverses and representation adjustments are easy to get wrong.

mateuszbaran commented 10 months ago

This fix was a little harder then I expected but I think this is finished now.