Repository: stack-of-tasks/eigenpy (language: python)

Issue:
Python bindings changing type depending on imports
Hello all, I've found what I think is an issue that I would like your comments, please.

## Bug description

In a python script, I am trying to import both pinocchio and ompl, which I am able to. The strange behavior that I am witnessing is that, depending on which one I import first, the types of the variables within pinocchio AND ompl change! 

## Expected behavior

I was expecting them to don't change.

## Reproduction steps

When I run 

```py
from os.path import abspath, dirname, join
from sys import argv

from ompl import base, geometric
import pinocchio

# This path refers to Pinocchio source code but you can define your own directory here.
pinocchio_model_dir = join(dirname(dirname(str(abspath(__file__)))), "models")

# You should change here to set up your own URDF file or just pass it as an argument of this example.
urdf_filename = (
    pinocchio_model_dir
    + "/example-robot-data/robots/ur_description/urdf/ur5_robot.urdf"
    if len(argv) < 2
    else argv[1]
)

# Load the urdf model
model = pinocchio.buildModelFromUrdf(urdf_filename)
# print("model name: " + model.name)

# Create data required by the algorithms
data = model.createData()

print("Pinocchio model.names type:", end=" ")
print(type(model.names))
print("")


bounds = base.RealVectorBounds(model.nq)
print("OMPL base.RealVectorBounds.high type:", end=" ")
print(type(bounds.high))
print("")
```

I expected the output to be 

```
Pinocchio model.names type: <class 'pinocchio.pinocchio_pywrap.StdVec_StdString'>

OMPL base.RealVectorBounds.high type: <class 'ompl.util._util.vectorDouble'>
```

Yet the actual output is 

```
Pinocchio model.names type: <class 'ompl.util._util.vectorString'>

OMPL base.RealVectorBounds.high type: <class 'ompl.util._util.vectorDouble'
```

and if switch from 

```py
from ompl import base, geometric
import pinocchio
```

to

```py
import pinocchio
from ompl import base, geometric
```

the output becomes

```
Pinocchio model.names type: <class 'pinocchio.pinocchio_pywrap.StdVec_StdString'>

OMPL base.RealVectorBounds.high type: <class 'pinocchio.pinocchio_pywrap.StdVec_Double'
```


## Additional context


### System

- OS: ubuntu 22.04
- Pinocchio version: 2.6.20



Discussion / hints:
Hi @joao-pm-santos96,

Do you have any issue other than the name changes ?

I guess ompl is also using boost python to define how to translate some simple stuff between C++ and Python, and it might be defined in the same way and do the same thing, so changing the name wouldn't be an issue.
As mentioned by @nim65s, behind it will be the same C++ objects. This is indeed related to Boost.Python: it just registers only type to avoid duplicate symbols and incompatibility issues.
Hi @nim65s and @jcarpent . Once again your speed of response was incredible. Thank you!

> Do you have any issue other than the name changes ?

I've found this behavior because if I run 

```py
jointsToLockIDs = [1, 2, 3]
initialJointConfig = np.zeros(model.nq)
model_reduced = pinocchio.buildReducedModel(model, jointsToLockIDs, initialJointConfig)
``` 

when I first import ompl and only after pinocchio, I get the error

```
Exception has occurred: ArgumentError
Python argument types in
    pinocchio.pinocchio_pywrap.buildReducedModel(Model, list, numpy.ndarray)
did not match C++ signature:
    buildReducedModel(pinocchio::ModelTpl<double, 0, pinocchio::JointCollectionDefaultTpl> model, std::vector<pinocchio::GeometryModel, Eigen::aligned_allocator<pinocchio::GeometryModel> > list_of_geom_models, std::vector<unsigned long, std::allocator<unsigned long> > list_of_joints_to_lock, Eigen::MatrixBase<Eigen::Matrix<double, -1, 1, 0, -1, 1> > reference_configuration)
    buildReducedModel(pinocchio::ModelTpl<double, 0, pinocchio::JointCollectionDefaultTpl> model, pinocchio::GeometryModel geom_model, std::vector<unsigned long, std::allocator<unsigned long> > list_of_joints_to_lock, Eigen::MatrixBase<Eigen::Matrix<double, -1, 1, 0, -1, 1> > reference_configuration)
    buildReducedModel(pinocchio::ModelTpl<double, 0, pinocchio::JointCollectionDefaultTpl> model, std::vector<unsigned long, std::allocator<unsigned long> > list_of_joints_to_lock, Eigen::MatrixBase<Eigen::Matrix<double, -1, 1, 0, -1, 1> > reference_configuration)
  File "/home/joao/test/main.py", line 39, in <module>
    model_reduced = pinocchio.buildReducedModel(model, jointsToLockIDs, initialJointConfig)
Boost.Python.ArgumentError: Python argument types in
    pinocchio.pinocchio_pywrap.buildReducedModel(Model, list, numpy.ndarray)
did not match C++ signature:
    buildReducedModel(pinocchio::ModelTpl<double, 0, pinocchio::JointCollectionDefaultTpl> model, std::vector<pinocchio::GeometryModel, Eigen::aligned_allocator<pinocchio::GeometryModel> > list_of_geom_models, std::vector<unsigned long, std::allocator<unsigned long> > list_of_joints_to_lock, Eigen::MatrixBase<Eigen::Matrix<double, -1, 1, 0, -1, 1> > reference_configuration)
    buildReducedModel(pinocchio::ModelTpl<double, 0, pinocchio::JointCollectionDefaultTpl> model, pinocchio::GeometryModel geom_model, std::vector<unsigned long, std::allocator<unsigned long> > list_of_joints_to_lock, Eigen::MatrixBase<Eigen::Matrix<double, -1, 1, 0, -1, 1> > reference_configuration)
    buildReducedModel(pinocchio::ModelTpl<double, 0, pinocchio::JointCollectionDefaultTpl> model, std::vector<unsigned long, std::allocator<unsigned long> > list_of_joints_to_lock, Eigen::MatrixBase<Eigen::Matrix<double, -1, 1, 0, -1, 1> > reference_configuration)
```

which does not happen if I import them the other way around. I just don't know which problems I will potentially encounter further down the line...



> As mentioned by @nim65s, behind it will be the same C++ objects. This is indeed related to Boost.Python: it just registers only type to avoid duplicate symbols and incompatibility issues.

One thing that differs is that most of your implementations of `std::vector` have the `.tolist()` method while ompl's don't. This obviously is a non issue since we cant use python's `list()` but is just one more difference that I don't know if they will or wont create problems further down the line.
You're right. We can work on adding it when ompl is called first. It will take some time because we have a limited bandwidth.
@jorisv If you are interested, I can explain you how to do this properly in EigenPy
Awesome, you guys are amazing, thank you!
@jcarpent I'm interested in working on it. I will create a MR soon.

Fix patch:
diff --git a/include/eigenpy/registration.hpp b/include/eigenpy/registration.hpp
index f7f77b69a..7644f2916 100644
--- a/include/eigenpy/registration.hpp
+++ b/include/eigenpy/registration.hpp
@@ -7,6 +7,7 @@
 #define __eigenpy_registration_hpp__
 
 #include "eigenpy/fwd.hpp"
+#include "eigenpy/registration_class.hpp"
 
 namespace eigenpy {
 
@@ -50,6 +51,25 @@ inline bool register_symbolic_link_to_registered_type() {
 
   return false;
 }
+
+/// Same as \see register_symbolic_link_to_registered_type() but apply \p
+/// visitor on \tparam T if it already exists
+template <typename T, typename Visitor>
+inline bool register_symbolic_link_to_registered_type(const Visitor& visitor) {
+  if (eigenpy::check_registration<T>()) {
+    const bp::type_info info = bp::type_id<T>();
+    const bp::converter::registration* reg =
+        bp::converter::registry::query(info);
+    bp::handle<> class_obj(reg->get_class_object());
+    bp::object object(class_obj);
+    bp::scope().attr(reg->get_class_object()->tp_name) = object;
+    registration_class<T> cl(object);
+    cl.def(visitor);
+    return true;
+  }
+
+  return false;
+}
 }  // namespace eigenpy
 
 #endif  // ifndef __eigenpy_registration_hpp__
diff --git a/include/eigenpy/registration_class.hpp b/include/eigenpy/registration_class.hpp
new file mode 100644
index 000000000..ebe72d7f7
--- /dev/null
+++ b/include/eigenpy/registration_class.hpp
@@ -0,0 +1,129 @@
+/*
+ * Copyright 2023, INRIA
+ */
+
+#ifndef __eigenpy_registration_class_hpp__
+#define __eigenpy_registration_class_hpp__
+
+#include <boost/python/class.hpp>
+
+#include "eigenpy/fwd.hpp"
+
+namespace eigenpy {
+
+/*! Copy of the \see boost::python::class_
+ * This class allow to add methods to an existing class without registering it
+ * again.
+ **/
+template <class W>
+class registration_class {
+ public:
+  using self = registration_class;
+
+  /// \p object Hold the namespace of the class that will be modified
+  registration_class(bp::object object) : m_object(object) {}
+
+  /// \see boost::python::class_::def(bp::def_visitor<Derived> const& visitor)
+  template <class Visitor>
+  self& def(Visitor const& visitor) {
+    visitor.visit(*this);
+    return *this;
+  }
+
+  /// \see boost::python::class_::def(char const* name, F f)
+  template <class F>
+  self& def(char const* name, F f) {
+    def_impl(bp::detail::unwrap_wrapper((W*)0), name, f,
+             bp::detail::def_helper<char const*>(0), &f);
+    return *this;
+  }
+
+  /// \see boost::python::class_::def(char const* name, A1 a1, A2 const& a2)
+  template <class A1, class A2>
+  self& def(char const* name, A1 a1, A2 const& a2) {
+    def_maybe_overloads(name, a1, a2, &a2);
+    return *this;
+  }
+
+  /// \see boost::python::class_::def(char const* name, Fn fn, A1 const& a1, A2
+  /// const& a2)
+  template <class Fn, class A1, class A2>
+  self& def(char const* name, Fn fn, A1 const& a1, A2 const& a2) {
+    def_impl(bp::detail::unwrap_wrapper((W*)0), name, fn,
+             bp::detail::def_helper<A1, A2>(a1, a2), &fn);
+
+    return *this;
+  }
+
+  /// \see boost::python::class_::def(char const* name, Fn fn, A1 const& a1, A2
+  /// const& a2, A3 const& a3)
+  template <class Fn, class A1, class A2, class A3>
+  self& def(char const* name, Fn fn, A1 const& a1, A2 const& a2, A3 const& a3) {
+    def_impl(bp::detail::unwrap_wrapper((W*)0), name, fn,
+             bp::detail::def_helper<A1, A2, A3>(a1, a2, a3), &fn);
+
+    return *this;
+  }
+
+ private:
+  /// \see boost::python::class_::def_impl(T*, char const* name, Fn fn, Helper
+  /// const& helper, ...)
+  template <class T, class Fn, class Helper>
+  inline void def_impl(T*, char const* name, Fn fn, Helper const& helper, ...) {
+    bp::objects::add_to_namespace(
+        m_object, name,
+        make_function(fn, helper.policies(), helper.keywords(),
+                      bp::detail::get_signature(fn, (T*)0)),
+        helper.doc());
+
+    def_default(name, fn, helper,
+                boost::mpl::bool_<Helper::has_default_implementation>());
+  }
+
+  /// \see boost::python::class_::def_default(char const* name, Fn, Helper
+  /// const& helper, boost::mpl::bool_<true>)
+  template <class Fn, class Helper>
+  inline void def_default(char const* name, Fn, Helper const& helper,
+                          boost::mpl::bool_<true>) {
+    bp::detail::error::virtual_function_default<
+        W, Fn>::must_be_derived_class_member(helper.default_implementation());
+
+    bp::objects::add_to_namespace(
+        m_object, name,
+        make_function(helper.default_implementation(), helper.policies(),
+                      helper.keywords()));
+  }
+
+  /// \see boost::python::class_::def_default(char const*, Fn, Helper const&,
+  /// boost::mpl::bool_<false>)
+  template <class Fn, class Helper>
+  inline void def_default(char const*, Fn, Helper const&,
+                          boost::mpl::bool_<false>) {}
+
+  /// \see boost::python::class_::def_maybe_overloads(char const* name, SigT
+  /// sig,OverloadsT const& overloads,bp::detail::overloads_base const*)
+  template <class OverloadsT, class SigT>
+  void def_maybe_overloads(char const* name, SigT sig,
+                           OverloadsT const& overloads,
+                           bp::detail::overloads_base const*)
+
+  {
+    bp::detail::define_with_defaults(name, overloads, *this,
+                                     bp::detail::get_signature(sig));
+  }
+
+  /// \see boost::python::class_::def_maybe_overloads(char const* name, Fn fn,
+  /// A1 const& a1, ...)
+  template <class Fn, class A1>
+  void def_maybe_overloads(char const* name, Fn fn, A1 const& a1, ...) {
+    def_impl(bp::detail::unwrap_wrapper((W*)0), name, fn,
+             bp::detail::def_helper<A1>(a1), &fn);
+  }
+
+ private:
+  bp::object m_object;
+};
+
+}  // namespace eigenpy
+
+#endif  // ifndef __eigenpy_registration_class_hpp__
diff --git a/include/eigenpy/std-vector.hpp b/include/eigenpy/std-vector.hpp
index 8d732158a..dc191a9f9 100644
--- a/include/eigenpy/std-vector.hpp
+++ b/include/eigenpy/std-vector.hpp
@@ -352,6 +352,44 @@ struct contains_vector_derived_policies
     return contains_algo<key_type>::run(container, key);
   }
 };
+
+///
+/// \brief Add standard method to a std::vector.
+/// \tparam NoProxy When set to false, the elements will be copied when
+/// returned to Python.
+///
+template <typename Container, bool NoProxy, typename CoVisitor>
+struct ExposeStdMethodToStdVector
+    : public boost::python::def_visitor<
+          ExposeStdMethodToStdVector<Container, NoProxy, CoVisitor> > {
+  typedef StdContainerFromPythonList<Container, NoProxy>
+      FromPythonListConverter;
+
+  ExposeStdMethodToStdVector(const CoVisitor &co_visitor)
+      : m_co_visitor(co_visitor) {}
+
+  template <class Class>
+  void visit(Class &cl) const {
+    cl.def(m_co_visitor)
+        .def("tolist", &FromPythonListConverter::tolist, bp::arg("self"),
+             "Returns the std::vector as a Python list.")
+        .def("reserve", &Container::reserve,
+             (bp::arg("self"), bp::arg("new_cap")),
+             "Increase the capacity of the vector to a value that's greater "
+             "or equal to new_cap.")
+        .def(CopyableVisitor<Container>());
+  }
+
+  const CoVisitor &m_co_visitor;
+};
+
+/// Helper to ease ExposeStdMethodToStdVector construction
+template <typename Container, bool NoProxy, typename CoVisitor>
+static ExposeStdMethodToStdVector<Container, NoProxy, CoVisitor>
+createExposeStdMethodToStdVector(const CoVisitor &co_visitor) {
+  return ExposeStdMethodToStdVector<Container, NoProxy, CoVisitor>(co_visitor);
+}
+
 }  // namespace internal
 
 struct EmptyPythonVisitor
@@ -362,24 +400,16 @@ struct EmptyPythonVisitor
 
 ///
 /// \brief Expose an std::vector from a type given as template argument.
-///
-/// \tparam T Type to expose as std::vector<T>.
-/// \tparam Allocator Type for the Allocator in std::vector<T,Allocator>.
-/// \tparam NoProxy When set to false, the elements will be copied when returned
-/// to Python. \tparam EnableFromPythonListConverter Enables the conversion from
-/// a Python list to a std::vector<T,Allocator>
-///
-/// \sa StdAlignedVectorPythonVisitor
+/// \tparam vector_type std::vector type to expose
+/// \tparam NoProxy When set to false, the elements will be copied when
+/// returned to Python.
+/// \tparam EnableFromPythonListConverter Enables the
+/// conversion from a Python list to a std::vector<T,Allocator>
 ///
 template <class vector_type, bool NoProxy = false,
           bool EnableFromPythonListConverter = true>
-struct StdVectorPythonVisitor
-    : public ::boost::python::vector_indexing_suite<
-          vector_type, NoProxy,
-          internal::contains_vector_derived_policies<vector_type, NoProxy> >,
-      public StdContainerFromPythonList<vector_type, NoProxy> {
+struct StdVectorPythonVisitor {
   typedef typename vector_type::value_type value_type;
-  typedef typename vector_type::allocator_type allocator_type;
   typedef StdContainerFromPythonList<vector_type, NoProxy>
       FromPythonListConverter;
 
@@ -388,40 +418,42 @@ struct StdVectorPythonVisitor
     expose(class_name, doc_string, EmptyPythonVisitor());
   }
 
-  template <typename VisitorDerived>
-  static void expose(
-      const std::string &class_name,
-      const boost::python::def_visitor<VisitorDerived> &visitor) {
+  template <typename Visitor>
+  static void expose(const std::string &class_name, const Visitor &visitor) {
     expose(class_name, "", visitor);
   }
 
-  template <typename VisitorDerived>
-  static void expose(
-      const std::string &class_name, const std::string &doc_string,
-      const boost::python::def_visitor<VisitorDerived> &visitor) {
-    if (!register_symbolic_link_to_registered_type<vector_type>()) {
+  template <typename Visitor>
+  static void expose(const std::string &class_name,
+                     const std::string &doc_string, const Visitor &visitor) {
+    // Apply visitor on already registered type or if type is not already
+    // registered, we define and apply the visitor on it
+    auto add_std_visitor =
+        internal::createExposeStdMethodToStdVector<vector_type, NoProxy>(
+            visitor);
+    if (!register_symbolic_link_to_registered_type<vector_type>(
+            add_std_visitor)) {
       bp::class_<vector_type> cl(class_name.c_str(), doc_string.c_str());
-      cl.def(StdVectorPythonVisitor())
 
-          .def(bp::init<size_t, const value_type &>(
-              bp::args("self", "size", "value"),
-              "Constructor from a given size and a given value."))
+      // Standard vector indexing definition
+      boost::python::vector_indexing_suite<
+          vector_type, NoProxy,
+          internal::contains_vector_derived_policies<vector_type, NoProxy> >
+          vector_indexing;
+
+      cl.def(bp::init<size_t, const value_type &>(
+                 bp::args("self", "size", "value"),
+                 "Constructor from a given size and a given value."))
           .def(bp::init<const vector_type &>(bp::args("self", "other"),
                                              "Copy constructor"))
 
-          .def("tolist", &FromPythonListConverter::tolist, bp::arg("self"),
-               "Returns the std::vector as a Python list.")
-          .def(visitor)
-          .def("reserve", &vector_type::reserve,
-               (bp::arg("self"), bp::arg("new_cap")),
-               "Increase the capacity of the vector to a value that's greater "
-               "or equal to new_cap.")
-          .def_pickle(PickleVector<vector_type>())
-          .def(CopyableVisitor<vector_type>());
-
+          .def(vector_indexing)
+          .def(add_std_visitor)
+          .def_pickle(PickleVector<vector_type>());
+    }
+    if (EnableFromPythonListConverter) {
       // Register conversion
-      if (EnableFromPythonListConverter)
-        FromPythonListConverter::register_converter();
+      FromPythonListConverter::register_converter();
     }
   }
 };
@@ -436,7 +468,7 @@ void exposeStdVectorEigenSpecificType(const char *name) {
   typedef std::vector<MatType, Eigen::aligned_allocator<MatType> > VecMatType;
   std::string full_name = "StdVec_";
   full_name += name;
-  StdVectorPythonVisitor<VecMatType, false>::expose(
+  StdVectorPythonVisitor<VecMatType>::expose(
       full_name.c_str(),
       details::overload_base_get_item_for_std_vector<VecMatType>());
 }


Test patch:
diff --git a/unittest/python/test_std_vector.py b/unittest/python/test_std_vector.py
index 8cd6ab768..b34a32cb9 100644
--- a/unittest/python/test_std_vector.py
+++ b/unittest/python/test_std_vector.py
@@ -14,6 +14,7 @@
 l3.append(np.eye(2))
 l4 = [np.random.randn(3, 3).T for _ in range(3)]
 l4[-1] = l4[-1].T
+l5 = [np.random.randn(2, 2).T for _ in range(3)]
 
 
 def checkAllValues(li1, li2):
@@ -83,3 +84,16 @@ def checkZero(l):
 # vector.setZero(l4)
 # pprint.pprint(list(l4))
 # checkZero(l4)
+
+# Test StdVec_Mat2d that had been registered
+# before calling exposeStdVectorEigenSpecificType
+
+# Test conversion and tolistl5 == l5_copy == l5_py
+l5_copy = std_vector.StdVec_Mat2d(l5)
+l5_py = l5_copy.tolist()
+checkAllValues(l5, l5_copy)
+checkAllValues(l5, l5_py)
+
+# Test mutable __getitem__
+l5[0][:] = 0.0
+assert np.allclose(l5[0], 0.0)
diff --git a/unittest/std_vector.cpp b/unittest/std_vector.cpp
index fa8a95e8a..942d4ae4e 100644
--- a/unittest/std_vector.cpp
+++ b/unittest/std_vector.cpp
@@ -49,4 +49,13 @@ BOOST_PYTHON_MODULE(std_vector) {
   typedef Eigen::Ref<Eigen::MatrixXd> RefXd;
   StdVectorPythonVisitor<std::vector<RefXd>, true>::expose("StdVec_MatRef");
   bp::def("setZero", setZero<Eigen::MatrixXd>, "Sets the coeffs to 0.");
+
+  // Test matrix modification
+  // Mat2d don't have tolist, reserve, mutable __getitem__ and from list
+  // conversion
+  // exposeStdVectorEigenSpecificType must add those methods to StdVec_Mat2d
+  bp::class_<std::vector<Eigen::Matrix2d> >("StdVec_Mat2d")
+      .def(boost::python::vector_indexing_suite<
+           std::vector<Eigen::Matrix2d> >());
+  exposeStdVectorEigenSpecificType<Eigen::Matrix2d>("Mat2d");
 }