Fix pybind11 calling overhead #116
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Maybe try using nanobind. Nanobind is created by the same author as pybind11, which we currently use without any fancy features. Therefore, nanobind should be a drop-in replacement that provides the same core functionality. Porting guide: |
sarlinpe
reviewed
Oct 23, 2024
pybind/pybind11_extension.h
Outdated
Comment on lines
51
to
86
| // Autocast from numpy.ndarray to std::vector<Eigen::Vector> | ||
| template <typename Scalar, int Size> | ||
| struct type_caster<std::vector<Eigen::Matrix<Scalar, Size, 1>>> { | ||
| public: | ||
| using MatrixType = | ||
| typename Eigen::Matrix<Scalar, Eigen::Dynamic, Size, Eigen::RowMajor>; | ||
| using VectorType = typename Eigen::Matrix<Scalar, Size, 1>; | ||
| using props = EigenProps<MatrixType>; | ||
| PYBIND11_TYPE_CASTER(std::vector<VectorType>, props::descriptor); | ||
|
|
||
| bool load(handle src, bool) { | ||
| const auto buf = array::ensure(src); | ||
| if (!buf) { | ||
| return false; | ||
| } | ||
| const buffer_info info = buf.request(); | ||
| if (info.ndim != 2 || info.shape[1] != Size) { | ||
| return false; | ||
| } | ||
| const size_t num_elements = info.shape[0]; | ||
| value.resize(num_elements); | ||
| const auto& mat = src.cast<Eigen::Ref<const MatrixType>>(); | ||
| Eigen::Map<MatrixType>( | ||
| reinterpret_cast<Scalar*>(value.data()), num_elements, Size) = mat; | ||
| return true; | ||
| } | ||
|
|
||
| static handle cast(const std::vector<VectorType>& vec, | ||
| return_value_policy /* policy */, | ||
| handle h) { | ||
| Eigen::Map<const MatrixType> mat( | ||
| reinterpret_cast<const Scalar*>(vec.data()), vec.size(), Size); | ||
| return type_caster<Eigen::Map<const MatrixType>>::cast( | ||
| mat, return_value_policy::copy, h); | ||
| } | ||
| }; |
There was a problem hiding this comment.
You only need this code block, which should indeed speed up all conversions to std::vector<Eigen::Vector>. I'd appreciate due credit being given since it is copied from COLMAP and I wrote it :)
There was a problem hiding this comment.
Perfect, thanks! Of course, I'll add the proper attribution before merging it :)
sarlinpe
approved these changes
Nov 23, 2024
pablospe
approved these changes
Nov 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Playing around a bit with dense matches it seems that the pybind is quite slow. Adding this file (stolen from pycolmap) seem to improve runtime a bit. Without it there seems to be a 4-6ms overhead calling from python with 10k matches.
Maybe @sarlinpe or @Phil26AT have some more insight. In particular if anything needs to be adapted, or could be removed.