ami-iit / adam

adam implements a collection of algorithms for calculating rigid-body dynamics in Jax, CasADi, PyTorch, and Numpy.
https://adam-docs.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
123 stars 19 forks source link

Add parametrization w.r.t Hardware Parameters #49

Closed CarlottaSartore closed 9 months ago

CarlottaSartore commented 10 months ago

With this PR the changes done in the tag AdamWithHardwareParameters have been ported into main .

@Giulero I have tried to be compliant with the refactor you did in https://github.com/ami-iit/ADAM/pull/37, but please let me know if you would prefer a different code organization :)

Currently the CI are failing due to #https://github.com/icub-tech-iit/ergocub-gazebo-simulations/issues/49 but in my machine everything is working fine, I will update the CI as soon as we find a way to solve the issue.

C.C. @S-Dafarra @lrapetti @traversaro

CarlottaSartore commented 10 months ago

Solved the CI in 5bec8244823cbb87e864b2d5e5692b7227401d8e

I added a workaround to avoid reading the line of the stickbot urdf declaring the XML encoding, this was an outcome of a conversation done in https://github.com/icub-tech-iit/ergocub-gazebo-simulations/issues/49#issuecomment-1798997651

traversaro commented 10 months ago

I added a workaround to avoid reading the line of the stickbot urdf declaring the XML encoding, this was an outcome of a conversation done in icub-tech-iit/ergocub-gazebo-simulations#49 (comment)

Can't we add the hack directly in https://github.com/CarlottaSartore/ADAM/blob/3ad9e5d64a3469856db22b8f7e3f35422b40f0db/src/adam/core/urdf_tree.py#L45 ? Otherwise ADAM will continue to not work for stickbot or similar models.

CarlottaSartore commented 10 months ago

Can't we add the hack directly in https://github.com/CarlottaSartore/ADAM/blob/3ad9e5d64a3469856db22b8f7e3f35422b40f0db/src/adam/core/urdf_tree.py#L45 ? Otherwise ADAM will continue to not work for stickbot or similar models.

For me, it is fine, if @Giulero agrees I can add it directly to this PR

traversaro commented 10 months ago

Can't we add the hack directly in https://github.com/CarlottaSartore/ADAM/blob/3ad9e5d64a3469856db22b8f7e3f35422b40f0db/src/adam/core/urdf_tree.py#L45 ? Otherwise ADAM will continue to not work for stickbot or similar models.

For me, it is fine, if @Giulero agrees I can add it directly to this PR

By the way, a compact way to remove the header if present seems to be:

output_xml_string = ET.tostring(ET.fromstring(input_xml_string), xml_declaration=False)

This is a bit more robust as the xml declaration could also have a non-UTF-8 encoding, or also have other attributes such as standalone (see https://www.tutorialspoint.com/xml/xml_declaration.htm).

CarlottaSartore commented 10 months ago

About this

By the way, a compact way to remove the header if present seems to be:

output_xml_string = ET.tostring(ET.fromstring(input_xml_string), xml_declaration=False) This is a bit more robust as the xml declaration could also have a non-UTF-8 encoding, or also have other attributes such as standalone (see https://www.tutorialspoint.com/xml/xml_declaration.htm).

Thanks to https://github.com/ros/urdf_parser_py/pull/83#issuecomment-1802866735 we should have it fixed now!

@Giulero I have implemented the refactor we talked f2f and the CI passed !

traversaro commented 10 months ago

Thanks to ros/urdf_parser_py#83 (comment) we should have it fixed now!

Sorry, I missed something. How it is possible that that fixed the problem if it still needs to be merged and released?

CarlottaSartore commented 10 months ago

Sorry, I missed something. How it is possible that that fixed the problem if it still needs to be merged and released? Yes, but once it is merged it will be fix!

traversaro commented 10 months ago

Sorry, I missed something. How it is possible that that fixed the problem if it still needs to be merged and released? Yes, but once it is merged it will be fix!

Sure, but how why the CI is green now?

CarlottaSartore commented 10 months ago

Sure, but how why the CI is green now?

In https://github.com/ami-iit/ADAM/commit/5bec8244823cbb87e864b2d5e5692b7227401d8e I have modified the CI to remove the encoding of the stickbot, thus the CI runs, see

https://github.com/CarlottaSartore/ADAM/blob/5bdcf9c3c575dd6d15f6265b5c967a7e8bf38e3a/tests/parametric/test_CasADi_computations_parametric.py#L25-L32

But the problem is not solved in adam itself

traversaro commented 10 months ago

Sure, but how why the CI is green now?

In https://github.com/ami-iit/ADAM/commit/5bec8244823cbb87e864b2d5e5692b7227401d8e I have modified the CI to remove the encoding of the stickbot, thus the CI runs, see

https://github.com/CarlottaSartore/ADAM/blob/5bdcf9c3c575dd6d15f6265b5c967a7e8bf38e3a/tests/parametric/test_CasADi_computations_parametric.py#L25-L32

But the problem is not solved in adam itself

Ahh so we have still the workaround, I missed that.

CarlottaSartore commented 9 months ago

@Giulero I should have resolved all your comments, however I see the CI fails, namely the one related to pytorch and Jax

Giulero commented 9 months ago

Thanks @CarlottaSartore! Yeah the CI fails since there are some issues related to the new default Python version in conda.
I'm opening an issue for this and solve it in a different PR.

Giulero commented 9 months ago

Thanks @CarlottaSartore ! I guess we can merge :) There is just a conflict with the PR #53. Solving it!