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

Merge 6.9.0 to main #344

Merged
merged 12 commits into from
Dec 29, 2021
Merged

Merge 6.9.0 to main #344

merged 12 commits into from
Dec 29, 2021

Conversation

scpeters
Copy link
Member

➡️ Forward port

Port ign-math6 as of 6.9.0 to main. Needed by gazebosim/sdformat#803 and gazebo-tooling/release-tools#574

It has compilation warnings that need fixing still

Branch comparison: main...ignition-math6_6.9.0

Note to maintainers: Remember to Merge with commit (not squash-merge or rebase)

chapulina and others added 11 commits September 16, 2021 11:34
* Migration guide and LOCAL2

Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Carlos Agüero <caguero@osrfoundation.org>
Co-authored-by: Steve Peters <scpeters@openrobotics.org>
* 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>
…230)

* Adds python interface to Filter, MovingWindowFilter, RotationSpline.

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

Co-authored-by: Steve Peters <scpeters@openrobotics.org>
Co-authored-by: Franco Cipollone <franco.c@ekumenlabs.com>
* Use python interpreter instead of hardcode python3
* Use previous PythonInterp instead of Python3

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

* find python version 3

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

Co-authored-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Hill Ma <hillma@google.com>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>
Co-authored-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
* Add MACOS install instructions

Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>

* Improve instructions

Signed-off-by: Alejandro Hernández <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

We need to update the python bindings to accommodate the deprecated functions so the warnings are fixed. Since the python API is still in beta, we don't need to tick-tock it, so I say we just update the SWIG files to use the new functions.

chapulina added a commit that referenced this pull request Dec 28, 2021
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor

We need to update the python bindings to accommodate the deprecated functions so the warnings are fixed.

Done in c69f1a9. One of the tests fails for me locally before and after this PR though. I tried a few different things, and even if I don't use abs, the vector's value doesn't look right locally. Let's see what CI says.

ERROR: test_math (__main__.TestQuaternion)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "src/ign-math/src/python/Quaternion_TEST.py", line 191, in test_math
    self.assertAlmostEqual(
  File "/usr/lib/python3.8/unittest/case.py", line 943, in assertAlmostEqual
    diff = abs(first - second)
TypeError: bad operand type for abs(): 'Vector3d'

scpeters pushed a commit that referenced this pull request Dec 28, 2021
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@scpeters scpeters force-pushed the merge_6.9.0_to_main branch from c69f1a9 to 482381d Compare December 28, 2021 07:01
@scpeters
Copy link
Member Author

We need to update the python bindings to accommodate the deprecated functions so the warnings are fixed.

Done in c69f1a9. One of the tests fails for me locally before and after this PR though. I tried a few different things, and even if I don't use abs, the vector's value doesn't look right locally. Let's see what CI says.

ERROR: test_math (__main__.TestQuaternion)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "src/ign-math/src/python/Quaternion_TEST.py", line 191, in test_math
    self.assertAlmostEqual(
  File "/usr/lib/python3.8/unittest/case.py", line 943, in assertAlmostEqual
    diff = abs(first - second)
TypeError: bad operand type for abs(): 'Vector3d'

I fixed a few more instances of deprecated APIs being called in python tests and amended c69f1a9 to 482381d and force-pushed

@chapulina
Copy link
Contributor

I fixed a few more instances of deprecated APIs being called in python tests and amended c69f1a9 to 482381d and force-pushed

It's still failing. I've been looking into it, I think it's because of some of the API changes on main for constructors and accessors. For example, this test failure:

  FAIL: test_pose (__main__.TestPose3)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/github/workspace/src/python/Pose3_TEST.py", line 44, in test_pose
      self.assertAlmostEqual((B + A).pos().x(), 1.0 + 1.0/math.sqrt(2))
  AssertionError: 0.0 != 1.7071067811865475 within 7 places (1.7071067811865475 difference)

Is fixed if we use (B + A).x() instead of (B + A).pos().x().

chapulina added a commit that referenced this pull request Dec 29, 2021
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@chapulina
Copy link
Contributor

Updated the last commit from 482381d to 84837c2. I changed some tests to workaround the failures, and commented out others. I still don't know what's the issue, but I suspect it's related to the Vector3 copy constructor. I tried changing that from default to an implementation though and that didn't help 🤷‍♀️

@codecov
Copy link

codecov bot commented Dec 29, 2021

Codecov Report

Merging #344 (237fab5) into main (04a4c85) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #344      +/-   ##
==========================================
- Coverage   99.27%   99.23%   -0.04%     
==========================================
  Files          67       68       +1     
  Lines        6077     6305     +228     
==========================================
+ Hits         6033     6257     +224     
- Misses         44       48       +4     
Impacted Files Coverage Δ
include/ignition/math/Box.hh 100.00% <ø> (ø)
include/ignition/math/Sphere.hh 100.00% <ø> (ø)
include/ignition/math/Vector3.hh 98.84% <ø> (ø)
include/ignition/math/Line3.hh 100.00% <100.00%> (ø)
include/ignition/math/Plane.hh 100.00% <100.00%> (ø)
include/ignition/math/Pose3.hh 100.00% <100.00%> (ø)
include/ignition/math/detail/Box.hh 100.00% <100.00%> (ø)
include/ignition/math/detail/Sphere.hh 100.00% <100.00%> (ø)
include/ignition/math/detail/WellOrderedVector.hh 100.00% <100.00%> (ø)
src/SphericalCoordinates.cc 100.00% <100.00%> (ø)
... and 2 more

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 04a4c85...237fab5. Read the comment docs.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Added TODOs in 237fab5 to revisit the quaternion python tests after migrating to pybind11. I think there's little point in improving the SWIG python implementation right now. It may become important when we use it for Ruby though.

@scpeters scpeters merged commit 02fcef7 into main Dec 29, 2021
@scpeters scpeters deleted the merge_6.9.0_to_main branch December 29, 2021 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants