Add `ContactModel` base class and abstract contact handling in `JaxSimModel` and `JaxSimModelData` #178

flferretti · 2024-06-13T14:59:19Z

This pull request refactors the contact models to enhance flexibility and future extensibility. Key changes include:

Introduced a ContactModel base class together with ContactsState and ContactParams
Refactored SoftContactModel, SoftContactParams and SoftContactState to inherit from ContactModel for consistency.
Added contact_model attribute to JaxSimModel.

These changes allow specifying different contact models during instantiation, facilitating the development of additional contact models in the future.

C.C. @xela-95

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

flferretti · 2024-06-14T13:22:30Z

For some reason, CI is failing only in Python 3.10 (https://github.com/ami-iit/jaxsim/actions/runs/9511311149)

diegoferigo · 2024-06-14T14:26:21Z

For some reason, CI is failing only in Python 3.10 (https://github.com/ami-iit/jaxsim/actions/runs/9511311149)

Mmh, since it is related to hash and equality, maybe we can wait to merge #179.

diegoferigo

Thanks @flferretti for this first attempt aimed at abstracting the contact model in view of the addition of alternative ones. I left few initial comments in the code.

What if we leave the actual soft-contact class defining the algorithm in jaxsim.rbda? If we plan to add multiple contact models, I'm ok in adding a new subpackage jaxsim.rbda.contacts.{soft|...}.
I'd keep the high-level detection of the contact model much more simple, right now there are many hacks I'd like to avoid at this initial stage.

src/jaxsim/api/contact.py

src/jaxsim/api/ode_data.py

src/jaxsim/api/contact.py

flferretti · 2024-06-17T13:24:50Z

What if we leave the actual soft-contact class defining the algorithm in jaxsim.rbda? If we plan to add multiple contact models, I'm ok in adding a new subpackage jaxsim.rbda.contacts.{soft|...}.

Done in 2999198

diegoferigo

Great, thanks @flferretti! Last review pass with the final suggestions. Well done 🚀

src/jaxsim/api/contact.py

src/jaxsim/rbda/contacts/common.py

diegoferigo · 2024-06-18T07:56:27Z

src/jaxsim/rbda/contacts/common.py

+        position: jtp.Vector,
+        velocity: jtp.Vector,
+        **kwargs,
+    ) -> tuple[Any, ...]:


I suggest to provide as first output the computed contact forces, and as second output a tuple of auxiliary data. For the current soft contacts model, it will be (ṁ,)

Suggested change

) -> tuple[Any, ...]:

) -> tuple[jtp.Matrix, tuple[Any, ...]]:

Done in f64764f

Note: this has to be propagated to the implementation before resolving this conversation.

Done in a9e3b2f

src/jaxsim/rbda/contacts/common.py

diegoferigo

Final suggestions that I overlooked in my previous review:

Can you double check in our codebase where JaxSimModel.build is called? For sure, the new JaxSimModel.contact_model attribute has to be copied when the model is reduced here.
Instead of pass, you can also use ... but I would not leave the body empty.

src/jaxsim/api/model.py

src/jaxsim/rbda/contacts/common.py

flferretti · 2024-06-18T14:43:25Z

Can you double check in our codebase where JaxSimModel.build is called? For sure, the new JaxSimModel.contact_model attribute has to be copied when the model is reduced here.

The only part in which the contact_model is not specified is in the test, yet this is not a problem since the soft contact model is the default value

flferretti · 2024-06-18T14:46:13Z

I added the pass statements and the rest of the suggestions in ab7a688

diegoferigo · 2024-06-18T14:48:32Z

Can you double check in our codebase where JaxSimModel.build is called? For sure, the new JaxSimModel.contact_model attribute has to be copied when the model is reduced here.

The only part in which the contact_model is not specified is in the test, yet this is not a problem since the soft contact model is the default value

Ok. Instead, regarding the model reduction, I meant that something to prevent a problem similar to #168 is still missing, right?

flferretti · 2024-06-18T15:06:07Z

Can you double check in our codebase where JaxSimModel.build is called? For sure, the new JaxSimModel.contact_model attribute has to be copied when the model is reduced here.

The only part in which the contact_model is not specified is in the test, yet this is not a problem since the soft contact model is the default value

Ok. Instead, regarding the model reduction, I meant that something to prevent a problem similar to #168 is still missing, right?

It's indeed there

jaxsim/src/jaxsim/api/model.py

Line 369 in dfebf77

contact_model=model.contact_model,

diegoferigo · 2024-06-18T15:11:15Z

Can you double check in our codebase where JaxSimModel.build is called? For sure, the new JaxSimModel.contact_model attribute has to be copied when the model is reduced here.

The only part in which the contact_model is not specified is in the test, yet this is not a problem since the soft contact model is the default value

Ok. Instead, regarding the model reduction, I meant that something to prevent a problem similar to #168 is still missing, right?

It's indeed there

jaxsim/src/jaxsim/api/model.py

Line 369 in dfebf77

contact_model=model.contact_model,

Woops, sorry my bad. All good then, thanks!

src/jaxsim/api/model.py

Co-authored-by: Diego Ferigo <[email protected]>

….]]`

flferretti self-assigned this Jun 13, 2024

flferretti requested a review from diegoferigo as a code owner June 13, 2024 14:59

flferretti mentioned this pull request Jun 13, 2024

Add rigid contacts model #149

Closed

flferretti force-pushed the feature/base_contacts branch 3 times, most recently from 0755ca0 to c202825 Compare June 13, 2024 16:53

flferretti force-pushed the feature/base_contacts branch from c8560af to 8883e59 Compare June 14, 2024 13:23

flferretti force-pushed the feature/base_contacts branch from 8883e59 to daee668 Compare June 14, 2024 16:24

diegoferigo requested changes Jun 17, 2024

View reviewed changes

flferretti force-pushed the feature/base_contacts branch from 50bdbee to 8120bdf Compare June 17, 2024 13:24

flferretti force-pushed the feature/base_contacts branch 4 times, most recently from 4556dbb to 5640547 Compare June 17, 2024 13:53

flferretti requested a review from diegoferigo June 17, 2024 14:14

flferretti force-pushed the feature/base_contacts branch from d3b7d38 to 1a97511 Compare June 17, 2024 14:28

diegoferigo requested changes Jun 18, 2024

View reviewed changes

flferretti force-pushed the feature/base_contacts branch from f64764f to 2cac06c Compare June 18, 2024 09:08

diegoferigo requested changes Jun 18, 2024

View reviewed changes

flferretti force-pushed the feature/base_contacts branch from a9e3b2f to dfebf77 Compare June 18, 2024 14:45

flferretti requested a review from diegoferigo June 18, 2024 14:46

diegoferigo approved these changes Jun 18, 2024

View reviewed changes

src/jaxsim/api/model.py Outdated Show resolved Hide resolved

flferretti and others added 18 commits June 18, 2024 17:33

Add pre-commit configuration for Jupyter Notebooks

3eafca6

Add ContactParams, ContactModel, ContactsState ABCs

e66790c

Move soft_contacts to jaxsim.api

8def38e

Add contact_model attribute to JaxSimModel

0d4a60c

Use generic contact params name in JaxSimModelData

a4919b1

Update documentation

1e6dd86

Update tests

5ec77d3

Move SoftContactsState to soft_contacts

c9bdfed

Move base contact classes to jaxsim.rbda.contacts

ba6b50f

Make base contact classes inherit from abc only

efe0fb3

Rename method that computes contact forces

a89c91f

Co-authored-by: Diego Ferigo <[email protected]>

Rename ODEState.contact_state to ODEState.contact

fc8b994

Co-authored-by: Diego Ferigo <[email protected]>

Simplify contact model indentification logic

c39e9ec

Rename soft_contacts module to soft

965bddd

Fix contact state pattern matching in ode_data

1784910

Apply suggestions from code review

7ec9d13

Co-authored-by: Diego Ferigo <[email protected]>

Make compute_contact_forces return `tuple[jtp.Vector, tuple[Any, ..…

8f0118c

….]]`

Move contact_model attribute in JaxSimModel

11a557e

flferretti force-pushed the feature/base_contacts branch from 882a4a0 to 11a557e Compare June 18, 2024 15:33

flferretti merged commit 5653178 into main Jun 18, 2024
29 checks passed

flferretti deleted the feature/base_contacts branch June 18, 2024 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `ContactModel` base class and abstract contact handling in `JaxSimModel` and `JaxSimModelData` #178

Add `ContactModel` base class and abstract contact handling in `JaxSimModel` and `JaxSimModelData` #178

flferretti commented Jun 13, 2024 •

edited by github-actions bot

Loading

flferretti commented Jun 14, 2024

diegoferigo commented Jun 14, 2024

diegoferigo left a comment

flferretti commented Jun 17, 2024 •

edited

Loading

diegoferigo left a comment

diegoferigo Jun 18, 2024

flferretti Jun 18, 2024

diegoferigo Jun 18, 2024

flferretti Jun 18, 2024

diegoferigo left a comment

flferretti commented Jun 18, 2024

flferretti commented Jun 18, 2024

diegoferigo commented Jun 18, 2024

flferretti commented Jun 18, 2024

diegoferigo commented Jun 18, 2024

	) -> tuple[Any, ...]:
	) -> tuple[jtp.Matrix, tuple[Any, ...]]:

Add ContactModel base class and abstract contact handling in JaxSimModel and JaxSimModelData #178

Add ContactModel base class and abstract contact handling in JaxSimModel and JaxSimModelData #178

Conversation

flferretti commented Jun 13, 2024 • edited by github-actions bot Loading

flferretti commented Jun 14, 2024

diegoferigo commented Jun 14, 2024

diegoferigo left a comment

Choose a reason for hiding this comment

flferretti commented Jun 17, 2024 • edited Loading

diegoferigo left a comment

Choose a reason for hiding this comment

diegoferigo Jun 18, 2024

Choose a reason for hiding this comment

flferretti Jun 18, 2024

Choose a reason for hiding this comment

diegoferigo Jun 18, 2024

Choose a reason for hiding this comment

flferretti Jun 18, 2024

Choose a reason for hiding this comment

diegoferigo left a comment

Choose a reason for hiding this comment

flferretti commented Jun 18, 2024

flferretti commented Jun 18, 2024

diegoferigo commented Jun 18, 2024

flferretti commented Jun 18, 2024

diegoferigo commented Jun 18, 2024

Add `ContactModel` base class and abstract contact handling in `JaxSimModel` and `JaxSimModelData` #178

Add `ContactModel` base class and abstract contact handling in `JaxSimModel` and `JaxSimModelData` #178

flferretti commented Jun 13, 2024 •

edited by github-actions bot

Loading

flferretti commented Jun 17, 2024 •

edited

Loading