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 3x3 covariance matrix rotation/transformation #1998

Merged
merged 2 commits into from
Oct 10, 2024
Merged

Fix 3x3 covariance matrix rotation/transformation #1998

merged 2 commits into from
Oct 10, 2024

Conversation

leocencetti
Copy link
Contributor

@leocencetti leocencetti commented Oct 8, 2024

The template specializations of transform_static_frame() and transform_frame() for the type Covariance3d improperly rotate the covariance matrix.

Implementation details

Old (broken) implementation

The old implementation is wrong and returns an ill-conditioned covariance matrix, breaking symmetry and positive-semi-definitiveness.

Taking for example the following covariance matrix P and quaternion q:

    | 0.1 0.2 0.3 |
P = | 0.2 0.4 0.5 |
    | 0.3 0.5 0.6 |

q = 1i + 0j + 0k + 0  # 180deg around X

The old implementation would return the matrix P':

                        | 0.1 -0.2 -0.3 |
P' = P * q = P * R(q) = | 0.2 -0.4 -0.5 |
                        | 0.3 -0.5 -0.6 |

which is clearly invalid (asymmetric and negative).

New (proper) implementation

The new implementation correctly rotates the 3x3 covariance matrix by left- and right-multiplying by the rotation quaternion (as done for the 6x6 and 9x9 cases):

                                                 |  0.1 -0.2 -0.3 |
P' = q * P * q.conjugate() = R(q) * P * R(q).T = | -0.2  0.4  0.5 |
                                                 | -0.3  0.5  0.6 |

which is a valid covariance matrix.

Useful resources

GodBolt example: link

Covariance matrix rotation derivation: https://robotics.stackexchange.com/questions/2556/how-to-rotate-covariance

Edit: Updated GodBolt link to use un-shortened version for posterity

@leocencetti
Copy link
Contributor Author

This PR is currently targeting the ros2 branch, but the issue is present everywhere else. Might be a good idea to back-port these fixes to master (ROS1).

Copy link
Member

@vooon vooon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks!

@TSC21

@vooon vooon merged commit f5e4f1f into mavlink:ros2 Oct 10, 2024
0 of 4 checks passed
@leocencetti
Copy link
Contributor Author

@vooon FYI there were 4 errors in the test code (reason for the failing CI). I hadn't built the tests locally so they slipped through...

Since you merged already, I'll open a second PR to patch them.

@vooon
Copy link
Member

vooon commented Oct 10, 2024

I'm on release process, though to fix myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: TODO
Development

Successfully merging this pull request may close these issues.

2 participants