DM-14864: Make afw pybind11 wrappers compatible with pybind11 2.2 #372
Conversation
Automatic conversion of ``std::shared_ptr<T>`` is not possible when ``T`` is not directly registered with ``py::class_<T>`` (e.g. ``std::shared_ptr<int>`` or ``std::shared_ptr<std::vector<T>>`` are not automatically convertible). In this case all we need to do is get rid of an overload that was not being used. But I also made the existing overload more efficient by using std::move (as suggested by Pim Schellart) and simplified the associated code comments.
python/lsst/afw/math/stack.cc
Outdated
| std::vector<std::shared_ptr<std::vector<PixelT>>> vcopy; | ||
| vcopy.reserve(vectors.size()); | ||
| for (auto const & v : vectors) { | ||
| auto vshared = std::make_shared<std::vector<PixelT>>(std::move(v)); |
There was a problem hiding this comment.
I think this is unsafe, as it will empty the elements of vectors. I realize that the original statisticsStack takes the vectors by modifiable reference, but surely that's not the change they're supposed to make.
There was a problem hiding this comment.
The vectors argument is a copy that pybind11 makes, so I figured it was safe to alter it.
What do you suggest instead? I am reluctant to copy the data again, after pybind11 has already copied it once. @TallJimbo or @pschella may also want to weigh in.
There was a problem hiding this comment.
If it really is a temporary copy than that's fine, but that's not at all obvious to me. I think at the very least a comment would be necessary.
There was a problem hiding this comment.
I think the move is safe, as long as this function is only called by pybind11. I'm not sure you're actually moving, though, given that you're calling std::move on a const reference; that will just cast the object to const T&&, which will invoke the copy constructor. I think changing the loop variable to auto & v should fix that.
All that said, it'd probably be best (and perhaps not even any more disruptive) to just change the main C++ API to take a vector-of-vectors. Using shared_ptr was almost certainly a premature attempt at optimization, and quite possibly a counterproductive one.
There was a problem hiding this comment.
@TallJimbo Sounds good to me. Any objection to having that function return a std::vector instead of a shared_ptrstd::vector as well? That would make it directly compatible with pybind11
There was a problem hiding this comment.
Oh, definitely do that as well. I didn't realize that was also an issue. I'd be surprised if we have anywhere in the stack where using a shared_ptr<vector> is actually a good idea (which is not to say that fixing this would always be easy, but I think it would be here, because there's probably very little C++ code that uses statisticsStack).
There was a problem hiding this comment.
OK. I have made that change (including returning a std::vector. It all seems to work and it's certainly an improvement. A global search strongly suggests it won't break anything but I'll start a Jenkins run to be sure.
python/lsst/afw/math/stack.cc
Outdated
| vcopy.emplace_back(vshared); | ||
| } | ||
| auto result = statisticsStack(vcopy, flags, sctrl, wvector); | ||
| return std::vector<PixelT>(result->begin(), result->end()); |
There was a problem hiding this comment.
This seems unnecessarily convoluted. Why not just vector<PixelT>(*result)? If anything, a constructor that knows it's dealing with a vector may be more optimal than one that works on a generic iterator.
There was a problem hiding this comment.
Or return std::vector<PixelT>(*statisticsStack(vcopy, flags, sctrl, wvector)) to make sure the compiler knows it is a temporary enabling a move? It may or may not do that now too though.
python/lsst/afw/math/stack.cc
Outdated
| vcopy.emplace_back(vshared); | ||
| } | ||
| auto result = statisticsStack(vcopy, flags, sctrl, wvector); | ||
| return std::vector<PixelT>(result->begin(), result->end()); |
There was a problem hiding this comment.
Or return std::vector<PixelT>(*statisticsStack(vcopy, flags, sctrl, wvector)) to make sure the compiler knows it is a temporary enabling a move? It may or may not do that now too though.
declareGetImage should wrap Background::getImage but not BackgroundMI::getImage (since that is inherited). pybind11 2.2 rejected the old code.
Still to fix:
from .image import *
E ImportError: arg(): could not convert default argument 'planeDefs: std::__1::map<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, int, std::__1::less<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const, int> > >' in method '<class 'lsst.afw.image.image.image.MaskX'>.__init__' into a Python object (type not registered yet?)
which appears to be this bit of code:
cls.def(py::init<unsigned int, unsigned int, typename Mask<MaskPixelT>::MaskPlaneDict const &>(),
"width"_a, "height"_a, "planeDefs"_a = typename Mask<MaskPixelT>::MaskPlaneDict());
note that MaskPlaneDict is defined as follows in Mask.h:
typedef std::map<std::string, int> MaskPlaneDict;
Including coord, cameraGeom, display, detection, fits, geom, math, math.detail and also the wrapper in test/. Use addUnderscore=False to SConscript. Use <x>Continued.py and subdirectories where appropriate. Use @continueClass where appropriate. Delete unused wrappers, which had no content and were not built. Change the way the code pulled the contents of lsst.afw.math.detail into lsst.afw.math to avoid the `convolve` module (formerly `_convolve`) shadowing the `convolve` function.
Make it accept a vector of vectors instead of a vector of shared_ptr to vectors, and return a vector instead of a shared pointer to a vector.
pybind11 2.2 appears to be much pickier than 2.1 about requiring wrapper modules to import what they need.
No description provided.