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

Simplify integrators module structure #82

Merged
merged 11 commits into from
Feb 12, 2024
Merged

Simplify integrators module structure #82

merged 11 commits into from
Feb 12, 2024

Conversation

flferretti
Copy link
Collaborator

@flferretti flferretti commented Feb 8, 2024

This PR has the aim of simplifying the integrators module. This will ease up the process of adding new types of integrators and facilitate the debugging.

In order to do this, I made the following changes:

  • the class IntegratorType has been moved from ode_integration to integrators. This has no effect on the rest of the code as the integrators module is imported before ode_integration
  • the odeint_INTEGRATORTYPE functions have been concentrated in one
  • the odeint_INTEGRATORTYPE_one_step functions have been merged in one oneint_one_step that changes the body function only. For the manifold integrator, the additional operation have been moved inside a conditional

Since @diegoferigo is working on the variable step integrators, we could think to merge this PR after rebasing on the #72


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

@flferretti flferretti self-assigned this Feb 8, 2024
@flferretti flferretti marked this pull request as ready for review February 12, 2024 08:48
src/jaxsim/simulation/integrators.py Outdated Show resolved Hide resolved
src/jaxsim/simulation/integrators.py Outdated Show resolved Hide resolved
src/jaxsim/simulation/integrators.py Outdated Show resolved Hide resolved
src/jaxsim/simulation/integrators.py Outdated Show resolved Hide resolved
@diegoferigo
Copy link
Member

diegoferigo commented Feb 12, 2024

I was doing the review in the online VSCode and it got sent before actually finishing it :)

It looks good even though there's some issue with typing. In fact, the Euler/RK integrators are generic and can work on any pytree. Instead, the semi-implicit variants and those that integrate the quaternion on SO(3) expect and return an ODEState.

I originally developed these integrators for a possible new external library, but it always made sense to keep them inside the project. Eventually, the schemes that work better are those with the base orientation on the manifold, and they are pretty custom for floating-base robots. I have a better implementation of schemes based on Butcher tableau in #72, if we feel like, we can use those resources also for the vanilla fixed-step integrators (forward euler, RK4).

Copy link
Member

@diegoferigo diegoferigo left a comment

Choose a reason for hiding this comment

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

Looks good! Some remaining minor comments.

src/jaxsim/simulation/integrators.py Outdated Show resolved Hide resolved
src/jaxsim/simulation/integrators.py Outdated Show resolved Hide resolved
src/jaxsim/simulation/integrators.py Outdated Show resolved Hide resolved
src/jaxsim/simulation/integrators.py Outdated Show resolved Hide resolved
src/jaxsim/simulation/integrators.py Outdated Show resolved Hide resolved
src/jaxsim/simulation/integrators.py Outdated Show resolved Hide resolved
src/jaxsim/simulation/integrators.py Outdated Show resolved Hide resolved
@flferretti flferretti merged commit 0ea14af into main Feb 12, 2024
19 checks passed
@flferretti flferretti deleted the flferretti-patch-1 branch February 12, 2024 11:45
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.

None yet

2 participants