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

Minor improvements: clean-up examples, throw_pretty in Python actuaction, and remove memory allocation in quasiStatic #980

Merged
merged 19 commits into from
Aug 2, 2021

Conversation

cmastalli
Copy link
Member

The changes on the examples are targeted to extend them along a set of cases that used inverse-dynamics (instead of the current forward dynamics).

Note that the inverse-dynamics based optimal control is still an ongoing work which will be released in Crocoddyl v2.

@cmastalli
Copy link
Member Author

@wxmerkt -- do you know why the ROS-CIs are not passing?

@wxmerkt
Copy link
Collaborator

wxmerkt commented Jul 30, 2021

The momentum cost test failed. I restarted it in case it was flaky. However, it's likely that we are already using Pinocchio 2.6.3 on the ROS-CI which has the fixed centroidal momentum derivatives in case that has an impact here or requires adjustments to the unit test.
30 - pybinds-momemtum_cost (Failed)

@cmastalli
Copy link
Member Author

The momentum cost test failed. I restarted it in case it was flaky. However, it's likely that we are already using Pinocchio 2.6.3 on the ROS-CI which has the fixed centroidal momentum derivatives in case that has an impact here or requires adjustments to the unit test.
30 - pybinds-momemtum_cost (Failed)

I tested locally, the error appears with Pinocchio 2.3.6 (i.e., after the changes in stack-of-tasks/pinocchio#1474).

It seems that we don't need anymore these lines: https://github.com/loco-3d/crocoddyl/blob/devel/include/crocoddyl/multibody/residuals/centroidal-momentum.hxx#L50-L52. At least, the test is passing if I remove them.

@jcarpent -- I need your help here. Should I remove those lines?

@jcarpent
Copy link
Member

@jcarpent -- I need your help here. Should I remove those lines?

Yes, you have to.

@proyan
Copy link
Member

proyan commented Jul 30, 2021

Yes, I put these lines as a local solution for the bug. Since we have now fixed it in pinocchio as well, we should also update the minimum version to 2.6.3

@proyan
Copy link
Member

proyan commented Jul 30, 2021

Also, it looks like LAAS-CNRS is missing from the copyright in all residual models. Since the residual-models were refactored from the original cost-models, the copyright should remain.

@cmastalli
Copy link
Member Author

Also, it looks like LAAS-CNRS is missing from the copyright in all residual models. Since the residual-models were refactored from the original cost-models, the copyright should remain.

Done!

The code is ready to be merged; however, gitlab-ci is failing since we need to deploy 2.6.3 in robotpkg. @nim65s -- do you know when it is expected the new release.

@cmastalli
Copy link
Member Author

I will merge this PR as ROS-CI is checking all the unittests (and I don't want to wait for robotpkg release)

@cmastalli cmastalli merged commit ad3d753 into loco-3d:devel Aug 2, 2021
@cmastalli cmastalli deleted the topic/minor-improvements branch August 2, 2021 10:16
@nim65s
Copy link
Collaborator

nim65s commented Aug 2, 2021

We could have used a version check instead of requiring the latest version.

@cmastalli
Copy link
Member Author

cmastalli commented Aug 3, 2021

We could have used a version check instead of requiring the latest version.

I indeed though about this possibility, but it seems more convenient to constrain everyone moving to another version as this is a bug.
We could still add the version check (as we just merged it), if you believe it is a good idea.

@proyan
Copy link
Member

proyan commented Aug 3, 2021

@cmastalli Accepting PRs when CI is failing is never a good idea. It might not be the cleanest way forward, but it is better to leave a PR open than to accept it without the CI.

@cmastalli
Copy link
Member Author

@cmastalli Accepting PRs when CI is failing is never a good idea. It might not be the cleanest way forward, but it is better to leave a PR open than to accept it without the CI.

There is redundancy, the ROS-CI is testing the same bits but with Pinocchio 2.6.3.

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.

5 participants