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

Refactor integrators module and redefine JaxSimModel class attributes #252

Merged
merged 23 commits into from
Oct 30, 2024

Conversation

flferretti
Copy link
Collaborator

@flferretti flferretti commented Oct 3, 2024

This pull request includes several changes to the integrators logic and API. The JaxSimModel class now incorporates integrator and dt as static attributes. In order to do this, it was necessary to refactor the jaxsim.integrators module in order to simplify the logic by removing the init method.

The integrators logic is simplified by avoiding the use of integrator_state to pass around variables and making wrap_system_dynamics_for_integration accept only the function describing the dynamics of the system, since model and data were required arguments for the integration anyway.


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

@flferretti flferretti self-assigned this Oct 3, 2024
@flferretti flferretti force-pushed the feature/static_integrator_dt branch 3 times, most recently from 99878f7 to 3a1e965 Compare October 4, 2024 10:23
@flferretti flferretti marked this pull request as ready for review October 4, 2024 13:58
@flferretti flferretti force-pushed the feature/static_integrator_dt branch from 3a1e965 to 81f6136 Compare October 4, 2024 13:58
@flferretti flferretti force-pushed the feature/static_integrator_dt branch 7 times, most recently from 11527fa to 104de71 Compare October 18, 2024 14:18
@flferretti
Copy link
Collaborator Author

@diegoferigo I modified the logic to be backward compatible, so it should still be compatible with parametric models

@flferretti flferretti force-pushed the feature/static_integrator_dt branch from d346b58 to 0d8d807 Compare October 25, 2024 10:29
@diegoferigo diegoferigo force-pushed the feature/static_integrator_dt branch from eabd74a to 81807a1 Compare October 29, 2024 09:58
@diegoferigo diegoferigo self-assigned this Oct 29, 2024
Copy link
Collaborator Author

@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.

Early comments

src/jaxsim/api/model.py Outdated Show resolved Hide resolved
src/jaxsim/api/model.py Outdated Show resolved Hide resolved
@diegoferigo diegoferigo force-pushed the feature/static_integrator_dt branch from 81807a1 to 7234614 Compare October 29, 2024 10:52
@diegoferigo diegoferigo force-pushed the feature/static_integrator_dt branch from a597f85 to 7ceca8a Compare October 29, 2024 14:30
@flferretti flferretti force-pushed the feature/static_integrator_dt branch from efec2ea to 1f7eec4 Compare October 30, 2024 15:21
Comment on lines +57 to +60
integrator: Static[jaxsim.integrators.Integrator | None] = dataclasses.field(
default=None, repr=False
)

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep an eye open on the hash of this attribute in the future if we face the usual problems on static elements.

Copy link
Collaborator Author

@flferretti flferretti Oct 30, 2024

Choose a reason for hiding this comment

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

Thanks Diego! Leaving unresolved

@flferretti flferretti merged commit 78c55cc into main Oct 30, 2024
24 checks passed
@flferretti flferretti deleted the feature/static_integrator_dt branch October 30, 2024 15:49
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.

2 participants