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

Resolve contacts_params attribute redundancy #296

Closed
flferretti opened this issue Nov 22, 2024 · 2 comments · Fixed by #299
Closed

Resolve contacts_params attribute redundancy #296

flferretti opened this issue Nov 22, 2024 · 2 comments · Fixed by #299
Assignees

Comments

@flferretti
Copy link
Collaborator

The contacts_params attribute in the JaxSimModelData object is redundant, as the same data is accessible directly through JaxSimModel.contact_model.parameters. This duplication creates unnecessary complexity and increases maintenance overhead. To improve code simplicity and clarity, I'd propose to remove contacts_params from JaxSimModelData and utilizing model.contact_model.parameters directly within the contact models.

Additionally, this change will eliminate the need to initialize both the model and the data for each integration step when computing the contact forces. As a result, the JaxSimModelData instance will be lighter and more efficient, improving the overall performance of the simulation.

The main disadvantage of this change is that the contact parameters for the SoftContacts model can no longer be initialized automatically. However, the contact parameters attribute should be updated after the JaxSimModel object initialization as follows:

import jaxsim.api as js

model = js.model.JaxSimModel.build_from_model_description(model_description=...)

with model.editable(validate=False) as model:

    model.contact_model = jaxsim.rbda.contacts.SoftContacts.build(
        terrain=model.terrain,
        parameters=js.contact.estimate_good_contact_parameters(
            model=model,
            number_of_active_collidable_points_steady_state=4,
            static_friction_coefficient=1.0,
            damping_ratio=1.0,
            max_penetration=0.001,
        ),
    )

Nevertheless, a different way to automatically initialize the SoftContactsParams can be investigated.

An initial version of the proposal can be found at ami-iit/jaxsim@remove_data_contacts_params

@flferretti flferretti self-assigned this Nov 22, 2024
@diegoferigo
Copy link
Member

In my opinion it would be better to keep the parameters in the JaxSimModelData object rather than the JaxSimModel object. Particularly for SoftContacts, including the terrain parameters in the domain randomization set can be extremely important for sim-to-real applications. If you move them to the model, a vectorized implementation cannot use a unique model operating on a batched data object having different contact parameters.

And this is also the reason why the static and non-static parameters have been split in #257.

@flferretti flferretti changed the title Remove redundant contacts_params attribute from JaxSimModelData Resolve contacts_params attribute redundancy Nov 25, 2024
@flferretti
Copy link
Collaborator Author

In my opinion it would be better to keep the parameters in the JaxSimModelData object rather than the JaxSimModel object. Particularly for SoftContacts, including the terrain parameters in the domain randomization set can be extremely important for sim-to-real applications. If you move them to the model, a vectorized implementation cannot use a unique model operating on a batched data object having different contact parameters.

And this is also the reason why the static and non-static parameters have been split in #257.

Thanks for your suggestions Diego! I opened #299 accordingly

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 a pull request may close this issue.

2 participants