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

Have type_caster for Eigen::Map create its own temp array #691

Closed
wants to merge 3 commits into from

Conversation

ManifoldFR
Copy link

@ManifoldFR ManifoldFR commented Aug 20, 2024

This PR:

  • Has type_caster<Eigen::Map<...>> create its own copy of the input NDArray
    • with appropriate strides,
    • pointing to data with same lifetime as the function (using the cleanup_list *cleanup arg in from_python).
    • This will only happen when the inner stride is 1 (maybe we can handle other strides later?), otherwise the caster will return false (Eigen::Ref will then dispatch to the Map type caster with dynamic strides and rely on its constructor creating an internal copy Ref::m_object, which was the previous behaviour)
  • adds a test to test_eigen.cpp|test_eigen.py which tests a function with argument std::vector<Eigen::Ref<const VectorXd>>, which creates a temp copy when an array with the wrong order is given.

Fixes #682

@wjakob
Copy link
Owner

wjakob commented Aug 21, 2024

This is a rather intrusive change given that it's only needed to work around a defect in Eigen that has since been fixed. I'm inclined to rather detect the conditions where a problem could occur at compile time, and to then inform the user that they need to update their Eigen version.

Another opinion would be useful -- do you have thoughts on this @WKarel?

@WKarel
Copy link
Contributor

WKarel commented Aug 21, 2024

#682 (comment)

@ManifoldFR ManifoldFR force-pushed the topic/fix-eigen-temp branch 4 times, most recently from e8679fa to 4f1af28 Compare August 23, 2024 15:54
@ManifoldFR ManifoldFR closed this Aug 29, 2024
@ManifoldFR ManifoldFR deleted the topic/fix-eigen-temp branch September 18, 2024 23:29
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

Successfully merging this pull request may close these issues.

[BUG]: Handling of containers (e.g. std::vector) of Eigen::Ref<const T>
3 participants