made repo pip installable

open-dynamic-robot-initiative / robot_properties_solo

BSD 3-Clause "New" or "Revised" License

40 stars 20 forks source link

made repo pip installable #31

Closed huaijiangzhu closed 3 years ago

huaijiangzhu commented 3 years ago

Description

How I Tested

Do not merge before

[ ] link to dependent pull request

I fulfilled the following requirements

[ ] All new code is formatted according to our style guide (for C++ run clang-format, for Python, run flake8 and fix all warnings).
[ ] All new functions/classes are documented and existing documentation is updated according to changes.
[ ] No commented code from testing/debugging is kept (unless there is a good reason to keep it).

jviereck commented 3 years ago

Thanks for your contribution! Approving with a few small comments.

I don't see a PR for bullet_utils yet. Are you planning to create a pull request for pinocchio_bullet as well and then rename the repo?

huaijiangzhu commented 3 years ago

Thanks for your contribution! Approving with a few small comments.

I don't see a PR for bullet_utils yet. Are you planning to create a pull request for pinocchio_bullet as well and then rename the repo?

Max and I agreed that we will simply create a new repo for bullet_utils to spare the hustle. What do you think?

jviereck commented 3 years ago

Max and I agreed that we will simply create a new repo for bullet_utils to spare the hustle. What do you think?

That's fine with me. We should have the new bullet_utils in place before merging this PR.

jviereck commented 3 years ago

I found the issue - in the test code of kino-dyn-opt, we use eigenpy.switchToNumpyMatrix() and pinocchio.switchToNumpyMatrix(). If I comment this out all the tests pass without problem and the error is not raised anymore. However I do not know the consequences of this for the rest of the code. Anyone has any thoughts on this?

Our code is intended to work with eigenpy.switchToNumpyArrat() by default by now (which is also the default by pinocchio and what we agreed on a while back). If you remove the line and thing still work, then it's all good ;)

Some background what goes on under the hood: Eigenpy allows to return np.matrix or np.array for Eigen matrixes. It turns out that returning np.array is 1) faster and 2) the future as the use of np.matrix is deprecated. It used to be the default that eigenpy/pinocchio returned np.matrixes. This is no longer the default and it returns np.arrays by now.

MaximilienNaveau commented 3 years ago

switchToNumpyMatrix

You are right this is the bug! This was used for historical reasons but must be deleted (see previous message of Julian) The last versionS of pinocchio switched to numpy.array hence should be compatible with the config files.