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

Subtraction operator for Inertial class #432

Merged
merged 14 commits into from
Jun 2, 2022

Conversation

deepanshubansal01
Copy link
Contributor

@deepanshubansal01 deepanshubansal01 commented May 24, 2022

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

@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel labels May 24, 2022
@deepanshubansal01 deepanshubansal01 changed the base branch from ign-math6 to main May 24, 2022 23:21
@scpeters
Copy link
Member

please add tests to Inertial_TEST.cc

@codecov
Copy link

codecov bot commented May 24, 2022

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 deepanshubansal01 marked this pull request as draft May 24, 2022 23:35
@deepanshubansal01
Copy link
Contributor Author

deepanshubansal01 commented May 25, 2022

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
Copy link
Contributor Author

deepanshubansal01 commented May 25, 2022

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?

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@deepanshubansal01 deepanshubansal01 May 25, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@deepanshubansal01 deepanshubansal01 May 25, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@deepanshubansal01 deepanshubansal01 May 25, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@adityapande-1995 adityapande-1995 marked this pull request as ready for review May 25, 2022 22:26
@deepanshubansal01 deepanshubansal01 self-assigned this May 26, 2022
EXPECT_EQ(left, tmp);
}
{
math::Inertiald tmp = right;
Copy link
Member

Choose a reason for hiding this comment

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

tmp = cube; here I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Signed-off-by: deepanshu <[email protected]>
@scpeters
Copy link
Member

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

@deepanshubansal01
Copy link
Contributor Author

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

yes, I can do that as well.

@deepanshubansal01
Copy link
Contributor Author

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

yes, I can do that as well.

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

@deepanshubansal01
Copy link
Contributor Author

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

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


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);
Copy link
Member

Choose a reason for hiding this comment

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

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


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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines 598 to 603
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());
Copy link
Member

Choose a reason for hiding this comment

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

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 <[email protected]>
// Only continue if remaining mass is positive
if (m1 <= 0)
{
return *this;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On it!

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

looks great! thanks for all the changes and improvements.

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

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.

@deepanshubansal01 deepanshubansal01 merged commit b250500 into main Jun 2, 2022
@deepanshubansal01 deepanshubansal01 deleted the deepanshu/inertial-subtract-operator branch June 2, 2022 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🏯 fortress Ignition Fortress Gazebo 1️1️ Dependency of Gazebo classic version 11
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants