From efa8726ff78c4520559d360e82c997d0ce3f8e90 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Fri, 17 Mar 2017 14:51:52 -0300 Subject: [PATCH] Eigen: don't require conformability on length-1 dimensions Fixes #738 The current check for conformability fails when given a 2D, 1xN or Nx1 input to a row-major or column-major, respectively, Eigen::Ref, leading to a copy-required state in the type_caster, but this later failed because the copy was also non-conformable because it had the same shape and strides (because a 1xN or Nx1 is both F and C contiguous). In such cases we can safely ignore the stride on the "1" dimension since it'll never be used: only the "N" dimension stride needs to match the Eigen::Ref stride, which both fixes the non-conformable copy problem, but also avoids a copy entirely as long as the "N" dimension has a compatible stride. --- include/pybind11/eigen.h | 12 +++++++++--- tests/test_eigen.cpp | 16 ++++++++++++++++ tests/test_eigen.py | 10 ++++++++++ 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/include/pybind11/eigen.h b/include/pybind11/eigen.h index 71fc6f1dc..e9c44e565 100644 --- a/include/pybind11/eigen.h +++ b/include/pybind11/eigen.h @@ -79,11 +79,17 @@ template struct EigenConformable { EigenRowMajor ? cstride : rstride /* inner stride */) {} // Vector type: - EigenConformable(EigenIndex r, EigenIndex c, EigenIndex stride) : EigenConformable(r, c, r == 1 ? c*stride : stride, c == 1 ? r : r*stride) {} + EigenConformable(EigenIndex r, EigenIndex c, EigenIndex stride) + : EigenConformable(r, c, r == 1 ? c*stride : stride, c == 1 ? r : r*stride) {} + template bool stride_compatible() const { + // To have compatible strides, we need (on both dimensions) one of fully dynamic strides, + // matching strides, or a dimension size of 1 (in which case the stride value is irrelevant) return - (props::inner_stride == Eigen::Dynamic || props::inner_stride == stride.inner()) && - (props::outer_stride == Eigen::Dynamic || props::outer_stride == stride.outer()); + (props::inner_stride == Eigen::Dynamic || props::inner_stride == stride.inner() || + (EigenRowMajor ? cols : rows) == 1) && + (props::outer_stride == Eigen::Dynamic || props::outer_stride == stride.outer() || + (EigenRowMajor ? rows : cols) == 1); } operator bool() const { return conformable; } }; diff --git a/tests/test_eigen.cpp b/tests/test_eigen.cpp index 98711b08f..eeaceac1e 100644 --- a/tests/test_eigen.cpp +++ b/tests/test_eigen.cpp @@ -50,6 +50,16 @@ void reset_refs() { // Returns element 2,1 from a matrix (used to test copy/nocopy) double get_elem(Eigen::Ref m) { return m(2, 1); }; + +// Returns a matrix with 10*r + 100*c added to each matrix element (to help test that the matrix +// reference is referencing rows/columns correctly). +template Eigen::MatrixXd adjust_matrix(MatrixArgType m) { + Eigen::MatrixXd ret(m); + for (int c = 0; c < m.cols(); c++) for (int r = 0; r < m.rows(); r++) + ret(r, c) += 10*r + 100*c; + return ret; +} + test_initializer eigen([](py::module &m) { typedef Eigen::Matrix FixedMatrixR; typedef Eigen::Matrix FixedMatrixC; @@ -261,4 +271,10 @@ test_initializer eigen([](py::module &m) { // Also test a row-major-only no-copy const ref: m.def("get_elem_rm_nocopy", [](Eigen::Ref> &m) -> long { return m(2, 1); }, py::arg().noconvert()); + + // Issue #738: 1xN or Nx1 2D matrices were neither accepted nor properly copied with an + // incompatible stride value on the length-1 dimension--but that should be allowed (without + // requiring a copy!) because the stride value can be safely ignored on a size-1 dimension. + m.def("iss738_f1", &adjust_matrix &>, py::arg().noconvert()); + m.def("iss738_f2", &adjust_matrix> &>, py::arg().noconvert()); }); diff --git a/tests/test_eigen.py b/tests/test_eigen.py index c57ba2778..f14d62255 100644 --- a/tests/test_eigen.py +++ b/tests/test_eigen.py @@ -604,3 +604,13 @@ def test_sparse_signature(doc): assert doc(sparse_copy_c) == """ sparse_copy_c(arg0: scipy.sparse.csc_matrix[float32]) -> scipy.sparse.csc_matrix[float32] """ # noqa: E501 line too long + + +def test_issue738(): + from pybind11_tests import iss738_f1, iss738_f2 + + assert np.all(iss738_f1(np.array([[1., 2, 3]])) == np.array([[1., 102, 203]])) + assert np.all(iss738_f1(np.array([[1.], [2], [3]])) == np.array([[1.], [12], [23]])) + + assert np.all(iss738_f2(np.array([[1., 2, 3]])) == np.array([[1., 102, 203]])) + assert np.all(iss738_f2(np.array([[1.], [2], [3]])) == np.array([[1.], [12], [23]]))