- edited description
Is it ok to return by const reference in pybind11 wrapping?
While working on pull request #424 I had some problems with methods that return a const reference.
See e.g. the following code
import numpy
from dolfin import *
## Copied unit/mesh/test_mesh_editor.py ##
# Create mesh object and open editor
mesh = Mesh()
editor = MeshEditor()
editor.open(mesh, "triangle", 2, 2)
editor.init_vertices(3) # test both versions of interface
editor.init_vertices_global(3, 3)
editor.init_cells(1) # test both versions of interface
editor.init_cells_global(1, 1)
# Add vertices
editor.add_vertex(0, numpy.array([0.0, 0.0], dtype='float'))
editor.add_vertex(1, numpy.array([1.0, 0.0], dtype='float'))
editor.add_vertex(2, numpy.array([0.0, 1.0], dtype='float'))
# Add cell
editor.add_cell(0, numpy.array([0, 1, 2], dtype='uint'))
# Close editor
editor.close()
## ------------------------------------- ###
# Try to init facets global indices
mesh_topology = mesh.topology()
mesh_topology.init_global_indices(1, 3)
assert mesh_topology.have_global_indices(1)
assert mesh.topology().have_global_indices(1)
The assert at the last line fails, while in swig changes were retained (as if the output of mesh.topology() was not marked as const). The aim is obviously to change the underlying topology object.
To make things even weirder
>>> from dolfin import *
>>> mesh = UnitSquareMesh(3,3)
>>> id(mesh.topology())
139735462497280
>>> id(mesh.topology())
139735462497336
>>> id(mesh.topology())
139735462497280
>>> id(mesh.topology())
139735462497336
>>> id(mesh.topology())
139735462497280
>>> id(mesh.topology())
139735462497336
Comments (19)
-
reporter -
reporter - edited description
-
>>> id(mesh.topology()) 139735462497336 >>> id(mesh.topology()) 139735462497280
What's wrong about it? It's a shared pointer (referencing the topology object) which is created and destroyed.
-
reporter You are probably right, I just thought it was weird that the first and third id were the same, as well as the second and fourth one, as if there were two copies lying around. Scratch that; the issue in the first part of the post remains.
-
You're right, the topology is returned by copy. That's horrible.
-
pybind11 is very simple. It very simply allows to write correct code with unintended behaviour.
There should be tests for things like this. I can think of simple checks like
assert mesh.topology().id() == mesh.topology().id()
-
But what should be done? From the pybind11 side it defaults to 'safe' options, which in the case of a reference is to return a copy, but we can specify the return value policy (see http://pybind11.readthedocs.io/en/stable/advanced/functions.html?highlight=return%20value#return-value-policies).
return_value_policy::reference_internal
may be appropriate.Other option is to make the topology object a shared pointer.
-
reporter So, is it best to return a std::shared_ptr<> rather than a reference for methods that would need their output to go through pybind11? Are there other options?
[apologies Garth, posted at the same time as you]
-
@francesco_ballarin See http://pybind11.readthedocs.io/en/stable/advanced/functions.html?highlight=return%20value#return-value-policies. A nice thing about pybind11 is that it gives a lot of control over the return policy.
-
But what should be done?
Indeed, the reference has to be returned:
diff --git a/python/src/mesh.cpp b/python/src/mesh.cpp index daae2dc6e..9bfc3f35e 100644 --- a/python/src/mesh.cpp +++ b/python/src/mesh.cpp @@ -106,7 +106,8 @@ namespace dolfin_wrappers .def("have_shared_entities", &dolfin::MeshTopology::have_shared_entities) .def("shared_entities", (std::map<std::int32_t, std::set<unsigned int> >&(dolfin::MeshTopology::*)(unsigned int)) - &dolfin::MeshTopology::shared_entities); + &dolfin::MeshTopology::shared_entities) + .def("str", &dolfin::MeshTopology::str); // dolfin::Mesh py::class_<dolfin::Mesh, std::shared_ptr<dolfin::Mesh>>(m, "Mesh", @@ -179,8 +180,9 @@ namespace dolfin_wrappers .def("smooth_boundary", &dolfin::Mesh::smooth_boundary) .def("snap_boundary", &dolfin::Mesh::snap_boundary, py::arg("subdomain"), py::arg("harmonic_smoothing")=true) - .def("topology", (const dolfin::MeshTopology& (dolfin::Mesh::*)() const) - &dolfin::Mesh::topology, "Mesh topology") + .def("topology", (dolfin::MeshTopology& (dolfin::Mesh::*)()) + &dolfin::Mesh::topology, "Mesh topology", + py::return_value_policy::reference) .def("translate", &dolfin::Mesh::translate) .def("type", (const dolfin::CellType& (dolfin::Mesh::*)() const) &dolfin::Mesh::type, py::return_value_policy::reference)
And this behaviour should be reviewed for other accessors and preferably tested.
Variable::id()
provides a simple way how to test this. -
Oh word,
reference_internal
is appropriate here, otherwisemesh.topology()
becomes dangling whenmesh
is destroyed.It seems that we could be more Pythonic and provide topology as a read/write property
mesh.topology
instead of using getter/setter methods which are more appropriate for C++ side. It could be a right time to change this now if we want but it would be a lot of work. -
@blechta Did you push a fix for this issue?
-
Fix in pull request #429.
-
reporter Thanks. Is there an easy way to double check if other methods currently wrapped by pybind11 return by reference and thus would need the appropriate py::return_value_policy?
-
- marked as major
-
- changed milestone to 2018.1
-
Wait, I have a fix or this to be merged before the release. Now testing in nexts.
-
-
- changed status to resolved
- Log in to comment