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

Introduced inertia/symmetric3 minus operators #2204

Open
wants to merge 6 commits into
base: devel
Choose a base branch
from

Conversation

cmastalli
Copy link
Member

This PR introduces minus operators for symmetric3 and inertia classes. It includes Python bindings and unit tests.

@cmastalli cmastalli changed the title Topic/inertia minus operators Introduced inertia/symmetric3 minus operators Apr 4, 2024
@cmastalli cmastalli added the suggestion API change and new requested features label Apr 4, 2024
Copy link
Contributor

@jcarpent jcarpent left a comment

Choose a reason for hiding this comment

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

Thanks @cmastalli for adding this operation.
I made some comments that you can easily handle to enhance the overall behavior and efficiency.


const Scalar eps = ::Eigen::NumTraits<Scalar>::epsilon();

const Scalar & mab = mass()-Yb.mass();
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to check whether the resulting inertia remains physically consistent, certainly with an assert.

Copy link
Member Author

@cmastalli cmastalli Apr 4, 2024

Choose a reason for hiding this comment

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

I have included asserts for checking mass is positive (here and in FromDynamicParameters). However, I wonder what is the expected design for this class. Should InertialTpl handle fully physically consistent only? Or could it accept "partial" physical consistency or anything?

For instance, the Random function generates positive masses and rotational inertias with positive principal components of inertia. However, this is not what we refer to as fully physical consistency. This condition also implies having positive second moments of inertia, i.e., satisfying the named "triangular inequalities". In short, Random function doesn't support full physical consistency, just a partial physical consistency.

Moreover, FromDynamicParameters doesn't assert positive masses or rotation inertia triangular inequalities. In short, FromDynamicParameters function supports any type of parameter including the ones that don't satisfy physics. It doesn't follow the same convention used in Random.

In conclusion, I am inclined

  1. to allow physically inconsistent inertias (mass and rotational inertia),
  2. to remove asserts (as requested and introduced in this PR), and
  3. to define a "checkConsistency" function where users can trigger their checks on demand.

Please let me know what are your thoughts and how to proceed with this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcarpent -- please let me know your thoughts. I still think we should not include these asserts as resulting operators might not necessarily satisfy fully physical consistency. The reason is this class should change much more to do that. We can arrange a meeting to discuss this.

Be aware that the unit tests are not passing because they are designed to test conditions that don't meet full physical consistency. I'll need to modify them if we want to do that.

include/pinocchio/spatial/symmetric3.hpp Outdated Show resolved Hide resolved
include/pinocchio/spatial/inertia.hpp Outdated Show resolved Hide resolved
include/pinocchio/spatial/inertia.hpp Outdated Show resolved Hide resolved
const Scalar eps = ::Eigen::NumTraits<Scalar>::epsilon();

const Scalar & mab = mass()-Yb.mass();
const Scalar mab_inv = (mass()-Yb.mass() >= 0) ? Scalar(1)/math::max((Scalar)(mass()-Yb.mass()),eps) : -Scalar(1)/math::max((Scalar)(Yb.mass()-mass()),eps);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that the case mab < 0 has a physical meaning.

Copy link
Member Author

Choose a reason for hiding this comment

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

This depends on how we want to design this class. At the moment, it is unclear to me. See my comment at #2204 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion API change and new requested features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants