Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix apply_diff_group for some actions #673

Merged
merged 29 commits into from
Nov 6, 2023
Merged

Conversation

mateuszbaran
Copy link
Member

@mateuszbaran mateuszbaran commented Oct 27, 2023

I've also added differential of matrix inversion.

TODO:

  • Check if this also affects translate_diff, inverse_translate_diff and adjoint variants.
  • Add tests & check coverage.

@mateuszbaran mateuszbaran added WIP Work in Progress (for a pull request) preview docs Add this label if you want to see a PR-preview of the documentation labels Oct 27, 2023
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

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

❗ 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%) ⬆️
ext/ManifoldsTestExt/tests_group.jl 100.00% <100.00%> (+0.26%) ⬆️
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

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

@mateuszbaran mateuszbaran linked an issue Nov 3, 2023 that may be closed by this pull request
@mateuszbaran
Copy link
Member Author

mateuszbaran commented Nov 3, 2023

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

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 mateuszbaran added Ready-for-Review A label for pull requests that are feature-ready and removed WIP Work in Progress (for a pull request) labels Nov 5, 2023
@mateuszbaran
Copy link
Member Author

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

@mateuszbaran mateuszbaran merged commit 0a71858 into master Nov 6, 2023
@kellertuer kellertuer deleted the mbaran/fix-apply-diff-goa branch May 4, 2024 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview docs Add this label if you want to see a PR-preview of the documentation Ready-for-Review A label for pull requests that are feature-ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

apply_diff_group gives wrong sign?
3 participants