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

Upgrade pybind11 to 2.2+ #18

Closed
qianyizh opened this issue May 10, 2017 · 9 comments
Closed

Upgrade pybind11 to 2.2+ #18

qianyizh opened this issue May 10, 2017 · 9 comments
Assignees

Comments

@qianyizh
Copy link
Collaborator

The current pybind11 is modified from 2.0 dev.
The main modifications are:

  1. Direct (reference) access of Eigen matrices, using this patch Eigen/numpy referencing pybind/pybind11#610
  2. Change of std::vector binding style

The first modification is now merged into pybind11 master branch.
The pybind11 in Open3D should be upgraded to mainstream stable pybind11 version.

@syncle
Copy link
Contributor

syncle commented Jan 18, 2018

I have been spent some spare time for upgrading pybind.
I fixed many issues by referring upgrade to pybind 2.2 except the error below.

/Users/jaesikpa/Research/Open3D/src/Python/py3d_eigen.cpp:90:10: error: no matching constructor for initialization of 'py::buffer_info'
                return py::buffer_info(
                       ^
/Users/jaesikpa/Research/Open3D/src/Python/py3d_eigen.cpp:176:2: note: in instantiation of function template specialization '(anonymous
      namespace)::pybind_eigen_vector_of_vector<Eigen::Matrix<double, 3, 1, 0, 3, 1> >' requested here
        pybind_eigen_vector_of_vector<Eigen::Vector3d>(m, "Vector3dVector",
        ^
/Users/jaesikpa/Research/Open3D/src/External/pybind11/include/pybind11/buffer_info.h:28:5: note: candidate constructor not viable: cannot
      convert initializer list argument to 'detail::any_container<ssize_t>' (aka 'any_container<long>')
    buffer_info(void *ptr, ssize_t itemsize, const std::string &format, ssize_t ndim,
    ^
/Users/jaesikpa/Research/Open3D/src/External/pybind11/include/pybind11/buffer_info.h:39:5: note: candidate constructor template not viable:
      requires 3 arguments, but 6 were provided
    buffer_info(T *ptr, detail::any_container<ssize_t> shape_in, detail::any_container<ssize_t> strides_in)
    ^
/Users/jaesikpa/Research/Open3D/src/External/pybind11/include/pybind11/buffer_info.h:46:5: note: candidate constructor template not viable:
      requires 2 arguments, but 6 were provided
    buffer_info(T *ptr, ssize_t size)
    ^
/Users/jaesikpa/Research/Open3D/src/External/pybind11/include/pybind11/buffer_info.h:83:5: note: candidate constructor not viable: requires 7
      arguments, but 6 were provided
    buffer_info(private_ctr_tag, void *ptr, ssize_t itemsize, const std::string &format, ssize_t ndim,
    ^
/Users/jaesikpa/Research/Open3D/src/External/pybind11/include/pybind11/buffer_info.h:42:5: note: candidate constructor not viable: requires 4
      arguments, but 6 were provided
    buffer_info(void *ptr, ssize_t itemsize, const std::string &format, ssize_t size)
    ^
/Users/jaesikpa/Research/Open3D/src/External/pybind11/include/pybind11/buffer_info.h:49:14: note: candidate constructor not viable: requires at
      most 2 arguments, but 6 were provided
    explicit buffer_info(Py_buffer *view, bool ownview = true)
             ^
/Users/jaesikpa/Research/Open3D/src/External/pybind11/include/pybind11/buffer_info.h:56:5: note: candidate constructor not viable: requires 1
      argument, but 6 were provided
    buffer_info(const buffer_info &) = delete;
    ^
/Users/jaesikpa/Research/Open3D/src/External/pybind11/include/pybind11/buffer_info.h:59:5: note: candidate constructor not viable: requires
      single argument 'other', but 6 arguments were provided
    buffer_info(buffer_info &&other) {
    ^
/Users/jaesikpa/Research/Open3D/src/External/pybind11/include/pybind11/buffer_info.h:26:5: note: candidate constructor not viable: requires 0
      arguments, but 6 were provided
    buffer_info() { }
    ^
:

The compile error comes from the following lines in src/Python/py3d_eigen.cpp:

	auto vec = py::bind_vector_without_repr<std::vector<EigenVector>>(
			m, bind_name, py::buffer_protocol());
	vec.def_buffer([](std::vector<EigenVector> &v) -> py::buffer_info {
		return py::buffer_info(
				v.data(), sizeof(Scalar),
				py::format_descriptor<Scalar>::format(),
				2, {v.size(), EigenVector::RowsAtCompileTime},
				{sizeof(EigenVector), sizeof(Scalar)});
	});

FYI, the following lines are safe:

	auto vec = py::bind_vector<std::vector<Scalar>>(m, bind_name,
			py::buffer_protocol());
	vec.def_buffer([](std::vector<Scalar> &v) -> py::buffer_info {
		return py::buffer_info(
				v.data(), sizeof(Scalar),
				py::format_descriptor<Scalar>::format(),
				1, {v.size()}, {sizeof(Scalar)});
	});

The only difference is to use py::bind_vector_without_repr() instead of py::bind_vector() made by @qianyizh. @qianyizh: Can you tell me how to resolve this issue?

@qianyizh
Copy link
Collaborator Author

Ah, I remember.

I hated the bind_vector function from pybind11, because its __repr__ prints the complete list of all elements. For example, when you call print(pcd.points) it prints the whole list which can be extremely long.

This is not the behavior I wanted, so I copied the pybind11 convenient function bind_vector but removed the __repr__ part, and called that function bind_vector_without_repr.

Can you check:

  1. If pybind11 still have the long list printing __repr__ behavior? If it is fixed, then we just replace all bind_vector_without_repr with bind_vector. The __repr__ part should also be removed from pybind_eigen_vector_of_vector and pybind_eigen_vector_of_matrix.
  2. If it is not fixed. Then copy out the body of the new bind_vector function, paste them into bind_vector_without_repr but remove the __repr__ part.

This should resolve the issue.

@syncle
Copy link
Contributor

syncle commented Jan 30, 2018

Good. I am working on this issue.

@syncle
Copy link
Contributor

syncle commented Jan 31, 2018

I decided to keep bind_vector_without_repr. Actually the error comes from converting
{v.size(), EigenVector::RowsAtCompileTime} into detail::any_container<ssize_t>. It is because pybind2.2 has strict compile time error-check.

This fixes the error:

auto vec = py::bind_vector_without_repr<std::vector<EigenVector>>(
			m, bind_name, py::buffer_protocol());
	vec.def_buffer([](std::vector<EigenVector> &v) -> py::buffer_info {
		size_t rows = EigenVector::RowsAtCompileTime;
		return py::buffer_info(
				v.data(), sizeof(Scalar),
				py::format_descriptor<Scalar>::format(),
				2, {v.size(), rows,
				{sizeof(EigenVector), sizeof(Scalar)});
	});

Now everything works fine. I tested pybind2.2 with the tutorial scripts.

@syncle
Copy link
Contributor

syncle commented Jan 31, 2018

The remaining job is to replace ___init___ for classes. It is not critical issue, but highly recommended by pybind2.2 to avoid runtime warnings.

For example, (from pybind upgrade guide)

// old -- deprecated (runtime warning shown only in debug mode)
py::class<Foo>(m, "Foo")
    .def("__init__", [](Foo &self, ...) {
        new (&self) Foo(...); // uses placement-new
    });

// new
py::class<Foo>(m, "Foo")
    .def(py::init([](...) { // Note: no `self` argument
        return new Foo(...); // return by raw pointer
        // or: return std::make_unique<Foo>(...); // return by holder
        // or: return Foo(...); // return by value (move constructor)
    }));

Now I am trying to replace

	pinhole_intr.def("__init__", [](PinholeCameraIntrinsic &c, int w, int h,
			double fx, double fy, double cx, double cy) {
		new (&c)PinholeCameraIntrinsic(w, h, fx, fy, cx, cy);
	}, "width"_a, "height"_a, "fx"_a, "fy"_a, "cx"_a, "cy"_a);

with

	pinhole_intr.def(py::init([](int w, int h,
	 		double fx, double fy, double cx, double cy) {
	 	return new PinholeCameraIntrinsic(w, h, fx, fy, cx, cy);
	}, "width"_a, "height"_a, "fx"_a, "fy"_a, "cx"_a, "cy"_a));

but this makes compile error. It is because of keyword arguments. I can compile it without keyword arguments. @qianyizh do you have any suggestion for this issue?

@qianyizh
Copy link
Collaborator Author

What is a keyword argument?

@syncle
Copy link
Contributor

syncle commented Jan 31, 2018

It is "width"_a, "height"_a, "fx"_a, "fy"_a, "cx"_a, "cy"_a. If I remove this line, it complies.

@qianyizh
Copy link
Collaborator Author

I think they are the decorations to the function, i.e., when you type help(xxx.__init__), these decorations will pop out as the argument names.

@qianyizh qianyizh reopened this Jan 31, 2018
@syncle syncle mentioned this issue Feb 1, 2018
@qianyizh
Copy link
Collaborator Author

qianyizh commented Feb 1, 2018

Addressed in #127

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

No branches or pull requests

2 participants