MJCF importer: correctly rotate inertia tensor#449
Conversation
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
📝 Walkthrough""" WalkthroughA new test was added to verify correct rotation of inertia tensors when importing MJCF files, using both diagonal and full inertia cases. The MJCF parsing utility was updated to accept either a filename or raw XML string and to correctly apply rotation to inertia tensors using a similarity transform with the rotation matrix derived from the quaternion. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as test_inertia_rotation
participant Importer as parse_mjcf
participant Builder as ModelBuilder
Test->>Importer: Call parse_mjcf with MJCF XML string
Importer->>Builder: Parse model and apply rotation to inertia tensor
Importer-->>Test: Return parsed model with rotated inertia
Test->>Test: Compare model inertia to expected rotated inertia
Estimated code review effort2 (10–30 minutes) 📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/tests/test_import_mjcf.py(1 hunks)newton/utils/import_mjcf.py(4 hunks)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (8)
newton/utils/import_mjcf.py (3)
34-34: Function signature appropriately updated for dual input support.The parameter name change from
mjcf_filenametomjcf_filename_or_stringclearly indicates the expanded functionality to accept both file paths and XML strings.
62-62: Documentation updated to reflect new parameter functionality.The parameter documentation correctly describes that the function now accepts either a filename or MJCF XML string content.
714-717: Inertia tensor rotation correctly implemented using similarity transform.The corrected implementation properly applies the similarity transform R * I * R^T for rotating the inertia tensor, which is mathematically correct. The previous implementation (mentioned in the summary) only applied left multiplication, which would not preserve the tensor properties.
The implementation:
- Converts quaternion to 3x3 rotation matrix using
wp.quat_to_matrix()- Converts to NumPy for matrix operations
- Applies the similarity transform:
rot_np @ I_m @ rot_np.T- Converts back to the internal matrix type
This ensures the inertia tensor is properly rotated to the new coordinate frame while maintaining its symmetric positive-definite properties.
newton/tests/test_import_mjcf.py (5)
120-133: Well-structured test setup for diagonal inertia case.The test uses a clear MJCF snippet with a 90-degree Z-axis rotation quaternion and diagonal inertia values. The XML is properly formatted and the test case is well-documented.
135-145: Comprehensive test coverage with full inertia matrix.The second test case covers the full inertia matrix scenario, ensuring both diagonal and off-diagonal elements are properly rotated. This provides good coverage of the inertia rotation functionality.
147-157: Correct analytical computation for diagonal inertia rotation.The expected result for the diagonal inertia case is mathematically correct. A 90-degree Z-axis rotation transforms the diagonal inertia [1, 2, 3] to [2, 1, 3], swapping the x and y components while keeping the z component unchanged.
159-194: Mathematically sound full inertia rotation test.The test correctly:
- Constructs the original 3x3 inertia matrix from the
fullinertiavalues- Applies the analytical rotation matrix for 90-degree Z-axis rotation
- Computes the expected result using the similarity transform R @ I @ R^T
- Verifies the result with appropriate numerical tolerance
The rotation matrix
[[0, -1, 0], [1, 0, 0], [0, 0, 1]]correctly represents a 90-degree counterclockwise rotation around the Z-axis.
195-196: Important verification that rotation is actually applied.This assertion ensures the test validates that the inertia tensor is actually modified by the rotation, preventing false positives where the rotation might be incorrectly implemented as an identity transform.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/tests/test_import_mjcf.py (1)
120-197: Excellent comprehensive test for inertia tensor rotation!This test thoroughly validates the inertia tensor rotation implementation with both diagonal and full inertia cases. The mathematical approach is sound:
- Diagonal case: Correctly expects [1,2,3] → [2,1,3] for 90° Z-rotation
- Full inertia case: Properly applies similarity transform R @ I @ R.T
- Verification: Ensures rotation was actually applied (not identity)
The test effectively uses the new XML string parsing functionality and validates the core PR objective.
Minor cleanup suggestion:
- # For full inertia, calculate the expected result analytically using the same quaternion - # Original inertia matrix: - # [1.0 0.1 0.2] - # [0.1 2.0 0.3] - # [0.2 0.3 3.0] - - # The quaternion (0.7071068, 0, 0, 0.7071068) in MuJoCo WXYZ format represents a 90-degree rotation around Z-axis - # Calculate the expected result analytically using the correct rotation matrix - # For a 90-degree Z-axis rotation: R = [0 -1 0; 1 0 0; 0 0 1] - original_inertia = np.array([[1.0, 0.1, 0.2], [0.1, 2.0, 0.3], [0.2, 0.3, 3.0]])
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/tests/test_import_mjcf.py(1 hunks)newton/utils/import_mjcf.py(4 hunks)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (4)
newton/utils/import_mjcf.py (4)
33-34: LGTM! Enhanced function signature for better flexibility.The parameter rename clearly indicates the function now accepts both file paths and XML string content, improving API flexibility and testability as outlined in the PR objectives.
62-62: LGTM! Clear documentation update.The documentation accurately reflects the dual functionality of accepting either a filename or XML string content.
91-100: LGTM! Clean implementation of XML string vs file detection.The logic correctly differentiates between XML content and file paths. The fallback to "." for
mjcf_dirnamewhen parsing XML strings is appropriate since there's no file system context.
713-717: Excellent! Correct implementation of inertia tensor rotation.This properly implements the similarity transform
R @ I @ R.Tto rotate the inertia tensor, which is mathematically correct. The conversion between Warp and NumPy formats is handled cleanly, and this addresses the core issue mentioned in the PR title.
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
70c229b to
ef2d67c
Compare
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
…on-physics#449) # Description <!-- Thank you for your interest in sending a pull request. Please make sure to check the contribution guidelines. Link: https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html --> 1. Adds an optional CLI argument to the robomimic training script that can be used to set the number of training epochs. If set, the epochs defined by the JSON training config is overwritten. 2. Save the last training epoch regardless if it does not satisfy the training interval defined in the JSON training config. This ensures that a model will always be saved even if the user specifies an arbitrary epoch number that does not divide evenly by the save interval defined in the JSON. ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - New feature (non-breaking change which adds functionality) ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [ ] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task -->
Signed-off-by: Alain Denzler <adenzler@nvidia.com>
based on #442
Adds:
Summary by CodeRabbit
New Features
Bug Fixes
Chores