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

Add parametrization w.r.t Hardware Parameters #49

Merged
merged 20 commits into from
Dec 6, 2023

Conversation

CarlottaSartore
Copy link
Collaborator

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 #37, but please let me know if you would prefer a different code organization :)

Currently the CI are failing due to #icub-tech-iit/ergocub-gazebo-simulations#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
Copy link
Collaborator Author

Solved the CI in 5bec824

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)

@traversaro
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

traversaro commented Nov 7, 2023

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
Copy link
Collaborator Author

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 ros/urdf_parser_py#83 (comment) we should have it fixed now!

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

@traversaro
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Collaborator Author

Sure, but how why the CI is green now?

In 5bec824 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
Copy link
Contributor

Sure, but how why the CI is green now?

In 5bec824 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.

Copy link
Collaborator

@Giulero Giulero left a comment

Choose a reason for hiding this comment

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

Just few comments!

src/adam/casadi/casadi_like.py Outdated Show resolved Hide resolved
Comment on lines +245 to +247
T[0, 3] = xyz[0]
T[1, 3] = xyz[1]
T[2, 3] = xyz[2]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remember me why the assignment T[:3, 3] = xyz does not work?
It's more of a log for the feature :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The issue was that xyz contains both symbolic and not symbolic elements if the robot model is parametric, therefore it was complaining due to different types in xyz !

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it!

@@ -381,6 +400,33 @@ def spatial_inertia(
IO[:3, :3] = self.factory.eye(3) * mass
return IO

def spatial_inertial_with_parameter(self, I, mass, c, rpy):
Copy link
Collaborator

Choose a reason for hiding this comment

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

parameter or parameters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in ab08cf9

root_link (str, optional): the first link. Defaults to 'root_link'.
"""
math = SpatialMath()
n_param_link = len(links_name_list)
Copy link
Collaborator

Choose a reason for hiding this comment

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

link or links?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in ab08cf9

src/adam/parametric/jax/computations_parametric.py Outdated Show resolved Hide resolved
tests/parametric/test_CasADi_computations_parametric.py Outdated Show resolved Hide resolved
@CarlottaSartore
Copy link
Collaborator Author

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

@Giulero
Copy link
Collaborator

Giulero commented Dec 5, 2023

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
Copy link
Collaborator

Giulero commented Dec 6, 2023

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

@Giulero Giulero merged commit 115a87a into ami-iit:main Dec 6, 2023
6 of 7 checks passed
@Giulero Giulero mentioned this pull request Dec 6, 2023
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.

3 participants