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

add python test for contactInverseDynamics #2263

Merged
merged 7 commits into from
May 30, 2024
Merged

add python test for contactInverseDynamics #2263

merged 7 commits into from
May 30, 2024

Conversation

fabinsch
Copy link
Contributor

@fabinsch fabinsch commented May 30, 2024 •
edited
Loading

In a similar way to the cpp test for the contactInverseDynamics function, I propose to add a python test and check if this function is usable from Python.

the c++ function requires for the contact models, data and cones

contactInverseDynamics(..., 
const std::vector<RigidConstraintModelTpl<Scalar, Options>, ConstraintModelAllocator> & contact_models,
std::vector<RigidConstraintDataTpl<Scalar, Options>, ConstraintDataAllocator> & contact_datas,
const std::vector<CoulombFrictionConeTpl<Scalar>, CoulombFrictionConelAllocator> & cones,

and this works well when constructing them as StdVec_RigidConstraintData() and then appending contact data here (see test1). However, if you try to pass a list of contact data, it does not work (see test3).
This is not the case for contact models and cones, they work both as list of model/cones and as pin.StdVec (see test2).

The different is, that models/cones are const std::vectors while data is not const.

@fabinsch fabinsch requested a review from jorisv May 30, 2024 08:26
@fabinsch fabinsch force-pushed the add-py-unittest-constraint-inverse-dynamics branch 4 times, most recently from 5e8bf6e to 57a9280 Compare May 30, 2024 15:02
@fabinsch
Copy link
Contributor Author

The problem of calling the Python bindings of contactInverseDynamics with a list of RigidConstraintData objects described above is fixed with 5dc80d6.

@ManifoldFR
Copy link
Member

The problem of calling the Python bindings of contactInverseDynamics with a list of RigidConstraintData objects described above is fixed with 5dc80d6.

I wasn't aware this list.hpp header even existed. I don't even know if it's actually useful.

This is a super-subtle bug, great investigation job @fabinsch !

@jcarpent jcarpent force-pushed the add-py-unittest-constraint-inverse-dynamics branch from 57a9280 to 7ee9e8f Compare May 30, 2024 15:16
jorisv
jorisv previously approved these changes May 30, 2024
View reviewed changes
jcarpent
jcarpent previously approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@jcarpent jcarpent left a comment

Choose a reason for hiding this comment

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

Very great job @fabinsch. Thanks for providing the fix AND the precious unit test.

@jorisv jorisv enabled auto-merge May 30, 2024 15:22
@fabinsch
Copy link
Contributor Author

I don't even know if it's actually useful.

I checked and we use it here in bindings/python/parsers/urdf/geometry.cpp. However, I am not sure why we also include it in expose-model.cpp and expose-rnea.cpp. This should probably be removed there.

@jorisv jorisv disabled auto-merge May 30, 2024 15:43
@fabinsch fabinsch dismissed stale reviews from jcarpent and jorisv via d3d0bdf May 30, 2024 15:47
@jorisv jorisv enabled auto-merge May 30, 2024 15:57
@jcarpent
Copy link
Contributor

I don't even know if it's actually useful.

I checked and we use it here in bindings/python/parsers/urdf/geometry.cpp. However, I am not sure why we also include it in expose-model.cpp and expose-rnea.cpp. This should probably be removed there.

We use in rnea because of fext arg, we might be a list.

@fabinsch fabinsch disabled auto-merge May 30, 2024 16:52
@fabinsch
Copy link
Contributor Author

yes, you are right that the external forces can be passed as a list from python to the rnea bindings.

I checked why our unittest for this case is still working (see CI) without the list.hpp header in expose-rnea. The reason is, that the StdAlignedVectorPythonVisitor for Force, called here to expose StdVec_Force, implements a FromPythonListConverter here. I also verified this on my machine with a quick test.

@jcarpent jcarpent merged commit 2ecbc9b into stack-of-tasks:devel May 30, 2024
18 checks passed
nim65s added a commit to nim65s/robotpkg that referenced this pull request Oct 9, 2024
@nim65s
[math/{py-,}pinocchio] Update to v3.2.0
197d1f3 
    ## [3.2.0] - 2024-08-27

    ### Fixed
    - Append pinocchio optional libraries into pkg-config file (stack-of-tasks/pinocchio#2322)
    - Fixed support of DAE meshes with MeshCat (stack-of-tasks/pinocchio#2331)
    - Fixed pointer casts in urdf parser (stack-of-tasks/pinocchio#2339)
    - Remove CMake CMP0167 warnings (stack-of-tasks/pinocchio#2347)
    - Fixed urdfdom in ROS packaging (stack-of-tasks/pinocchio#2341)
    - Fixed overview-urdf cpp example (stack-of-tasks/pinocchio#2384)
    - Fixed mjcf model without a base link parsing (stack-of-tasks/pinocchio#2386)
    - Fixed talos-simulation.py, simulation-contact-dynamics.py and simulation-closed-kinematic-chains.py examples (stack-of-tasks/pinocchio#2392)

    ### Added
    - Add getMotionAxis method to helical, prismatic, revolute and ubounded revolute joint (stack-of-tasks/pinocchio#2315)
    - Add initial compatiblity with coal (coal needs `-DCOAL_BACKWARD_COMPATIBILITY_WITH_HPP_FCL=ON`) (stack-of-tasks/pinocchio#2323)
    - Add compatibility with jrl-cmakemodules workspace (stack-of-tasks/pinocchio#2333)
    - Add ``collision_color`` parameter to `MeshcatVisualizer.loadViewerModel` (stack-of-tasks/pinocchio#2350)
    - Add ``BuildFromMJCF`` function to RobotWrapper (stack-of-tasks/pinocchio#2363)
    - Add more CasADi examples (stack-of-tasks/pinocchio#2388)

    ### Removed
    - Remove deprecated headers related to joint constraints (stack-of-tasks/pinocchio#2382)

    ### Changed
    - Use eigenpy to expose `GeometryObject::meshMaterial` variant (stack-of-tasks/pinocchio#2315)
    - GepettoViewer is no more the default viewer for RobotWrapper (stack-of-tasks/pinocchio#2331)
    - Modernize python code base with ruff (stack-of-tasks/pinocchio#2367)
    - Restructure CppAD and CasADi examples (stack-of-tasks/pinocchio#2388)
    - Enhance and fix CppAD benchmarks outputs (stack-of-tasks/pinocchio#2393)

    ## [3.1.0] - 2024-07-04

    ### Fixed

    - Fix `appendModel` when joints after the base are in parallel (stack-of-tasks/pinocchio#2295)
    - Fix `appendModel` build when called with template arguments different than the ones from `context` (stack-of-tasks/pinocchio#2284)
    - Fix `TransformRevoleTpl::rotation` and `TransformHelicalTpl::rotation` build (stack-of-tasks/pinocchio#2284)
    - Fix compilation issue for Boost 1.85 (stack-of-tasks/pinocchio#2255)
    - Fix python bindings of `contactInverseDynamics` (stack-of-tasks/pinocchio#2263)
    - Deactivate `BUILD_WITH_LIBPYTHON` when building with PyPy (stack-of-tasks/pinocchio#2274)
    - Fix Python bindings cross building with `hpp-fcl` (stack-of-tasks/pinocchio#2288)
    - Fix build issue on Windows when a deprecated header is included (stack-of-tasks/pinocchio#2292)
    - Fix build issue on Windows when building in Debug mode (stack-of-tasks/pinocchio#2292)
    - Fix visualization of meshes in meshcat (stack-of-tasks/pinocchio#2294)
    - Fix Anymal simulation test (stack-of-tasks/pinocchio#2299)
    - Fix contact derivatives and impulse dynamics tests (stack-of-tasks/pinocchio#2300)
    - Fix CMake compatibility with old console_bridge version (stack-of-tasks/pinocchio#2312)

    ### Added

    - Python unittest for `contactInverseDynamics` function (stack-of-tasks/pinocchio#2263)
    - Added helper functions to return operation count of CasADi functions. (stack-of-tasks/pinocchio#2275)
    - C++ and Python unittest for `dIntegrateTransport` to check vector transport and its inverse (stack-of-tasks/pinocchio#2273)
    - Add kinetic and potential energy regressors (stack-of-tasks/pinocchio#2282)

    ### Removed

    - Remove header `list.hpp` include for bindings of model and rnea (stack-of-tasks/pinocchio#2263)

Packaging Changes:
- Removed patches af, an, ap: fixed upstream
- Updated patches ag, ak
nim65s added a commit to nim65s/robotpkg that referenced this pull request Oct 11, 2024
@nim65s
[math/{py-,}pinocchio] Update to v3.2.0
1ccd712 
    ## [3.2.0] - 2024-08-27

    ### Fixed
    - Append pinocchio optional libraries into pkg-config file (stack-of-tasks/pinocchio#2322)
    - Fixed support of DAE meshes with MeshCat (stack-of-tasks/pinocchio#2331)
    - Fixed pointer casts in urdf parser (stack-of-tasks/pinocchio#2339)
    - Remove CMake CMP0167 warnings (stack-of-tasks/pinocchio#2347)
    - Fixed urdfdom in ROS packaging (stack-of-tasks/pinocchio#2341)
    - Fixed overview-urdf cpp example (stack-of-tasks/pinocchio#2384)
    - Fixed mjcf model without a base link parsing (stack-of-tasks/pinocchio#2386)
    - Fixed talos-simulation.py, simulation-contact-dynamics.py and simulation-closed-kinematic-chains.py examples (stack-of-tasks/pinocchio#2392)

    ### Added
    - Add getMotionAxis method to helical, prismatic, revolute and ubounded revolute joint (stack-of-tasks/pinocchio#2315)
    - Add initial compatiblity with coal (coal needs `-DCOAL_BACKWARD_COMPATIBILITY_WITH_HPP_FCL=ON`) (stack-of-tasks/pinocchio#2323)
    - Add compatibility with jrl-cmakemodules workspace (stack-of-tasks/pinocchio#2333)
    - Add ``collision_color`` parameter to `MeshcatVisualizer.loadViewerModel` (stack-of-tasks/pinocchio#2350)
    - Add ``BuildFromMJCF`` function to RobotWrapper (stack-of-tasks/pinocchio#2363)
    - Add more CasADi examples (stack-of-tasks/pinocchio#2388)

    ### Removed
    - Remove deprecated headers related to joint constraints (stack-of-tasks/pinocchio#2382)

    ### Changed
    - Use eigenpy to expose `GeometryObject::meshMaterial` variant (stack-of-tasks/pinocchio#2315)
    - GepettoViewer is no more the default viewer for RobotWrapper (stack-of-tasks/pinocchio#2331)
    - Modernize python code base with ruff (stack-of-tasks/pinocchio#2367)
    - Restructure CppAD and CasADi examples (stack-of-tasks/pinocchio#2388)
    - Enhance and fix CppAD benchmarks outputs (stack-of-tasks/pinocchio#2393)

    ## [3.1.0] - 2024-07-04

    ### Fixed

    - Fix `appendModel` when joints after the base are in parallel (stack-of-tasks/pinocchio#2295)
    - Fix `appendModel` build when called with template arguments different than the ones from `context` (stack-of-tasks/pinocchio#2284)
    - Fix `TransformRevoleTpl::rotation` and `TransformHelicalTpl::rotation` build (stack-of-tasks/pinocchio#2284)
    - Fix compilation issue for Boost 1.85 (stack-of-tasks/pinocchio#2255)
    - Fix python bindings of `contactInverseDynamics` (stack-of-tasks/pinocchio#2263)
    - Deactivate `BUILD_WITH_LIBPYTHON` when building with PyPy (stack-of-tasks/pinocchio#2274)
    - Fix Python bindings cross building with `hpp-fcl` (stack-of-tasks/pinocchio#2288)
    - Fix build issue on Windows when a deprecated header is included (stack-of-tasks/pinocchio#2292)
    - Fix build issue on Windows when building in Debug mode (stack-of-tasks/pinocchio#2292)
    - Fix visualization of meshes in meshcat (stack-of-tasks/pinocchio#2294)
    - Fix Anymal simulation test (stack-of-tasks/pinocchio#2299)
    - Fix contact derivatives and impulse dynamics tests (stack-of-tasks/pinocchio#2300)
    - Fix CMake compatibility with old console_bridge version (stack-of-tasks/pinocchio#2312)

    ### Added

    - Python unittest for `contactInverseDynamics` function (stack-of-tasks/pinocchio#2263)
    - Added helper functions to return operation count of CasADi functions. (stack-of-tasks/pinocchio#2275)
    - C++ and Python unittest for `dIntegrateTransport` to check vector transport and its inverse (stack-of-tasks/pinocchio#2273)
    - Add kinetic and potential energy regressors (stack-of-tasks/pinocchio#2282)

    ### Removed

    - Remove header `list.hpp` include for bindings of model and rnea (stack-of-tasks/pinocchio#2263)

Packaging Changes:
- Removed patches af, an, ap: fixed upstream
- Updated patches ag, ak
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorisv jorisv jorisv approved these changes

@jcarpent jcarpent jcarpent left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fabinsch @ManifoldFR @jcarpent @jorisv