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

Adds python interface to Quaternion, Pose3, Matrix3 and Matrix4 #221

Merged
merged 22 commits into from
Sep 17, 2021

Conversation

LolaSegura
Copy link
Contributor

@LolaSegura LolaSegura commented Aug 17, 2021

🎉 New feature

Goes on top of #216

Related to #101 #210

Summary

Adds Python interface for three math classes: Quaternion, Pose3, Matrix3, Matrix4. For each class a python test has been created.

Related issues and notes

Quaternion

  • To offer the serialization functionality in python, an extension of the operator<< had to be made. This adds a __str()__ method that calls operator<< inside.
  • The getter methods implemented in C++ returned a reference value. This wasn't being interpreted correctly in python. So the implementation of W(), X(), Y() and Z() was re-defined using %extend().
  • The method void ToAxis(Vector3<T> &_axis, T &_angle) const; is currently not being supported because both parameters (_axis and _angle) are being modify inside it and there is not a straight forward implementation for these cases when using swig.
  • There is a circular dependency between Quaternion and Matrix3. This genereates and error when the binding between C++ and python is being generated.
    • The solution we implemented was not giving support to the Quaternion constructor from a Matrix3 (which was the only method that was using the Matrix3 class) and add a python method to Matrix3 that makes this transformation. We used the %extend command and called the Quaternion(Matrix3<t> _mat); constructor inside it.
    • Another option could be that: instead of defining the Quaternion templates inside the Quaternion.i file, like is being done at the moment
           namespace math {
               template<typename T>
               class Quaternion {
                  /* methods */
               };
               %template(Quaternioni) Quaternion<int>;
               %template(Quaterniond) Quaternion<double>;
               %template(Quaternionf) Quaternion<float>;
           } //  math
           } //  ignition
      
    define them inside the Matrix3.i file, and add an extension of the matrix method that is being called in the constructor from the Quaternion class:
        namespace math {
            template<typename T>
            class Matrix3 {
               /* methods */
            };
            %extend Quaternion {
                 void matrix(const Matrix3<T> &_mat) {
                     (*$self).Matrix(_mat);
                 }
            }
             %template(Quaternioni) Quaternion<int>;
             %template(Quaterniond) Quaternion<double>;
             %template(Quaternionf) Quaternion<float>;
         } //  math
         } //  ignition
    

This last approach would imply that for using Quaternion in python the class Matrix3 has to be bind.

Pose3

  • Inside the Pose3 class the Reset() method resets the value of the quaternion as: this->q = Quaternion<double> ::Identity I change this to this->q = Quaternion<T> ::Identity because the implementation would only work when the template was of type double. '
  • To offer the serialization functionality in python, and extension of the operator<< had to be made. This adds a __str()__ method that calls operator<< inside.

Matrix3

  • To offer the serialization functionality in python, and extension of the operator<< had to be made. This adds a __str()__ method that calls operator<< inside.
  • To offer the operator() method functionality in python, the class had to be extended in the interface file.

Matrix4

  • Inside the Matrix4_TEST.ccon the constructor test both for loops where testing EXPECT_DOUBLE_EQ(mat2(i, **i**), 0.0);, so only the elements on the diagonal were tested. I changed to EXPECT_DOUBLE_EQ(mat2(i, **j**), 0.0);
  • To offer the serialization functionality in python, and extension of the operator<< had to be made. This adds a __str()__ method that calls operator<< inside.
  • To offer the operator() method functionality in python, the class had to be extended in the interface file.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Aug 17, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Aug 17, 2021
@codecov
Copy link

codecov bot commented Aug 17, 2021

Codecov Report

Merging #221 (2e7e7bd) into ign-math6 (46d5324) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           ign-math6     #221   +/-   ##
==========================================
  Coverage      99.40%   99.40%           
==========================================
  Files             66       66           
  Lines           6185     6185           
==========================================
  Hits            6148     6148           
  Misses            37       37           
Impacted Files Coverage Δ
include/ignition/math/Pose3.hh 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46d5324...2e7e7bd. Read the comment docs.

@LolaSegura LolaSegura force-pushed the LolaSegura/add_matrix_pose_quaternion branch from 6f5bb13 to ad00569 Compare August 18, 2021 19:14
@chapulina chapulina moved this from Inbox to In review in Core development Aug 19, 2021
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
…tyle.

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
…hon test to the python folder.

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
@LolaSegura LolaSegura force-pushed the LolaSegura/add_matrix_pose_quaternion branch from ad00569 to 88dc6f6 Compare August 20, 2021 19:52
Copy link
Contributor

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

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

Good job! Just a few comments.

src/python/Matrix3.i Outdated Show resolved Hide resolved
src/python/Matrix3.i Outdated Show resolved Hide resolved
src/python/Matrix3_TEST.py Outdated Show resolved Hide resolved
src/python/Matrix4.i Outdated Show resolved Hide resolved
src/python/Matrix4_TEST.py Outdated Show resolved Hide resolved
src/python/Pose3.i Show resolved Hide resolved
src/python/Pose3_TEST.py Outdated Show resolved Hide resolved
src/python/Pose3_TEST.py Outdated Show resolved Hide resolved
src/python/Quaternion_TEST.py Outdated Show resolved Hide resolved
src/python/python.i Outdated Show resolved Hide resolved
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
LolaSegura and others added 3 commits August 25, 2021 12:23
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
@francocipollone
Copy link
Contributor

@LolaSegura

The method void ToAxis(Vector3 &_axis, T &_angle) const; is currently not being supported because both parameters (_axis and _angle) are being modify inside it and there is not a straight forward implementation for these cases when using swig.

I've pushed a workaround at f394e9c for adding coverage to ToAxis method. PTAL

Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
@francocipollone
Copy link
Contributor

I've just fixed conflicts.

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021
@scpeters
Copy link
Member

I'm almost done reviewing this; looks good so far, and you've found bugs in our c++ code! Great work

This changes the test matrices that are multiplied
togther so that they aren't scalar multiples of each other.
This confirms non-commutativity in the test.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Member

I noticed a problem in our Matrix3_TEST.cc with regard to testing non-commutativity (order of multiplication arguments matters), and I updated both c++ and python tests in c541db4

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Member

I added a stream out test to Matrix3_TEST.py in bb6b120

@scpeters
Copy link
Member

there's one final thing that I've noticed and will file in an issue: the python tests are missing some statement in which scalars are left multiplied to Vector and Matrix types (mat * 0 is tested but not 0 * mat). I tried adding these test statements, but they fail (probably why they were omitted). For Matrix3, I believe this is because the friend operator at Matrix3.hh:331-338 is not included in Matrix3.i. I will file an issue for this because it also applies to the Vector classes

@scpeters
Copy link
Member

I will file an issue for this because it also applies to the Vector classes

#241

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters merged commit b54dce5 into ign-math6 Sep 17, 2021
@scpeters scpeters deleted the LolaSegura/add_matrix_pose_quaternion branch September 17, 2021 01:19
Core development automation moved this from In review to Done Sep 17, 2021
@francocipollone
Copy link
Contributor

there's one final thing that I've noticed and will file in an issue: the python tests are missing some statement in which scalars are left multiplied to Vector and Matrix types (mat * 0 is tested but not 0 * mat). I tried adding these test statements, but they fail (probably why they were omitted). For Matrix3, I believe this is because the friend operator at Matrix3.hh:331-338 is not included in Matrix3.i. I will file an issue for this because it also applies to the Vector classes

Nice catch. Thanks for filling the issue.

arjo129 pushed a commit that referenced this pull request Sep 20, 2021
* Adds scripting interface to Quaternion and a python test
* Adds scripting interface to Matrix3 and a python test
* Adds scripting interface to Pose3 and a python test
* Solves bug in the Reset() method inside Pose3
* Adds scripting interface to Matrix4 and a python test
* Solves bug in the Construct test for Matrix4 class
* Adds %rename tag to interface files in order to match pep-8 naiming style.
* Adds a python method to convert from a Matrix3 to a Quaternion.
* Adds to_quaternion() method to Matrix3.

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>

* Adds python binding for Quaternion::ToAxis method.

Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>

* Matrix3_TEST: improve multiplication test

This changes the test matrices that are multiplied
togther so that they aren't scalar multiples of each other.
This confirms non-commutativity in the test.

* Matrix3_TEST.py: add stream out test

Signed-off-by: Steve Peters <scpeters@openrobotics.org>

Co-authored-by: Franco Cipollone <franco.c@ekumenlabs.com>
Co-authored-by: Franco Cipollone <53065142+francocipollone@users.noreply.github.com>
Co-authored-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants