Closed Giulero closed 1 year ago
Concerning lines https://github.com/ami-iit/ADAM/pull/37/files#diff-f0645ad6bebf4dca2b69129d586b130db3c0e78532fffe9b30c09217d0b0740cR6-R32, it is not fully clear why ArrayLike
declares the abstract methods zeros
and eye
.
ArrayLike
is supposed to represent the underlying type of the calculations, while those methods are basically constructing the objects of a certain ArrayLike
type.
I would change the structure into something like:
class ArrayLike(abc.ABC):
"""Abstract class for a generic Array wrapper. Every method should be implemented for every data type."""
"""This class has to implemented the following operators: <list of operators>"""
class ArrayLikeFactory(abc.ABC):
"""Class implementing factory methods to construct object of a certain data type"""
@abc.abstractmethod
def zeros() -> npt.ArrayLike:
"""
Args:
x (npt.ArrayLike): matrix dimension
Returns:
npt.ArrayLike: zero matrix of dimension x
"""
pass
@abc.abstractmethod
def eye(x: npt.ArrayLike) -> npt.ArrayLike:
"""
Args:
x (npt.ArrayLike): matrix dimension
Returns:
npt.ArrayLike: identity matrix of dimension x
"""
pass
@abc.abstractmethod
def spatialMath() -> SpatialMath:
"""
Returns:
SpatialMath implementation that allows for computation on the underlying type provided by this class
"""
pass
class SpatialMath(abc.ABC):
"""Class implementing the main geometric functions used for computing rigid-body algorithm"""
.
.
.
In this way each object has a clearly specified responsibility which makes it easier to maintain:
ArrayLike
: the underlying type providing the common arithmetic operators (*
, +
,...)SpatialMath
: allows for common operation on the underlying type ArrayLikeFactory
: provides for the construction of object of the underlying type and for the related SpatialMath
to work with themConcerning https://github.com/ami-iit/ADAM/pull/37#issuecomment-1419434913, it would be great to add some abstract methods for ArrayLike
so that if someone wants to implement or use their custom type, they would know exactly which operations are needed. Unfortunately, there is no way to make operators "abstract" (at least, not that I know of).
A workaround might be something like:
@dataclass
class ArrayLike:
@abc.abstractmethod
def mymul(self, other):
pass
def __matmul__(self, other: ArrayLike) -> ArrayLike:
"""Overrides @ operator"""
self.mymul(other)
What do you think about this? @Giulero
Hi @RiccardoGrieco! It seems it is possible "to abstract" operation methods (see https://github.com/ami-iit/ADAM/pull/37/commits/311fb5ddc7012832cb10c14702ac69ec724871dc) I created a separate factory class for the array :)
@RiccardoGrieco I addressed the suggestions (at least tried :P)!
Tests are failing since I'm using icub-models
which is not present for now in the conda recipe. Added in https://github.com/conda-forge/adam-robotics-feedstock/pull/1
Thanks for all the suggestions @RiccardoGrieco! It has been inspiring :) Merging!
\Evviva !
Even if the branch is named
small refactor
it 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_py
elements). 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
Tree
class.