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

Uniformize usage of integrators API #378

Merged
merged 2 commits into from
Feb 26, 2025
Merged

Uniformize usage of integrators API #378

merged 2 commits into from
Feb 26, 2025

Conversation

flferretti
Copy link
Collaborator

@flferretti flferretti commented Feb 25, 2025

This PR changes the API usage for integrators by ensuring consistent parameter names and removing redundant calculations. This improves code clarity and maintainability.


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

@flferretti flferretti force-pushed the fix/integrators_usage branch from 5ce5b58 to cd884ca Compare February 25, 2025 13:40
@flferretti flferretti self-assigned this Feb 25, 2025
@flferretti flferretti force-pushed the fix/integrators_usage branch from cd884ca to 911648f Compare February 25, 2025 13:41
@flferretti flferretti marked this pull request as ready for review February 25, 2025 13:41
@flferretti flferretti requested a review from xela-95 as a code owner February 25, 2025 13:41
Copy link
Member

@younik younik left a comment

Choose a reason for hiding this comment

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

I strongly agree with these changes, thank you!

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 @flferretti !! The changes introduced in the PR seems good to me, and also the math used in the semi-implicit to get the derivative of the origin of the base frame (but @CarlottaSartore if you have time double check it please).

Copy link
Contributor

@CarlottaSartore CarlottaSartore left a comment

Choose a reason for hiding this comment

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

LGTM but also in this case, berfore mergign I would:

  • Check if the robot walks in comodo with relaxed rigid (it happened in the past when changing integrator that the tests passed but the robot was not walking)
  • Check the time performance

@flferretti
Copy link
Collaborator Author

Thanks for your review @CarlottaSartore! Regarding:

* Check if the robot walks in comodo with relaxed rigid (it happened  in the past when changing integrator that the tests passed but the robot was not walking)

Only the interface has been changed. There are no significant changes to the integration method apart from using this formulation:

$${} ^{B[W]} v _{W, B} = \begin{bmatrix} {} ^W \dot{o} _B \\\ {} ^W \omega _B \end{bmatrix} = {} ^W v _{W, B} - \begin{bmatrix} S({} ^W \omega _B) {} ^W o _B \\ 0 \end{bmatrix}$$

Instead of explicitly converting to mixed representation. This approach has already been verified as it is the one used by the RK4 integrator.

* Check the time performance

I removed an extra call to the system dynamics for the RK4 as it wasn't used. Since the default contact model has not been changed. Do you think it is really worth at this stage or can we address this in future PRs?

@CarlottaSartore
Copy link
Contributor

Thanks for your review @CarlottaSartore! Regarding:

* Check if the robot walks in comodo with relaxed rigid (it happened  in the past when changing integrator that the tests passed but the robot was not walking)

Only the interface has been changed. There are no significant changes to the integration method apart from using this formulation:

𝐵
[
𝑊
]
𝑣
𝑊
,
𝐵

[
𝑊
𝑜
˙
𝐵
 
𝑊
𝜔
𝐵
]

𝑊
𝑣
𝑊
,
𝐵

[
𝑆
(
𝑊
𝜔
𝐵
)
𝑊
𝑜
𝐵
0
]
Instead of explicitly converting to mixed representation. This approach has already been verified as it is the one used by the RK4 integrator.

* Check the time performance

I removed an extra call to the system dynamics for the RK4 as it wasn't used. Since the default contact model has not been changed. Do you think it is really worth at this stage or can we address this in future PRs?

I would double-check with Comodo, to be sure that we have replicated it correctly, for what concerns the time analysis I think you can do one for both this and #360, (merging in a unique PR) the same olds for comodo check ! Up to you!

@flferretti
Copy link
Collaborator Author

The changes correctly run on CoMoDo. Merging

@flferretti flferretti merged commit f5e291f into main Feb 26, 2025
21 of 22 checks passed
@flferretti flferretti deleted the fix/integrators_usage branch February 26, 2025 14:06
@CarlottaSartore
Copy link
Contributor

The changes correctly run on CoMoDo. Merging

Cool, I assume then that the time performance check will be done in #360, it would have been better to do it in a unique PR so that we are more careful in merging in main but since we are doing #380 this kind of issues will be automatically correctly handled.

@flferretti
Copy link
Collaborator Author

The changes correctly run on CoMoDo. Merging

Cool, I assume then that the time performance check will be done in #360, it would have been better to do it in a unique PR so that we are more careful in merging in main but since we are doing #380 this kind of issues will be automatically correctly handled.

The performance checks have already been done. The results didn't present any significant difference

@CarlottaSartore
Copy link
Contributor

Cool on which machine ? It would be better to report here the performance output as done here https://github.com/ami-iit/component_darwin/issues/59#issuecomment-2639530952 for completeness

@flferretti
Copy link
Collaborator Author

Cool on which machine ?

Intel Ultra 7 155H. The tests were performed using comodo walking task as a reference.

It would be better to report here the performance output as done here https://github.com/ami-iit/component_darwin/issues/59#issuecomment-2639530952 for completeness

You're right, I'll keep working on #380, so we don't have to do this manually

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.

4 participants