Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce consistency between input numpy layout and expected Eigen object memory layout #330

Open
ManifoldFR opened this issue Nov 29, 2022 · 0 comments

Comments

@ManifoldFR
Copy link
Member

ManifoldFR commented Nov 29, 2022

Consider the following example:

template <typename MatType>
const Eigen::Ref<const MatType> asConstRef(Eigen::Ref<const MatType> mat) {
  doSomething(mat);
  return Eigen::Ref<const MatType>(mat);
}

// in BOOST_PYTHON_MODULE
  bp::def("asRef", asConstRef<Eigen::MatrixXd>);   // notice this is the *ColMajor* matrix
//

This will work:

A = np.eye(2, order="F")
aref = asConstRef(A)

assert np.array_equal(A, aref)
assert not aref.flags.owndata
assert not aref.flags.writeable

This will not:

A = np.eye(2, order="C")  # "C" is the numpy default
aref = asConstRef(A)

assert np.array_equal(A, aref)  # fails
assert not aref.flags.owndata  # ok
assert not aref.flags.writeable  # ok

After discussing with @jcarpent we came to the conclusion that the return statement in C++, due to how EigenAllocator<Eigen::Ref<cv MatType>, ...> is wired up, returns something pointing to the wrong memory - actually, to temporary C++ memory created by EigenAllocator and of which an Eigen::Ref is given to asConstRef. This temporary memory is a (col-major) Eigen::MatrixXd created to host a copy of the (row-major) Numpy input, and destroyed in the destructor of the rvalue_from_python_data handler before the function returns. What is inside the returned aref view is random memory.

This was discovered while working in PR #325.

Two possibilities:

  • redefine the Eigen::Ref types to have dynamic stride as to allow mapping from column major to row major; rework the EigenAllocator
  • disallow the above.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant