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

Closed

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.

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).

@jorisv
Copy link
Contributor

jorisv commented Mar 4, 2025

Hello @cmastalli,

Do you still need this PR to be merged ? @jcarpent tell me it was to do some Inertia estimation.
Is the Pseudo Inertia and Log Cholesky Parametrization solving your issue ?

@cmastalli
Copy link
Member Author

Yes, @jorisv!

I apologize if I haven't been able to jump into this. Please let me know how we can proceed. I imagine that the easiest is to pick my changes and reimplement them using the latest devel branch.

@jorisv
Copy link
Contributor

jorisv commented Mar 4, 2025

@cmastalli I will take care of the rebase.

@cmastalli
Copy link
Member Author

@cmastalli I will take care of the rebase.

Thanks :)

@jorisv
Copy link
Contributor

jorisv commented Mar 4, 2025

@cmastalli, I just saw that InertiaTpl InertiaTpl::__minus__(const InertiaTpl & Yb) const and InertiaTpl & InertiarTpl::__mequ__(const InertiaTpl & Yb) are already implemented in devel.

Can you check if the implementation is fine to you ?

@cmastalli
Copy link
Member Author

@cmastalli, I just saw that InertiaTpl InertiaTpl::__minus__(const InertiaTpl & Yb) const and InertiaTpl & InertiarTpl::__mequ__(const InertiaTpl & Yb) are already implemented in devel.

Can you check if the implementation is fine to you ?

I just checked and can confirm that everything looks fine.

I'll close this PR!

@cmastalli cmastalli closed this Mar 4, 2025
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.

3 participants