-
Notifications
You must be signed in to change notification settings - Fork 26
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
Minor additions to gym_ignition.rbd package #283
Minor additions to gym_ignition.rbd package #283
Conversation
python/gym_ignition/rbd/utils.py
Outdated
import numpy as np | ||
|
||
|
||
def hat(vector3: np.ndarray) -> np.ndarray: |
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.
Not a big problem, but the symbol that in latex is typically used for representing this operator is the wedge, not the hat.
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.
Yeah, makes sense to match latex terminology here 👍
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 start having second thoughts on this, using hat instead of wedge would give the code a more colloquial sense since standing to the notation document the operator is pronounced hat while in latex is typesetted as wedge.
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.
the operator is pronounced hat while in latex is typesetted as wedge.
I guess there is no absolute reference, but for me
python/gym_ignition/rbd/utils.py
Outdated
skew_symmetric[1, 0]]) | ||
|
||
|
||
def skew(matrix: np.ndarray) -> np.ndarray: |
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 am afraid that this may be confusing to user, as they could expect that this function takes a 3\times 1 vector and return the 3x3 skew symmetric matrices, what now is done by the hat operator.
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.
Do you have any better name for the decomposition of a matrix in its skew symmetric and symmetric parts?
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.
extract_skew
and extract_symmetric
or skew_part
and symmetric_part
seems to be more clear to me, especially if no docs are provided.
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.
Minor comments.
bbedf68
to
aadee25
Compare
@traversaro addressed naming suggestions and added documentation |
aadee25
to
cc04bd4
Compare
- get_average_velocity_jacobian - get_generalized_gravity_forces
cc04bd4
to
0929564
Compare
utils
module with usefulextract_skew
,extract_symm
,vee
andwedge
methodsKinDynComputations.get_generalized_gravity_forces
KinDynComputations.get_average_velocity_jacobian