Skip to content
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

Add frame example #300

Merged
merged 7 commits into from
Nov 28, 2024
Merged

Conversation

CarlottaSartore
Copy link
Contributor

@CarlottaSartore CarlottaSartore commented Nov 26, 2024

This PR adds the example for computing velocity and transformation of the frames. In addition, it introduces minor changes to align the frames API with the link's one


📚 Documentation preview 📚: https://jaxsim--300.org.readthedocs.build//300/

Copy link
Collaborator

@flferretti flferretti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @CarlottaSartore! LGTM. I just added a minor comment

examples/jaxsim_as_multibody_dynamics_library.ipynb Outdated Show resolved Hide resolved
Copy link
Member

@xela-95 xela-95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CarlottaSartore for your contribution! I just left some minor comments.

Other than this, would you add a little unit test on the jaxsim.api.frame.velocity function to avoid regressions in future? It can be added to https://github.com/ami-iit/jaxsim/blob/main/tests/test_api_frame.py

src/jaxsim/api/frame.py Outdated Show resolved Hide resolved
src/jaxsim/api/frame.py Outdated Show resolved Hide resolved
@CarlottaSartore
Copy link
Contributor Author

Thanks @CarlottaSartore for your contribution! I just left some minor comments.

Other than this, would you add a little unit test on the jaxsim.api.frame.velocity function to avoid regressions in future? It can be added to https://github.com/ami-iit/jaxsim/blob/main/tests/test_api_frame.py

Thanks @xela-95, I have implemented the CI in 5fdbe5b

Let me know if there ar any additional changes needed !

@CarlottaSartore
Copy link
Contributor Author

The tests are not passing, checking why

@CarlottaSartore
Copy link
Contributor Author

I shoudl have fix with commit 436fde6

The error was that Jaxsim creates a single vector organise as : $[l_0, ..., l_{nl}, f_0,..., f_{nf}]$
What happens is that the idx returned by frame name_to_idx is the idx inside such vector and not the index of the vector containing only frames.
Thus such index should be $>nl$ and $<nl+nf$

@xela-95
Copy link
Member

xela-95 commented Nov 27, 2024

The error was that Jaxsim creates a single vector organise as : [ l 0 , . . . , l n l , f 0 , . . . , f n f ] What happens is that the idx returned by frame name_to_idx is the idx inside such vector and not the index of the vector containing only frames. Thus such index should be > n l and < n l + n f

Right nice catch!

Copy link
Collaborator

@flferretti flferretti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typo

src/jaxsim/api/frame.py Outdated Show resolved Hide resolved
Copy link
Member

@xela-95 xela-95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's good for me! :)

Co-authored-by: Filippo Luca Ferretti <[email protected]>
@CarlottaSartore
Copy link
Contributor Author

Thanks @xela-95, and @flferretti I should have also implemented your review, is it right ? can I proceed with the merge ?

Copy link
Collaborator

@flferretti flferretti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CarlottaSartore, LGTM

@flferretti flferretti merged commit 524ad42 into ami-iit:main Nov 28, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants