Question about the possible ROT3 quaternion bug?

[成旭元]

## Description

There is an issue when using `Rot3().quaternion().x()`, `Rot3().quaternion().y()`, `Rot3().quaternion().z()`, or `Rot3().quaternion().w()` to access the components of a quaternion. For example, when using `Rot3().quaternion().w()` to access `qw`, it actually returns `qz`.  


## Steps to reproduce

Using the flowing code to reproduce the bug:
```C++
    double qw=0.782240, qx=0.589819, qy=0.199939, qz=0.000000;
    std::printf("Correct quaternion:\n");
    std::printf(
            "qw: %f  qx: %f  qy: %f  qz: %f\n",
            qw, qx, qy, qz
    );

    std::printf("\nIn GTSAM Rot3:\n");
    gtsam::Rot3 a_rotation(qw, qx, qy, qz);
    std::printf(
            "qw: %f  qx: %f  qy: %f  qz: %f\n",
            a_rotation.quaternion().w(),
            a_rotation.quaternion().x(),
            a_rotation.quaternion().y(),
            a_rotation.quaternion().z()
    );
    std::printf(
            "q[0]: %f  q[1]: %f  q[2]: %f  q[3]: %f\n",
            a_rotation.quaternion()[0],
            a_rotation.quaternion()[1],
            a_rotation.quaternion()[2],
            a_rotation.quaternion()[3]
    );
    std::printf("\n\n\n\n");

```

## Expected behavior

From the first 2 lines of the result above we can see that, the correct components of the quaternion should be
`qw: 0.782240  qx: 0.589819  qy: 0.199939  qz: 0.000000`

However, when I construct a `Rot3()` instance with the quaternion constructor and access the components through the getters such as `quaternion().x()`, it returns the wrong component(it returns quaternion().w()).

Though using the index I can get the component in the right order(see the last line of the result), where `quaternion()[0],[1],[2],[3]` corresponding to qw, qx, qy, and qz.

Additionally, after checking the source code, I'm sure the quaternion passed to the `Rot3()` constructor is correct, as shown below:
https://github.com/borglab/gtsam/blob/342f30d148fae84c92ff71705c9e50e0a3683bda/gtsam/geometry/Rot3.h#L121

## Environment
1. GTSAM 4.0.2
2. Ubuntu 18.04
3. GCC & G++ 7.5.0

[成旭元]

Here are the code above and its result, sorry for the formatting issue.

[Dellaert, Frank]

I currently have no time to look into it but if someone can confirm and possibley create a PR that fixes it I’ll review quickly

FD

 

From: gtsam...@googlegroups.com <gtsam...@googlegroups.com> on behalf of 成旭元 <hvikt...@gmail.com>
Date: Saturday, June 4, 2022 at 4:13 AM
To: gtsam users <gtsam...@googlegroups.com>
Subject: [GTSAM] Re: Question about the possible ROT3 quaternion bug?

Here are the code above and its result, sorry for the formatting issue.

--
You received this message because you are subscribed to the Google Groups "gtsam users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to gtsam-users...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/gtsam-users/8ccdcae6-024f-4a0c-8f0f-21da7bef5687n%40googlegroups.com.

[Brice Rebsamen]

This is because `Rot3::quaternion()` returns a `Vector` as `[qw, qx, qy, qz]`, not a quaternion. `Vector` is a typedef for `Eigen::VectorXd`, for which `.x()` returns the first element (so here qw), `.y()` the second (so here qx), etc...

So no bug here, just a non-intuitive API, which admittedly could lead to a bug in user code if they don't pay attention enough. I'd suggest removing this `Rot3::quaternion` function, and only leave the `toQuaternion` function which returns a proper quaternion object...

To view this discussion on the web visit https://groups.google.com/d/msgid/gtsam-users/BN7PR07MB48358FBE16A39352E337479ECFA09%40BN7PR07MB4835.namprd07.prod.outlook.com.

[Dellaert, Frank]

I think that would be a good solution.

[成旭元]

Thank you very much, Brice. After testing, `toQuaternion` does return the proper quaternion components. I'm trying to create a pull request to modify the `Rot3::quaternion` API and make it return a quaternion object instead of the Eigen Vector object. However, A major concern may be that there are many downstream codes relying on the original `quaternion` API.  

[Dellaert, Frank]

You can put “quaternion” into a deprecated block (search for it in the code base) and at least replace all occurrences within GTSAM? PS, unlike what you do below, assign to a temporary so you don’t call 4 times ?

 

From: gtsam...@googlegroups.com <gtsam...@googlegroups.com> on behalf of 成旭元 <hvikt...@gmail.com>
Date: Tuesday, June 7, 2022 at 10:44 AM
To: gtsam users <gtsam...@googlegroups.com>
Subject: Re: [GTSAM] Re: Question about the possible ROT3 quaternion bug?

Thank you very much, Brice. After testing, `toQuaternion` does return the proper quaternion components. I'm trying to create a pull request to modify the `Rot3::quaternion` API and make it return a quaternion object instead of the Eigen Vector object. However, A major concern may be that there are many downstream codes relying on the original `quaternion` API.  

To view this discussion on the web visit https://groups.google.com/d/msgid/gtsam-users/f28e7aed-59d5-4517-b1d2-232df11b6cban%40googlegroups.com.

[成旭元]

Hi Dellaert, we have created a pull request https://github.com/borglab/gtsam/pull/1219 to make a potential fix for this issue, please review when available. 

And for the code occurrences of `Rot3::quaternion`,  we only found 1 usage in  `gtsam_unstable/timing/timeShonanAveraging.cpp`(actually the usage is also commented out), so there seems no need to modify the codes that depend on this API. And we will check how to put the API into a deprecated block.

And for the PS... it's a toy example to demonstrate this issue, will be cautious when writing practical code.

[成旭元]

Hi professor, we have moved `Rot3::quaternion` to a deprecated block, please review https://github.com/borglab/gtsam/pull/1219 when available.

在2022年6月7日星期二 UTC+8 23:22:23<Dellaert, Frank> 写道：

[Varun Agrawal]

I don't understand the deprecation discussion. Normally you would use it like so

```cpp
Vector3 q = rot.quaternion();
std::cout << q[0] << std::endl;  // this is `w`
```

The fact that `.x()` uses Eigen's x-axis method should be obvious from the type information aka `Vector3` and the fact that it is not a `Quaternion` object.

[Dellaert, Frank]

I think it’s definitely confusing. I approved the  PR. We can perfectly do it with the toQuaternion method.

Best!

Frank

---

From: gtsam...@googlegroups.com <gtsam...@googlegroups.com> on behalf of Varun Agrawal <varag...@gmail.com>
Sent: Wednesday, June 8, 2022 12:12:22 PM

To view this discussion on the web visit https://groups.google.com/d/msgid/gtsam-users/167be6e3-7fe5-4d87-86d5-c72204d34717n%40googlegroups.com.

[Varun Agrawal]

The issue is that it affects the wrapper. Eigen::Quaternion isn't wrapped, so if we want quaternions we are SOL since both Python and Matlab expect a vector representation.

[Varun Agrawal]

Maybe nvm. toQuaternion returns gtsam::Quaternion which is indeed wrapped.

[Varun Agrawal]

Something to keep note of, `gtsam::Quaternion`'s `coeffs` method returns the quaternion as `x y z w`. This would be more confusing.

Moreover, do we have solid evidence that the current `.quaternion()` API is confusing? This method was and is intended for the wrapper so I don't see the argument here. Why would someone do

q = R.toQuaternion()

q = [q.w(), q.x(), q.y() q.z()]

instead of the more convenient `q = R.quaternion()`?