DM-15187: Modernize sphgeom pickle support for pybind11 2. #17
Conversation
| }); | ||
| cls.def(py::pickle( | ||
| [](const Box &self) { return python::encode(self); }, | ||
| [](py::bytes bytes) { return decode(bytes).release(); })); |
There was a problem hiding this comment.
Are you sure the explicit release is needed?
There was a problem hiding this comment.
Good question, but I was hoping you could tell me! I copied the code from the existing wrappers for decode (e.g. in box.cc, ellipse.cc, etc.). However, I did try building without .release() in the pickle wrapper (but not in the decode wrapper) and I didn't see any difference -- the unit tests passed. I figured it was safer to go with what was there, but it might be code that was never needed, or was only needed for older versions of pybind11. What do you think?
There was a problem hiding this comment.
http://pybind11.readthedocs.io/en/stable/advanced/smart_ptrs.html this implies that the release is not needed (and perhaps not wanted).
There was a problem hiding this comment.
Based on more tests I see the following:
- If I remove
.release()fromcloneordecodeinregion.ccI get segfaults - If I remove
.release()fromdecodeinbox.ccI also get segfaults - If I remove
.release()from the pickle wrapper inbox.ccI do not get segfaults
Do you know what's going on? The py::class_ templates do list std::shared_ptr<X> for each type X, as usual for LSST. Does this cause trouble for functions that return a unique pointer to X? Why does the pickle support seem to work without release() when the identical decode does not?
Barring an explanation and a commensurate desire to change something, I propose to leave the .release() in everywhere.
There was a problem hiding this comment.
I agree with the proposed solution. Will have to look into this later. Perhaps file a new mini ticket? The unique_ptr -> shared_ptr shouldn't matter.
python/lsst/sphgeom/unitVector3d.cc
Outdated
| cls.def(py::pickle([](UnitVector3d const &self) { return py::make_tuple(self.x(), self.y(), self.z()); }, | ||
| [](py::tuple t) { | ||
| if (t.size() != 3) { | ||
| throw std::runtime_error("Invalid state!"); |
There was a problem hiding this comment.
You may want to be a bit more explicit.
python/lsst/sphgeom/unitVector3d.cc
Outdated
| throw std::runtime_error("Invalid state!"); | ||
| } | ||
| return new UnitVector3d(UnitVector3d::fromNormalized( | ||
| t[0].cast<double>(), t[1].cast<double>(), t[2].cast<double>())); |
There was a problem hiding this comment.
I know this was already there, but why a heap one?
There was a problem hiding this comment.
I'm not sure what you are asking. Are you asking why this code returns a pointer instead of a vector? If so, my guess is because UnitVector3d has no move constructor -- though I don't know if the compiler would still manage to move the data in this situation. In any case...pybind11 pickle docs show returning the new object by bare pointer, so it is a reasonable option. If you think something else would be better let me know.
Use pybind11::pickle instead of defining `__getstate__` and `__setstate__` directly, as recommended for pybind11 2.2. This eliminates compiler warnings.
| }); | ||
| cls.def(py::pickle( | ||
| [](const Box &self) { return python::encode(self); }, | ||
| [](py::bytes bytes) { return decode(bytes).release(); })); |
There was a problem hiding this comment.
I agree with the proposed solution. Will have to look into this later. Perhaps file a new mini ticket? The unique_ptr -> shared_ptr shouldn't matter.
No description provided.