Introduced inertia/symmetric3 minus operators #2204
Conversation
cmastalli
changed the title
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
It would be nice to check whether the resulting inertia remains physically consistent, certainly with an assert.
There was a problem hiding this comment.
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
- to allow physically inconsistent inertias (mass and rotational inertia),
- to remove asserts (as requested and introduced in this PR), and
- 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.
There was a problem hiding this comment.
@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); |
There was a problem hiding this comment.
I don't think that the case mab < 0 has a physical meaning.
There was a problem hiding this comment.
This depends on how we want to design this class. At the moment, it is unclear to me. See my comment at #2204 (comment).
jcarpent
force-pushed
the
topic/inertia-minus-operators
branch
from
b5f30fc to
13d6acb
Compare
|
Hello @cmastalli, Do you still need this PR to be merged ? @jcarpent tell me it was to do some Inertia estimation. |
|
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. |
|
@cmastalli I will take care of the rebase. |
Thanks :) |
|
@cmastalli, I just saw that 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! |
This PR introduces minus operators for symmetric3 and inertia classes. It includes Python bindings and unit tests.