Subtraction operator for Inertial class by deepanshubansal01 · Pull Request #432 · gazebosim/gz-math

deepanshubansal01 · 2022-05-24T23:20:45Z

Summary

This PR adds a feature for minus operator in inertial class, full description here #423

Checklist

Signed all commits for DCO
Added tests
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
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

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

scpeters · 2022-05-24T23:29:56Z

please add tests to Inertial_TEST.cc

codecov · 2022-05-24T23:32:31Z

Codecov Report

Merging #432 (0142b93) into main (d6284a5) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #432      +/-   ##
==========================================
- Coverage   99.84%   99.69%   -0.15%     
==========================================
  Files          49       73      +24     
  Lines        4396     6588    +2192     
==========================================
+ Hits         4389     6568    +2179     
- Misses          7       20      +13

Impacted Files	Coverage Δ
include/gz/math/Inertial.hh	`100.00% <100.00%> (ø)`
.../install/include/ignition/math7/gz/math/Vector4.hh
...ke/install/include/ignition/math7/gz/math/Angle.hh
...install/include/ignition/math7/gz/math/Cylinder.hh
...nstall/include/ignition/math7/gz/math/Triangle3.hh
.../install/include/ignition/math7/gz/math/Vector2.hh
...stall/include/ignition/math7/gz/math/Quaternion.hh
...clude/ignition/math7/gz/math/MovingWindowFilter.hh
...all/include/ignition/math7/gz/math/SpeedLimiter.hh
.../install/include/ignition/math7/gz/math/Helpers.hh
... and 113 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 d6284a5...0142b93. Read the comment docs.

deepanshubansal01 · 2022-05-25T15:59:31Z

Based on the initial testing of -operator with two cubes, it seems to work. Right now a separate negative operator has been implemented.

After implementation it seems like we might be able to piggy bank on the +operator for -operator as suggested by @adityapande-1995. If we agree on piggy banking on +operator, we will have to reverse the signs of some of the fields(inertia matrix and mass) of the Inertial object, which seems reasonable?

deepanshubansal01 · 2022-05-25T16:05:03Z

Based on the initial testing of -operator with two cubes, it seems to work. Right now a separate negative operator has been implemented.

After implementation it seems like we might be able to piggy bank on the +operator for -operator as suggested by @adityapande-1995. If we agree on piggy banking on +operator, we will have to reverse the signs all the fields(pose, inertia matrix and mass) of the Inertial object, which seems reasonable?

Also, one small thing though that since Inertial objects will be passed as const in -operator we might need to copy the Inertial object first then change it's signs and pass it to +operator?

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

scpeters · 2022-05-25T21:53:18Z

include/gz/math/Inertial.hh

+     public: Inertial<T> &operator-=(const Inertial<T> &_inertial)
+      {
+          MassMatrix3<T> _mass_matrix_copy(-_inertial.MassMatrix().Mass(),
+              -_inertial.MassMatrix().DiagonalMoments(), -_inertial.MassMatrix().OffDiagonalMoments());


I'm not sure this is correct. This MassMatrix object contains invalid inertias

I guess the MassMatrix object is invalid on it's own but the reason for doing this was to make it work with +operator.

Inside +operator this is what happens:
auto moi = this->Moi() + _inertial.Moi();
What we want for -operator is this:
auto moi = this->Moi() - _inertial.Moi();

So we make the _inertial.Moi() negative before sending it to +operator

I can also have a separate implementation for -=operator if this creates confusion.

I think it would be better to have a separate -=operator implementation

scpeters · 2022-05-25T21:54:43Z

src/Inertial_TEST.cc

+    const math::Vector3d size(1, 1, 1);
+    math::MassMatrix3d cubeMM3;
+    EXPECT_TRUE(cubeMM3.SetFromBox(mass, size));
+    const math::Inertiald cube(cubeMM3, math::Pose3d::Zero);


I think you could add expectations for the - operator in the Addition test instead of duplicating everything here. The test could be renamed as AdditionSubtraction

yeah, makes sense. I was also thinking to add some more unit tests for subtraction operator different from addition operator. Should I add them as well inside AdditionSubtraction

I think the tests can be shared in most cases. Anywhere that you would test a + b == c, you can also test c - a == b and c - b == a and it would prevent a lot of duplicated code

…usion Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

scpeters · 2022-05-26T18:59:15Z

src/Inertial_TEST.cc

+      EXPECT_EQ(left, tmp);
+    }
+    {
+      math::Inertiald tmp = right;


tmp = cube; here I think?

yeah. Let me check real quick. Sorry about that.

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

scpeters · 2022-05-26T20:17:28Z

are you up for adding this to the python bindings as well?

https://github.com/gazebosim/gz-math/blob/ign-math6/src/python_pybind11/src/Inertial.hh#L59-L60

deepanshubansal01 · 2022-05-26T20:19:01Z

are you up for adding this to the python bindings as well?

https://github.com/gazebosim/gz-math/blob/ign-math6/src/python_pybind11/src/Inertial.hh#L59-L60

yes, I can do that as well.

deepanshubansal01 · 2022-05-26T20:20:26Z

are you up for adding this to the python bindings as well?

https://github.com/gazebosim/gz-math/blob/ign-math6/src/python_pybind11/src/Inertial.hh#L59-L60

yes, I can do that as well.

I am currently in the middle of another issue, so I will start on this by tomorrow.

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

deepanshubansal01 · 2022-05-26T23:09:50Z

are you up for adding this to the python bindings as well?

https://github.com/gazebosim/gz-math/blob/ign-math6/src/python_pybind11/src/Inertial.hh#L59-L60

@scpeters I have added the python binding and unit test as well. Thanks!

scpeters · 2022-06-01T03:38:51Z

src/Inertial_TEST.cc

    math::MassMatrix3d trueCubeMM3;
    EXPECT_TRUE(trueCubeMM3.SetFromBox(8*mass, 2*size));
    EXPECT_EQ(addedCube, math::Inertiald(trueCubeMM3, math::Pose3d::Zero));
+    EXPECT_EQ(lastCube, addedCube - sevenCubes);


nit: let's add EXPECT_EQ(sevenCubes, addedCube - lastCube); as well

scpeters · 2022-06-01T03:39:36Z

src/Inertial_TEST.cc

    math::MassMatrix3d trueCubeMM3;
    EXPECT_TRUE(trueCubeMM3.SetFromBox(8*mass, 2*size));
    EXPECT_EQ(addedCube, math::Inertiald(trueCubeMM3, math::Pose3d::Zero));
+    EXPECT_EQ(lastCube, addedCube - sevenCubes);


add EXPECT_EQ(sevenCubes, addedCube - lastCube); as well

scpeters · 2022-06-01T04:25:06Z

src/Inertial_TEST.cc

+    EXPECT_EQ(
+        math::Pose3d(-0.5, -0.5, -0.5, 0, 0, 0),
+        (diagonalCubes - cube2).Pose());
+    EXPECT_EQ(
+        math::Pose3d(0.5,  0.5, 0.5, 0, 0, 0),
+        (diagonalCubes - cube1).Pose());


Suggested change

EXPECT_EQ(

math::Pose3d(-0.5, -0.5, -0.5, 0, 0, 0),

(diagonalCubes - cube2).Pose());

EXPECT_EQ(

math::Pose3d(0.5, 0.5, 0.5, 0, 0, 0),

(diagonalCubes - cube1).Pose());

EXPECT_EQ(cube1.Pose(), (diagonalCubes - cube2).Pose());

EXPECT_EQ(cube2.Pose(), (diagonalCubes - cube1).Pose());

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

scpeters · 2022-06-01T21:13:25Z

include/gz/math/Inertial.hh

+        // Only continue if remaining mass is positive
+        if (m1 <= 0)
+        {
+          return *this;


we should also add a test case that exercises this line of code. It can be in a separate test case than AdditionSubtraction, like InvalidSubtraction or something like that

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

scpeters

looks great! thanks for all the changes and improvements.

when merging, please edit the commit message as suggested in our contributing guide

https://gazebosim.org/docs/all/contributing#process

Default to “squash and merge”
Review the pull request title and reword if necessary since this will be part of the commit message.
Make sure the commit message concisely captures the core ideas of the pull request and contains all authors' signatures.

Added initial work on subtraction operator

f8e9375

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

deepanshubansal01 requested a review from scpeters as a code owner May 24, 2022 23:20

github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel labels May 24, 2022

deepanshubansal01 changed the base branch from ign-math6 to main May 24, 2022 23:21

add + operator inside ruby file

e36c314

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

deepanshubansal01 requested a review from adityapande-1995 May 24, 2022 23:31

deepanshubansal01 marked this pull request as draft May 24, 2022 23:35

deepanshubansal01 added 3 commits May 25, 2022 12:41

unit test subtract operator with two half cubes

a52d5bb

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

unit test two rotated half cubes

5d978f6

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

piggy bank on +operator for -operator

fc2b01d

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

scpeters reviewed May 25, 2022

View reviewed changes

adityapande-1995 marked this pull request as ready for review May 25, 2022 22:26

deepanshubansal01 added 4 commits May 25, 2022 18:26

implement -=operator separately instead of piggybanking to avoid conf…

0ed01be

…usion Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

add -operator unit test along with +operator

8508889

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

add remaining unit tests for -operator

286086c

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

make codecheck

7689ede

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

deepanshubansal01 self-assigned this May 26, 2022

scpeters reviewed May 26, 2022

View reviewed changes

correct unit test bug

48cc689

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

added python binding and unit test

9b7f131

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

Merge branch 'main' into deepanshu/inertial-subtract-operator

3ffe8eb

scpeters reviewed Jun 1, 2022

View reviewed changes

code review changes

fb94579

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

scpeters reviewed Jun 1, 2022

View reviewed changes

added subtraction invalid unit test

0142b93

Signed-off-by: deepanshu <deepanshubansal01@gmail.com>

scpeters approved these changes Jun 2, 2022

View reviewed changes

deepanshubansal01 merged commit b250500 into main Jun 2, 2022

deepanshubansal01 deleted the deepanshu/inertial-subtract-operator branch June 2, 2022 14:44

deepanshubansal01 mentioned this pull request Jun 2, 2022

Subtraction operator for Inertial class #423

Closed

Conversation

deepanshubansal01 commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

scpeters commented May 24, 2022

Uh oh!

codecov bot commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

deepanshubansal01 commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepanshubansal01 commented May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepanshubansal01 May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepanshubansal01 May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deepanshubansal01 May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scpeters commented May 26, 2022

Uh oh!

deepanshubansal01 commented May 26, 2022

Uh oh!

deepanshubansal01 commented May 26, 2022

Uh oh!

deepanshubansal01 commented May 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scpeters left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

deepanshubansal01 commented May 24, 2022 •

edited

Loading

codecov bot commented May 24, 2022 •

edited

Loading

deepanshubansal01 commented May 25, 2022 •

edited

Loading

deepanshubansal01 commented May 25, 2022 •

edited

Loading

deepanshubansal01 May 25, 2022 •

edited

Loading

deepanshubansal01 May 25, 2022 •

edited

Loading

deepanshubansal01 May 25, 2022 •

edited

Loading