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.

Sign up for GitHub

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

Jump to bottom

Implement QPInverseKinematics python bindings #303

Merged
merged 21 commits into from
Jul 21, 2021
Merged

Implement QPInverseKinematics python bindings #303

merged 21 commits into from
Jul 21, 2021

Conversation

paolo-viceconte
Copy link
Contributor

This PR implements python bindings for the QPInverseKinematics class, which basically required to:

Tests for all the implemented bindings are provided.

@paolo-viceconte
Copy link
Contributor Author

This PR is ready to be reviewed, but it doesn't seem like I have permissions to ask for reviewers.

cc @GiulioRomualdi

@GiulioRomualdi GiulioRomualdi self-requested a review May 10, 2021 12:55
@GiulioRomualdi GiulioRomualdi added the enhancement New feature or request label May 10, 2021
@GiulioRomualdi
Copy link
Member

The tests are failing https://github.com/dic-iit/bipedal-locomotion-framework/pull/303/checks?check_run_id=2544820147#step:20:211 Did you experience the same failure also on your PC?

@paolo-viceconte
Copy link
Contributor Author

The tests are failing https://github.com/dic-iit/bipedal-locomotion-framework/pull/303/checks?check_run_id=2544820147#step:20:211 Did you experience the same failure also on your PC?

I just tried to run the tests several times on my pc and, after a few successfull runs, they indeed failed (no solution found for the ik).

The variability could be due to the (small) random variation of the joint position targets in test_qp_inverse_kinematics(). Let me fix it.

@paolo-viceconte
Copy link
Contributor Author

paolo-viceconte commented May 10, 2021 •
edited
Loading

Tests are not failing now in my local configuration, but they are still failing here.

Another fix required by this PR concerns the manif.SE3Tangent() objects in the tests, whose proper assignment is not completely clear for me. If you can provide suggestions on how to handle them @GiulioRomualdi, I can proceed with this additional fix and check the tests again.

EDIT: I replaced the initialization and the pytests on manif.SE3Tangent via 68f27ac and 86aba53.

GiulioRomualdi
GiulioRomualdi reviewed May 12, 2021
View reviewed changes
bindings/python/FloatingBaseEstimators/src/FloatingBaseEstimators.cpp Outdated Show resolved Hide resolved
bindings/python/IK/src/CoMTask.cpp Outdated Show resolved Hide resolved
bindings/python/IK/src/IntegrationBasedIK.cpp Outdated Show resolved Hide resolved
bindings/python/IK/src/JointTrackingTask.cpp Outdated Show resolved Hide resolved
bindings/python/IK/src/QPInverseKinematics.cpp Outdated Show resolved Hide resolved
bindings/python/IK/src/JointTrackingTask.cpp Outdated Show resolved Hide resolved
bindings/python/IK/src/SE3Task.cpp Outdated Show resolved Hide resolved
src/Estimators/include/BipedalLocomotion/FloatingBaseEstimators/ModelComputationsHelper.h Outdated Show resolved Hide resolved
bindings/python/FloatingBaseEstimators/src/FloatingBaseEstimators.cpp Outdated Show resolved Hide resolved
bindings/python/IK/tests/model.urdf Outdated Show resolved Hide resolved
@GiulioRomualdi GiulioRomualdi mentioned this pull request May 12, 2021
Merged
diegoferigo
diegoferigo suggested changes May 12, 2021
View reviewed changes
Copy link
Member

@diegoferigo diegoferigo left a comment

Choose a reason for hiding this comment

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

I need another pass, first few minor comments

bindings/python/System/src/Module.cpp Outdated Show resolved Hide resolved
bindings/python/FloatingBaseEstimators/tests/test_legged_odometry.py Outdated Show resolved Hide resolved
bindings/python/IK/src/CoMTask.cpp Outdated Show resolved Hide resolved
bindings/python/IK/src/CoMTask.cpp Outdated Show resolved Hide resolved
diegoferigo
diegoferigo approved these changes May 17, 2021
View reviewed changes
Copy link
Member

@diegoferigo diegoferigo left a comment

Choose a reason for hiding this comment

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

I'm going to approve the PR even though I'd like to have another zoom-out pass. Well done @paolo-viceconte!

bindings/python/FloatingBaseEstimators/src/FloatingBaseEstimators.cpp Outdated Show resolved Hide resolved
bindings/python/FloatingBaseEstimators/src/FloatingBaseEstimators.cpp Outdated Show resolved Hide resolved
@diegoferigo
Copy link
Member

Feel free to mark as solved addressed comments so we have less clutter in the PR.

@paolo-viceconte
Copy link
Contributor Author

paolo-viceconte commented Jun 7, 2021 •
edited
Loading

I added a few binded functions to the kindyn computations object via 086597a and the tests are now failing on macOS but I have no clue of the reason why.

EDIT: failure-related issue #324

Still, I need to address some open points in the reviews of this PR.

diegoferigo
diegoferigo approved these changes Jul 12, 2021
View reviewed changes
Copy link
Member

@diegoferigo diegoferigo left a comment

Choose a reason for hiding this comment

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

LGTM!

@GiulioRomualdi GiulioRomualdi mentioned this pull request Jul 15, 2021
Closed
@GiulioRomualdi
Copy link
Member

Let's wait for the CI if there are no problems I will merge it

@GiulioRomualdi
Copy link
Member

Conda is failing with this error:
https://github.com/dic-iit/bipedal-locomotion-framework/pull/303/checks?check_run_id=3116322815#step:7:321

@traversaro , is it due to conda-forge/manif-feedstock#9 ?

@GiulioRomualdi
Copy link
Member

GiulioRomualdi commented Jul 20, 2021 •
edited
Loading

The CI fails also because of #364 I will take care of the modification

@GiulioRomualdi
Copy link
Member

The CI fails also because of #364 I will take care of the modification

a8022f1 fixes the problem

@paolo-viceconte
Copy link
Contributor Author

The CI fails also because of #364 I will take care of the modification

a8022f1 fixes the problem

thanks @GiulioRomualdi!

@GiulioRomualdi
Copy link
Member

conda-forge/manif-feedstock#11 should fix the CI failure

@traversaro
Copy link
Collaborator

conda-forge/manif-feedstock#11 should fix the CI failure

Merged, it should probably take the usual 1/1.5 hours for CDN to propagate.

@GiulioRomualdi
Copy link
Member

The conda CI is passing. The windows one is still failing but it is not related to this PR. I'm currently debugging the issue here: #374

I think it's time to merge the PR.
Thank you all for the effort 🎉

@GiulioRomualdi GiulioRomualdi merged commit a933536 into ami-iit:master Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GiulioRomualdi GiulioRomualdi GiulioRomualdi left review comments

@traversaro traversaro traversaro left review comments

@diegoferigo diegoferigo diegoferigo approved these changes

Assignees

@paolo-viceconte paolo-viceconte

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@paolo-viceconte @GiulioRomualdi @diegoferigo @traversaro