-
Notifications
You must be signed in to change notification settings - Fork 26
Add MujocoModelFactory #145
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
Conversation
…DF and Mujoco model instantiation
… future renaming to `model`
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.
Pull request overview
This pull request adds comprehensive support for loading robot models directly from MuJoCo MjModel objects, enabling seamless integration between MuJoCo simulations and the adam library's kinematics/dynamics computations.
Key changes:
- Introduced
MujocoModelFactoryto parse MuJoCo models and convert them to adam's internal representation - Added a unified
build_model_factoryfunction that automatically selects the appropriate factory (URDF or MuJoCo) based on input type - Extended all computation classes across backends (NumPy, JAX, CasADi, PyTorch) with
KinDynFactoryMixinto support both URDF and MuJoCo model initialization
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 24 comments.
Show a summary per file
| File | Description |
|---|---|
| src/adam/model/mj_factory/mujoco_model.py | Implements the MuJoCo model factory that parses MjModel objects and extracts links, joints, and inertial properties |
| src/adam/model/kindyn_mixin.py | Provides factory methods (from_urdf, from_mujoco_model) to instantiate computation classes from different model sources |
| src/adam/model/factory.py | Implements the unified factory selection logic based on input type detection |
| src/adam/model/init.py | Exports the new MuJoCo factory and factory builder function |
| src/adam/numpy/computations.py | Updates NumPy computation class to use unified factory and inherit from mixin |
| src/adam/jax/computations.py | Updates JAX computation class to use unified factory and inherit from mixin |
| src/adam/casadi/computations.py | Updates CasADi computation class to use unified factory and inherit from mixin |
| src/adam/pytorch/computations.py | Updates PyTorch computation class to use unified factory and inherit from mixin |
| src/adam/pytorch/computation_batch.py | Updates PyTorch batch computation class to use unified factory and inherit from mixin |
| src/adam/model/std_factories/std_model.py | Fixes imports to use explicit paths instead of relative imports |
| tests/test_mujoco.py | Comprehensive test suite validating MuJoCo integration across kinematics and dynamics computations |
| README.md | Documents the new MuJoCo interface with usage examples and important notes about velocity representations |
| pyproject.toml | Adds mujoco and robot_descriptions as test dependencies |
| ci_env.yml | Adds mujoco and robot_descriptions to CI environment for Linux |
| ci_env_win.yml | Adds mujoco and robot_descriptions to CI environment for Windows |
Comments suppressed due to low confidence (3)
tests/test_mujoco.py:1
- Import of 'os' is not used.
import os
tests/test_mujoco.py:2
- Import of 'tempfile' is not used.
import tempfile
tests/test_mujoco.py:3
- Import of 'Path' is not used.
from pathlib import Path
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
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.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
xela-95
left a comment
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.
I took a look at the tests and they seem good to me! :)
src/adam/model/factory.py
Outdated
| def _is_mujoco_model(obj: Any) -> bool: | ||
| try: | ||
| from mujoco import MjModel | ||
| except ImportError: | ||
| return False | ||
| return isinstance(obj, MjModel) | ||
|
|
||
|
|
||
| def build_model_factory(description: Any, math) -> ModelFactory: | ||
| """Return a ModelFactory from a URDF string/path or a MuJoCo model.""" | ||
|
|
||
| if _is_mujoco_model(description): | ||
| from adam.model.mj_factory.mujoco_model import MujocoModelFactory | ||
|
|
||
| return MujocoModelFactory(mj_model=description, math=math) | ||
|
|
||
| if isinstance(description, (str, pathlib.Path)): | ||
| return URDFModelFactory(path=description, math=math) | ||
|
|
||
| raise ValueError( | ||
| "Unsupported model description. Pass a URDF path/string or a MuJoCo MjModel." |
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.
Non-blocking, but I think this is confusing. If a mujoco model is passed and mujoco is not available, this will print that the model is unsupported, but that is not true, it is mujoco that it is missing. However, we can't just raise an error if mujoco is missing, as perhaps the model is a URDF one. Perhaps we can do a custom logic to check if a file/string is a mujoco model (check if a root mujoco element exists? probably an llm may have a good idea on this) instead of just trying to pass it to the parser?
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.
True!
What about something like?
def _is_mujoco_model(obj: Any) -> bool:
"""Check if obj is a MuJoCo MjModel without importing mujoco."""
cls = obj.__class__
cls_name = getattr(cls, "__name__", "")
cls_module = getattr(cls, "__module__", "")
if cls_name != "MjModel" or "mujoco" not in cls_module:
return False
return all(hasattr(obj, attr) for attr in ("nq", "nv", "nu", "nbody"))
def _is_urdf(obj: Any) -> bool:
"""Check if obj is a URDF."""
if isinstance(obj, pathlib.Path):
return obj.suffix.lower() == ".urdf"
if isinstance(obj, str):
s = obj.lstrip()
if s.startswith("<") and "<robot" in s[:2048].lower():
return True
try:
return pathlib.Path(obj).suffix.lower() == ".urdf"
except Exception:
return False
return False
def build_model_factory(description: Any, math) -> ModelFactory:
"""Return a ModelFactory from a URDF string/path or a MuJoCo model."""
if _is_mujoco_model(description):
from adam.model.mj_factory.mujoco_model import MujocoModelFactory
return MujocoModelFactory(mj_model=description, math=math)
if _is_urdf(description):
return URDFModelFactory(path=description, math=math)
raise ValueError(
f"Unsupported model description. Expected a URDF path/string or a mujoco.MjModel. "
f"Got: {type(description)!r}"
)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.
Hi @traversaro ! A friendly ping about this comment!
|
Not directly related, but I think we could use this in https://github.com/ami-iit/mujoco-urdf-loader (or whatever we actually keep the code to convert urdf to mjcf, the fact that the repo did not receive any commit in the last year suggest me that is not the place : ) ) to validate that the conversion is "consistent" according to our conversions (that probably adam captures well). |
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.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This pull request adds support for loading robot models directly from MuJoCo
MjModelobjects.MuJoCo interface and model loading:
MujocoModelFactory(src/adam/model/mj_factory/mujoco_model.py) that parses MuJoCo models and exposes them in the library's standard format.build_model_factoryfunction to select the appropriate model factory (URDF or MuJoCo) based on input, and updated imports to use this function. [1] [2] [3]KinDynFactoryMixin(src/adam/model/kindyn_mixin.py) to provide helper methods for instantiating kinematics/dynamics computation classes from URDF or MuJoCo models.Updates to computation classes:
KinDynComputationsclasses in NumPy, CasADi, and JAX backends to inherit fromKinDynFactoryMixinand use the new unified model factory interface, allowing them to accept MuJoCo models as input. [1] [2] [3] [4] [5] [6]Documentation and dependency updates:
README.mdto document the new MuJoCo interface, including usage examples and important notes/warnings about velocity representations. [1] [2] [3]mujocoandrobot_descriptionsto dependencies in environment and test configuration files (ci_env.yml,ci_env_win.yml,pyproject.toml). [1] [2] [3]📚 Documentation preview 📚: https://adam-docs--145.org.readthedocs.build/en/145/