Refactor #37
Refactor #37
Conversation
Co-authored-by: Riccardo Grieco <riccardo.grieco@hotmail.com>
…ance from abstract class.
|
@RiccardoGrieco I addressed the suggestions (at least tried :P)! |
|
Tests are failing since I'm using |
src/adam/model/model.py
Outdated
| factory: ModelFactory | ||
|
|
||
| def __post_init__(self): | ||
| """set the "lenght of the model as the number of links""" | ||
| self.N = len(self.links) | ||
|
|
||
| @staticmethod | ||
| def build(factory: ModelFactory, joints_name_list: List[str]) -> "Model": | ||
| """generates the model starting from the list of joints and the links-joints factory | ||
|
|
||
| Args: | ||
| factory (ModelFactory): the factory that generates the links and the joints, starting from a description (eg. urdf) | ||
| joints_name_list (List[str]): the list of the actuated joints | ||
|
|
||
| Returns: | ||
| Model: the model describing the robot | ||
| """ | ||
|
|
||
| joints = factory.get_joints() | ||
| links = factory.get_links() | ||
| frames = factory.get_frames() | ||
|
|
||
| # set idx to the actuated joints | ||
| for [idx, joint_str] in enumerate(joints_name_list): | ||
| for joint in joints: | ||
| if joint.name == joint_str: | ||
| joint.idx = idx | ||
|
|
||
| tree = Tree.build_tree(links=links, joints=joints) | ||
|
|
||
| # generate some useful dict | ||
| joints: Dict(str, Joint) = {joint.name: joint for joint in joints} | ||
| links: Dict(str, Link) = {link.name: link for link in links} | ||
| frames: Dict(str, Link) = {frame.name: frame for frame in frames} | ||
|
|
||
| return Model( | ||
| name=factory.name, | ||
| links=links, | ||
| frames=frames, | ||
| joints=joints, | ||
| tree=tree, | ||
| NDoF=len(joints_name_list), | ||
| factory=factory, |
There was a problem hiding this comment.
Why does model keep the factory object? That object seems to be needed only by the "build"
There was a problem hiding this comment.
Since I'm using that math in the RBD class! The math used in the model and the math used in the RBD class must be the same.
There was a problem hiding this comment.
If you want to stick with this solution, I think it would be better to store the only the math object instead of the factory one.
In general however, I would avoid keeping objects as attributes just to pass them to other ones. I think it should be "user" responsibility to initialize the objects with consistently, i.e. providing the RBD object with the same math as the one used for the model factory.
src/adam/casadi/computations.py
Outdated
| factory = URDFModelFactory(urdfstring, SpatialMath()) | ||
| model = Model.build(factory=factory, joints_name_list=joints_name_list) | ||
| self.rbdalgos = RBDAlgorithms(model=model, math=SpatialMath()) |
There was a problem hiding this comment.
I think it's better to create only one SpatialMath (same for the other implementations):
| factory = URDFModelFactory(urdfstring, SpatialMath()) | |
| model = Model.build(factory=factory, joints_name_list=joints_name_list) | |
| self.rbdalgos = RBDAlgorithms(model=model, math=SpatialMath()) | |
| math = SpatialMath() | |
| factory = URDFModelFactory(urdfstring, math) | |
| model = Model.build(factory=factory, joints_name_list=joints_name_list) | |
| self.rbdalgos = RBDAlgorithms(model=model, math=math) |
There was a problem hiding this comment.
You're right! Done in 78bf57c
|
Thanks for all the suggestions @RiccardoGrieco! It has been inspiring :) |
|
\Evviva ! |
Even if the branch is named
small refactorit turned out to be a big one :D@RiccardoGrieco made me reason a bit, and I made several changes.
The building of the tree is now more generic and leverages factory classes.
The standard implementation uses
urdf_parser_py(calledURDFModelFactory), but we could implement an additional class that uses another parser.KinDyn classes don't inherit from RBDAlgo but compose objects, eg:
the Factory takes as input also to the Math, which should be implemented based on the desired math data type (we have casadi, jax, numpy and torch implementations). Using the math implementation, we can create Joints and Links (their standard implementation takes
urdf_parser_pyelements). This factory return joints, links, and frames lists, which are used to create theModel, which in turn is used in the RBDAlgorithms.Potentially this restructuring allows the creation of additional modules with different math and different implementations of robot elements.
Still missing: even if the root link is an input of the KinDyn class, the root of the tree is still the link with no parents. We could change this in the future and it would require just some modifications just in the
Treeclass.