-
Notifications
You must be signed in to change notification settings - Fork 401
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
Different inertia parametrizations and Physical consistency #2296
Conversation
@jcarpent Do you think is a good idea to create dedicated type to store pseudo inertia and log cholesky parametrization ?
|
Thanks @simeon-ned for this PR and thanks @jorisv for making a first feedback. |
@jcarpent Sure, looking forward to your reply! Tag me anytime, and I'll be glad to assist. Just a note, I'm not very proficient in C++, but will do my best :) |
To move beyond, I do agree with @jorisv suggestions.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dear @simeon-ned,
Your PR provides a great enhancement to Pinocchio thanks a lot.
It would be nice to add in the doc some references to the papers you have mentioned.
Additionally, to enable proper differentiation, we might think adding dedicated types for parametrizing inertias, in such a way we can easily extract and compute Jacobians, and enable conversions between parametrization externally to the main InertiaTpl class.
Thanks again for contributing to Pinocchio, this definitely enriches the library.
Don't forget to add your name in the contributor list ;)
@jcarpent, sure, I will add more information in the docstrings, include myself in the contribution list, and work on implementing @jorisv's proposition regarding separate classes for log-Cholesky and pseudo inertia. Meanwhile, I would like to discuss the conversion to log-Cholesky parameters from dynamic and pseudo inertia. I have drafted a Python implementation for this conversion, as shown below: def pseudo2logcholesky(pseudo_inertia: np.ndarray) -> np.ndarray:
n = pseudo_inertia.shape[0]
indices = np.arange(n - 1, -1, -1) # Indices to reverse the order
# Apply the inversion using indices for rows and columns
reversed_inertia = pseudo_inertia[indices][:, indices]
# Perform Cholesky decomposition on the permuted matrix A'
L_prime = np.linalg.cholesky(reversed_inertia)
# Apply the reverse permutation to L_prime and transpose it to form U
U = L_prime[indices][:, indices]
alpha = np.log(U[3, 3])
d1 = np.log(U[0, 0] / U[3, 3])
d2 = np.log(U[1, 1] / U[3, 3])
d3 = np.log(U[2, 2] / U[3, 3])
s12 = U[0, 1] / U[3, 3]
s23 = U[1, 2] / U[3, 3]
s13 = U[0, 2] / U[3, 3]
t1 = U[0, 3] / U[3, 3]
t2 = U[1, 3] / U[3, 3]
t3 = U[2, 3] / U[3, 3]
return np.array([alpha, d1, d2, d3, s12, s23, s13, t1, t2, t3]) However, I am a bit confused about how to achieve the equivalent reversal of order in I certainly can achieve this with multiplication by permutation matrices, but this may not be the most efficient approach. If you have any tips or suggestions on how to handle this in C++ using Eigen in a more efficient manner, it would be greatly appreciated. |
@jcarpent, I hope you’re doing well. I’ve reworked the code to better align with @jorisv's suggestions. I’ve introduced two new structures in The I’ve also added a dedicated cpp test in Additionally, I have included references to relevant papers in the docstrings for the new types to provide more context. I've also added myself to the contributors list :) The remaining task is the implementation of Python bindings. I'm not very familiar with Boost.Python, so I'll give it a try but might need some help if you can offer it. Please let me know if anything else is needed or if there are further improvements that can be made. Thanks so much for your time and feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @simeon-ned,
Thanks for your contribution.
I have reviewed some of your change.
- Don't hesitate to discuss the review
- If you lake of time and agree with the review I can apply it myself
- For the Boost.Python binding:
- I can give you some guidance
- I can do it myself if you're not interested to learn about Boost.Python
Hello @jorisv, I've made progress on addressing the issues you mentioned in the C++ part of the code and updated the related unit tests. However, I'm still facing challenges with modifying the Python bindings. Here's what I've done so far:
The new methods are now visible in Could you please provide some help with that? Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @simeon-ned,
You did the most complicated part.
Now you can edit this file: https://github.com/stack-of-tasks/pinocchio/blob/master/bindings/python/spatial/expose-inertia.cpp
and add
PseudoInertiaPythonVisitor<context::PseudoInertia>::expose();
LogCholeskyParametersPythonVisitor<context::LogCholeskyParameters>::expose();
Don't forget to add PseudoInertia and LogCholeskyParameters typedef in include/pinocchio/bindings/python/context/generic.hpp
.
You can also add binding test in unittest/python/bindings_inertia.py
.
Hello @jorisv, I will take over this PR moving forward. The CI seems to be running, and I have addressed most of your previous comments. However, I have just discovered that the bindings are broken, I will update it shortly. I also have a few open questions that I hope you can help me with:
Looking forward to your feedback! Best regards, |
Just a quick update. I have managed to fix the python bindings and it works well in testing. While doing that I have partially answered the first question about template for From my understanding, it helps to cover these casts when dealing with python bindings and eigenpy. Other than that, I think I will just wait for the feedback from now. I believe I have completed most of the checkpoints for this PR to be ready :) |
Hello @simeon-ned and @lvjonok,
I don't think it's mandatory to have consistency between InertiaTpl, PseudoInertiaTpl and LogCholeskyTpl. Since we don't plan to be able to replace one of these type by another, we don't really need to have a common set of methods between them.
The cast visitor allow to cast to a different scalar type. It's useful if you want to convert an InertiaTpl made of double to an InertiaTpl made of casadi/cppad/cppadcg/boost.mpf scalar. I think is still a good idea to support it. |
I will work on the crash issue since it also affect InertiaTpl. If you want I can also address myself the last issues since it's really minor. |
Yes, sure. Thanks for explanations, it really matters |
Co-authored-by: Joris Vaillant <[email protected]>
All green we can merge. @simeon-ned, @lvjonok once again, thank you for your contribution ! |
## [3.3.0] - 2024-11-06 ### Added - Default visualizer can be changed with `PINOCCHIO_VIEWER` environment variable stack-of-tasks/pinocchio#2419 - Add more Python and C++ examples related to inverse kinematics with 3d tasks stack-of-tasks/pinocchio#2428 - Add parsing of equality/connect tag for closed-loop chains for MJCF format stack-of-tasks/pinocchio#2413 - Add compatibility with NumPy 2 `__array__` API stack-of-tasks/pinocchio#2436 - Added argument to let users decide of root joint name when parsing models (urdf, mjcf, sdf) stack-of-tasks/pinocchio#2402 - Allow use of `pathlib.Path | str` for paths in python bindings stack-of-tasks/pinocchio#2431 - Add Pseudo inertia and Log-cholesky parametrization stack-of-tasks/pinocchio#2296 - Add Pixi support stack-of-tasks/pinocchio#2459 ### Fixed - Fix linkage of Boost.Serialization on Windows stack-of-tasks/pinocchio#2400 - Fix mjcf parser appending of inertias at root joint stack-of-tasks/pinocchio#2403 - Fix unit tests with GCC 13.3 stack-of-tasks/pinocchio#2416) - Fix class abstract error for Rviz viewer stack-of-tasks/pinocchio#2425 - Fix compilation issue with MSCV and C++17 stack-of-tasks/pinocchio#2437 - Fix `pinocchio-test-py-robot_wrapper` when building with SDF and collision support stack-of-tasks/pinocchio#2437 - Fix crash when calling `Inertia::FromDynamicParameters` in Python with wrong vector size stack-of-tasks/pinocchio#2296 - Fix `examples/cassie-simulation.py` and `examples/talos-simulation.py` stack-of-tasks/pinocchio#2443 - Fix build with CppAd 2024 stack-of-tasks/pinocchio#2459 - Fix `pinocchio-test-cpp-mjcf` unittest with Boost 1.86 stack-of-tasks/pinocchio#2459 - Fix `pinocchio-test-cpp-constraint-variants` uninitialized values stack-of-tasks/pinocchio#2459 - Fix mixing library symbols between Pinocchio scalar bindings stack-of-tasks/pinocchio#2459 - Fix bug for get{Joint,Frame}JacobianTimeVariation stack-of-tasks/pinocchio#2466 ### Changed - Modernize python code base with ruff stack-of-tasks/pinocchio#2418 - Does not create a root_joint frame from parsed models (urdf, mjcf and sdf) when no root joint is provided stack-of-tasks/pinocchio#2402
## [3.3.0] - 2024-11-06 ### Added - Default visualizer can be changed with `PINOCCHIO_VIEWER` environment variable stack-of-tasks/pinocchio#2419 - Add more Python and C++ examples related to inverse kinematics with 3d tasks stack-of-tasks/pinocchio#2428 - Add parsing of equality/connect tag for closed-loop chains for MJCF format stack-of-tasks/pinocchio#2413 - Add compatibility with NumPy 2 `__array__` API stack-of-tasks/pinocchio#2436 - Added argument to let users decide of root joint name when parsing models (urdf, mjcf, sdf) stack-of-tasks/pinocchio#2402 - Allow use of `pathlib.Path | str` for paths in python bindings stack-of-tasks/pinocchio#2431 - Add Pseudo inertia and Log-cholesky parametrization stack-of-tasks/pinocchio#2296 - Add Pixi support stack-of-tasks/pinocchio#2459 ### Fixed - Fix linkage of Boost.Serialization on Windows stack-of-tasks/pinocchio#2400 - Fix mjcf parser appending of inertias at root joint stack-of-tasks/pinocchio#2403 - Fix unit tests with GCC 13.3 stack-of-tasks/pinocchio#2416) - Fix class abstract error for Rviz viewer stack-of-tasks/pinocchio#2425 - Fix compilation issue with MSCV and C++17 stack-of-tasks/pinocchio#2437 - Fix `pinocchio-test-py-robot_wrapper` when building with SDF and collision support stack-of-tasks/pinocchio#2437 - Fix crash when calling `Inertia::FromDynamicParameters` in Python with wrong vector size stack-of-tasks/pinocchio#2296 - Fix `examples/cassie-simulation.py` and `examples/talos-simulation.py` stack-of-tasks/pinocchio#2443 - Fix build with CppAd 2024 stack-of-tasks/pinocchio#2459 - Fix `pinocchio-test-cpp-mjcf` unittest with Boost 1.86 stack-of-tasks/pinocchio#2459 - Fix `pinocchio-test-cpp-constraint-variants` uninitialized values stack-of-tasks/pinocchio#2459 - Fix mixing library symbols between Pinocchio scalar bindings stack-of-tasks/pinocchio#2459 - Fix bug for get{Joint,Frame}JacobianTimeVariation stack-of-tasks/pinocchio#2466 ### Changed - Modernize python code base with ruff stack-of-tasks/pinocchio#2418 - Does not create a root_joint frame from parsed models (urdf, mjcf and sdf) when no root joint is provided stack-of-tasks/pinocchio#2402
## [3.3.0] - 2024-11-06 ### Added - Default visualizer can be changed with `PINOCCHIO_VIEWER` environment variable stack-of-tasks/pinocchio#2419 - Add more Python and C++ examples related to inverse kinematics with 3d tasks stack-of-tasks/pinocchio#2428 - Add parsing of equality/connect tag for closed-loop chains for MJCF format stack-of-tasks/pinocchio#2413 - Add compatibility with NumPy 2 `__array__` API stack-of-tasks/pinocchio#2436 - Added argument to let users decide of root joint name when parsing models (urdf, mjcf, sdf) stack-of-tasks/pinocchio#2402 - Allow use of `pathlib.Path | str` for paths in python bindings stack-of-tasks/pinocchio#2431 - Add Pseudo inertia and Log-cholesky parametrization stack-of-tasks/pinocchio#2296 - Add Pixi support stack-of-tasks/pinocchio#2459 ### Fixed - Fix linkage of Boost.Serialization on Windows stack-of-tasks/pinocchio#2400 - Fix mjcf parser appending of inertias at root joint stack-of-tasks/pinocchio#2403 - Fix unit tests with GCC 13.3 stack-of-tasks/pinocchio#2416) - Fix class abstract error for Rviz viewer stack-of-tasks/pinocchio#2425 - Fix compilation issue with MSCV and C++17 stack-of-tasks/pinocchio#2437 - Fix `pinocchio-test-py-robot_wrapper` when building with SDF and collision support stack-of-tasks/pinocchio#2437 - Fix crash when calling `Inertia::FromDynamicParameters` in Python with wrong vector size stack-of-tasks/pinocchio#2296 - Fix `examples/cassie-simulation.py` and `examples/talos-simulation.py` stack-of-tasks/pinocchio#2443 - Fix build with CppAd 2024 stack-of-tasks/pinocchio#2459 - Fix `pinocchio-test-cpp-mjcf` unittest with Boost 1.86 stack-of-tasks/pinocchio#2459 - Fix `pinocchio-test-cpp-constraint-variants` uninitialized values stack-of-tasks/pinocchio#2459 - Fix mixing library symbols between Pinocchio scalar bindings stack-of-tasks/pinocchio#2459 - Fix bug for get{Joint,Frame}JacobianTimeVariation stack-of-tasks/pinocchio#2466 ### Changed - Modernize python code base with ruff stack-of-tasks/pinocchio#2418 - Does not create a root_joint frame from parsed models (urdf, mjcf and sdf) when no root joint is provided stack-of-tasks/pinocchio#2402
Pseudo Inertia and Log Cholesky Parametrization
This PR introduces the capability to use pseudo inertia and Log Cholesky parametrization within Pinocchio. These additions are based on recent advancements in ensuring the physical consistency of inertial parameters and provide a novel approach to parameterize the pseudo inertia matrix. The new functionalities include conversions to/from pseudo inertia, Log Cholesky parametrization, and related handy functions such as the Jacobian of Log Cholesky to dynamic parameters. Here you may find some short related notes on these parametrizations.
Key Contributions
Pseudo Inertia Parametrization:
Log Cholesky Parametrization:
This PR also partially close the problem of checking physical consistency mentioned in #2204