DM-15126: Make meas_* compatible with pybind11 2.2 (as well as 2.1) #123
Conversation
r-owen
changed the title
python/lsst/meas/base/algorithm.cc
Outdated
| @@ -48,8 +47,6 @@ PYBIND11_PLUGIN(algorithm) { | |||
| mod, "SimpleAlgorithm", py::multiple_inheritance()); | |||
|
|
|||
| /* Members */ | |||
| python::declareAlgorithm<SingleFrameAlgorithm>(clsSingleFrameAlgorithm); | |||
|
|
|||
There was a problem hiding this comment.
If we can no longer call declareAlgorithm on an abstract class (and the inability to wrap abstract methods seems like a step backwards 😞), consider removing abstract class support from declareAlgorithm.
Based on discussion with @r-owen it seems the real problem is not that SingleFrameAlgorithm is abstract, but that BaseAlgorithm is a virtual base of SingleFrameAlgorithm. However, the pybind11 docs don't say anything forbidding the wrapping of classes with virtual bases. I suggest not modifying declareAlgorithm until we know for sure whether the new behavior is intentional or a bug.
python/lsst/meas/base/algorithm.cc
Outdated
| @@ -31,6 +31,7 @@ using namespace pybind11::literals; | |||
| namespace lsst { | |||
| namespace meas { | |||
| namespace base { | |||
| namespace { | |||
|
|
|||
| PYBIND11_PLUGIN(algorithm) { | |||
There was a problem hiding this comment.
This is different from the examples in the style guide. Are we now required to put PYBIND11_PLUGIN in an anonymous namespace? I can't find any mention of anonymous namespaces in the pybind11 docs...
There was a problem hiding this comment.
I'm quite surprised. I thought our standard called for putting everything in anonymous namespace -- it's certainly easier that way, as then one can add functions as needed without worrying about it. But I have redone my changes to follow the style guide.
r-owen
force-pushed
the
tickets/DM-15126
branch
2 times, most recently
from
68e89e1
to
f5d62e1
Compare
Standardize the use of anonymous namespace in our pybind11 wrappers and run clang-format on our wrappers
which defines it, instead of subclass SingleFrameAlgorithm, which does not. Also rearrange the declarations for clarity.
|
@kfindeisen based on your comments on slack I found and fixed the underlying error in |
No description provided.