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

Unscrambled Quaternion Averaging Function Results #1430

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dimvoly
Copy link

@dimvoly dimvoly commented Jul 30, 2024

Issue

The ordering of the final output in the geometry::quaternion_construction::UnitQuaternion<T>::mean_of appears to be incorrect. Presumably this was due to the w, i, j, k ordering changing at some point from i, j, k, w + the original doctest checking against an expected zero value. Testing against an expected zero value doesn't check if the zero came from what should've been a zero, or some other zero elsewhere, it's better to use a non-zero value.

Fix

The fix is swapping the output values around to follow the w, i, j, k ordering. Additionally, to avoid similar problems in the future, have added 3 tests which have values calculated using a python implementation of what appears to be the same algorithm.

Current Workaround

What I have to do currently is to unscramble the outputs from the mean_of function (without this PR):

let q1 = UnitQuaternion::from_euler_angles(0.0_f64, 0.0, 0.4);
let q2 = UnitQuaternion::from_euler_angles(0.0, 0.0, 0.2);
let q3 = UnitQuaternion::from_euler_angles(0.0, 0.0, 0.3);

let quat_vec = vec![q1, q2, q3];
let q_mean_scrambled = UnitQuaternion::mean_of(quat_vec);
let q_mean_corrected =         
        UnitQuaternion::from_quaternion(Quaternion::from_vector(Vector4::new(
                q_mean_scrambled[3],
                q_mean_scrambled[0],
                q_mean_scrambled[1],
                q_mean_scrambled[2],
        )));

Caveats

As a side note, for some reason the doctest on my machine doesn't want to run automatically when I click the "Run Doctest" code lens button. I have to invoke it manually by its name. It also doesn't seem to run under cargo test.

I'm not a quaternion expert, just a user. I don't have detailed knowledge about the actual averaging algorithm, I relied on the python implementation to provide me with the test values I was after. There's nothing inherently special about the values used in the tests.

Referencing Potentially Wrong

The algorithm quoted in the original PR is Markley et al. 2007. However the actual code appears to reference a paper from 2006. The 2006 paper is behind a paywall and from looking at its abstract does not appear to contain an averaging algorithm.

The 2007 paper is widely available and is widely used per the stackoverflow discussion. The python code used to generate the test values in this PR is also derived ultimately from Markley et al. 2007.

We'll need to ping the original author (@thibaultbarbie) for comment. If no response, then in the absence of better information, it may have to be assumed that Markley et al. 2007 was the intended reference all along and the doc string will have to be updated.

dimvoly added 3 commits July 29, 2024 22:46
With values generated from python implementation of algorithm.

mean_of not fixed yet.
Tests now working.
@tpdickso
Copy link
Collaborator

tpdickso commented Dec 5, 2024

Thank you for the contribution! I'll mark this as a breaking change, since downstream users who are currently unscrambling the results will now be re-scrambling them.

@tpdickso tpdickso added the breaking change Fixing this issue, or merging this pull-request is likely to require breaking changes label Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Fixing this issue, or merging this pull-request is likely to require breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants