Repository: stack-of-tasks/eigenpy (language: python)

Issue:
[2.8.0] bug for Quaternion constructor for strided data
I noticed a bug when feeding `eigenpy.Quaternion` with a slice of a bigger 2d `numpy` array.

Instead of using the coefficients of the slice to init the Quaternion, it seems `eigenpy` picks those of the contiguous array starting at the first coefficient location (the example below will make this sentence much clearer).

## Reproduction instruction

### Clean conda env

spawn a `conda` docker container
```sh
docker run -it continuumio/anaconda3 /bin/bash
```
Inside it
```sh
conda create --name snow python=3.8 numpy ipython
conda activate snow
conda install -c conda-forge eigenpy=2.8.0
ipython
```
In ipython
```py
import eigenpy
import numpy as np

# we (normally) only care about the first column
position = np.array(
    [[0, 0.2, 0.3, 0.4],
     [0,   1,   0,   0],
     [0,   0,   0,   0],
     [1,   0,   0,   0]]
)
print("total array")
print(position)
data_strided = position[:, 0]
print("quaternion data")
print(data_strided)


print("Strided data")
print(data_strided.flags)
# C_CONTIGUOUS : False
# F_CONTIGUOUS : False
# OWNDATA : False
# WRITEABLE : True
# ALIGNED : True
# WRITEBACKIFCOPY : False
q_strided = eigenpy.Quaternion(data_strided)
print(q_strided)
# (x,y,z,w) =   0 0.2 0.3 0.4

print("Contiguous data")
data_compact = np.ascontiguousarray(data_strided)
print(data_compact.flags)
# C_CONTIGUOUS : True
# F_CONTIGUOUS : True
# OWNDATA : True
# WRITEABLE : True
# ALIGNED : True
# WRITEBACKIFCOPY : False
q_compact = eigenpy.Quaternion(data_compact)
print(q_compact)
# (x,y,z,w) = 0 0 0 1
```

looks like we ended up using the coefficient of the first row, though we specified the column as input.

playing with different other versions:
- `2.6.7` works
- `2.7.0` breaks
- `2.7.14` breaks



Discussion / hints:

Fix patch:
diff --git a/include/eigenpy/eigen-allocator.hpp b/include/eigenpy/eigen-allocator.hpp
index 2ca889dbe..650da8dd9 100644
--- a/include/eigenpy/eigen-allocator.hpp
+++ b/include/eigenpy/eigen-allocator.hpp
@@ -245,7 +245,8 @@ inline bool is_arr_layout_compatible_with_mat_type(PyArrayObject *pyArray) {
   bool is_array_F_cont = PyArray_IS_F_CONTIGUOUS(pyArray);
   return (MatType::IsRowMajor && is_array_C_cont) ||
          (!MatType::IsRowMajor && is_array_F_cont) ||
-         MatType::IsVectorAtCompileTime;
+         (MatType::IsVectorAtCompileTime &&
+          (is_array_C_cont || is_array_F_cont));
 }
 
 template <typename MatType, int Options, typename Stride>
diff --git a/include/eigenpy/numpy-map.hpp b/include/eigenpy/numpy-map.hpp
index 51af11b1c..ae6b1e693 100644
--- a/include/eigenpy/numpy-map.hpp
+++ b/include/eigenpy/numpy-map.hpp
@@ -161,7 +161,6 @@ struct NumpyMapTraits<MatType, InputScalar, AlignmentValue, Stride, true> {
     const int R = (int)PyArray_DIMS(pyArray)[rowMajor];
     const long int itemsize = PyArray_ITEMSIZE(pyArray);
     const int stride = (int)PyArray_STRIDE(pyArray, rowMajor) / (int)itemsize;
-    ;
 
     if ((MatType::MaxSizeAtCompileTime != R) &&
         (MatType::MaxSizeAtCompileTime != Eigen::Dynamic)) {
@@ -171,6 +170,8 @@ struct NumpyMapTraits<MatType, InputScalar, AlignmentValue, Stride, true> {
 
     InputScalar* pyData = reinterpret_cast<InputScalar*>(PyArray_DATA(pyArray));
 
+    assert(Stride(stride).inner() == stride &&
+           "Stride should be a dynamic stride");
     return EigenMap(pyData, R, Stride(stride));
   }
 };


Test patch:
diff --git a/unittest/eigen_ref.cpp b/unittest/eigen_ref.cpp
index 9c334d10b..6934f7157 100644
--- a/unittest/eigen_ref.cpp
+++ b/unittest/eigen_ref.cpp
@@ -30,6 +30,12 @@ void setOnes(Eigen::Ref<MatType> mat) {
   mat.setOnes();
 }
 
+template <typename VecType>
+VecType copyVectorFromConstRef(const Eigen::Ref<const VecType> vec) {
+  std::cout << "copyVectorFromConstRef::vec: " << vec.transpose() << std::endl;
+  return VecType(vec);
+}
+
 template <typename MatType>
 Eigen::Ref<MatType> getBlock(Eigen::Ref<MatType> mat, Eigen::DenseIndex i,
                              Eigen::DenseIndex j, Eigen::DenseIndex n,
@@ -120,6 +126,9 @@ BOOST_PYTHON_MODULE(eigen_ref) {
   bp::def("getBlock", &getBlock<MatrixXd>);
   bp::def("editBlock", &editBlock<MatrixXd>);
 
+  bp::def("copyVectorFromConstRef", &copyVectorFromConstRef<VectorXd>);
+  bp::def("copyRowVectorFromConstRef", &copyVectorFromConstRef<RowVectorXd>);
+
   bp::class_<modify_block_wrap, boost::noncopyable>("modify_block",
                                                     bp::init<>())
       .def_readonly("J", &modify_block::J)
diff --git a/unittest/python/test_eigen_ref.py b/unittest/python/test_eigen_ref.py
index fe43e501f..8d466f1d8 100644
--- a/unittest/python/test_eigen_ref.py
+++ b/unittest/python/test_eigen_ref.py
@@ -9,6 +9,8 @@
     editBlock,
     modify_block,
     has_ref_member,
+    copyVectorFromConstRef,
+    copyRowVectorFromConstRef,
 )
 
 
@@ -44,6 +46,18 @@ def test_create_ref_to_static(mat):
     assert np.array_equal(A_ref, A_ref2)
 
 
+def test_read_block():
+    data = np.array([[0, 0.2, 0.3, 0.4], [0, 1, 0, 0], [0, 0, 0, 0], [1, 0, 0, 0]])
+
+    data_strided = data[:, 0]
+
+    data_strided_copy = copyVectorFromConstRef(data_strided)
+    assert np.all(data_strided == data_strided_copy)
+
+    data_strided_copy = copyRowVectorFromConstRef(data_strided)
+    assert np.all(data_strided == data_strided_copy)
+
+
 def test_create_ref(mat):
     print("[asRef(mat)]")
     ref = asRef(mat)
@@ -116,6 +130,7 @@ def do_test(mat):
     test_create_const_ref(mat)
     test_create_ref(mat)
     test_edit_block(rows, cols)
+    test_read_block()
     print("=" * 10)