-
Notifications
You must be signed in to change notification settings - Fork 14
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
[Sprint] Improve caching mechanism #359
Conversation
7846b93
to
a8c23cc
Compare
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.
Thanks a lot @younik, it looks to me overall. It does make sense to remove the reset_*
methods, but the problem is that we are storing some quantities in VelRepr.Inertial
, so we either need to add a comment to make this clear to the user, or to change the cached quantities in the same velocity representation of the JaxSimModelData
instance
@@ -549,6 +446,64 @@ def reset_base_velocity( | |||
base_linear_velocity=W_v_WB[0:3].squeeze().astype(float), | |||
base_angular_velocity=W_v_WB[3:6].squeeze().astype(float), | |||
) | |||
|
|||
def replace( |
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.
Adding typing.override
decorator would prevent in the future errors related to overriding methods from parent class
def replace( | |
@override | |
def replace( |
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.
done!
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.
Thanks @younik ! The replace approach in which to update the cached values is elegant to me and avoid potential future issues! I added some comments related to the handling of data attributes and their representation.
src/jaxsim/api/link.py
Outdated
@@ -295,7 +295,8 @@ def jacobian( | |||
B_J_WL_I = B_J_WL_B | |||
|
|||
case VelRepr.Mixed: | |||
W_R_B = data.base_orientation(dcm=True) | |||
W_R_B = data.base_orientation |
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.
now data.base_orientation return a quaternion
W_R_B = data.base_orientation | |
W_Q_B = data.base_orientation |
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.
done!
src/jaxsim/api/data.py
Outdated
base_linear_velocity=base_linear_velocity, | ||
base_angular_velocity=base_angular_velocity, |
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.
forward kinematics works only in inertial (this has been made more clear in #361 , so we need to convert base linear and angular velocities to this representation before calling it.
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.
good catch, done!
7d8cc14
to
3a0dc6c
Compare
@younik I noted that here we also miss a property for the base transform, since it is only cached as a private attribute. |
This PR goes a bit beyond what we agreed, so feel free to reject it. I can easily do a simpler version where we call
update_cached
inside the reset function.The motivation is that update cached was always called after
data.replace
(and the cached values are needed at every step), so it probably makes sense to compute it during data construction and at every replace.The
reset_
methods are calling a replace, which is an expensive operation. This is not immediately clear to the user, and leads to sequentially reset calls:One method that allows to change multiple things would be better, so I used the replace itself.
The other selling point is that with the current solution, we need to remember to call
update_cached
from now on, which may lead to future bugs.One negative aspect of this is that we need
model
inreplace
, which differs from thedataclass.replace
as the model is not an attribute of data.📚 Documentation preview 📚: https://jaxsim--359.org.readthedocs.build//359/