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.
diff --git a/include/pybind11/eigen.h b/include/pybind11/eigen.h
index 71fc6f1..e9c44e5 100644
--- a/include/pybind11/eigen.h
+++ b/include/pybind11/eigen.h
@@ -79,11 +79,17 @@
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 <typename props> 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 98711b0..eeaceac 100644
--- a/tests/test_eigen.cpp
+++ b/tests/test_eigen.cpp
@@ -50,6 +50,16 @@
// Returns element 2,1 from a matrix (used to test copy/nocopy)
double get_elem(Eigen::Ref<const Eigen::MatrixXd> 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 <typename MatrixArgType> 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<float, 5, 6, Eigen::RowMajor> FixedMatrixR;
typedef Eigen::Matrix<float, 5, 6> FixedMatrixC;
@@ -261,4 +271,10 @@
// Also test a row-major-only no-copy const ref:
m.def("get_elem_rm_nocopy", [](Eigen::Ref<const Eigen::Matrix<long, -1, -1, Eigen::RowMajor>> &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<const Eigen::Ref<const Eigen::MatrixXd> &>, py::arg().noconvert());
+ m.def("iss738_f2", &adjust_matrix<const Eigen::Ref<const Eigen::Matrix<double, -1, -1, Eigen::RowMajor>> &>, py::arg().noconvert());
});
diff --git a/tests/test_eigen.py b/tests/test_eigen.py
index c57ba27..f14d622 100644
--- a/tests/test_eigen.py
+++ b/tests/test_eigen.py
@@ -604,3 +604,13 @@
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]]))