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

Deal with non-contiguous numpy arrays #8

Merged
merged 10 commits into from
Dec 6, 2024
Merged

Deal with non-contiguous numpy arrays #8

merged 10 commits into from
Dec 6, 2024

Conversation

tlpss
Copy link
Collaborator

@tlpss tlpss commented Dec 6, 2024

Use nanobind's built-in Eigen typecasters that relax the row-major requirements for going from python to C++ in the IK functions, which fixes #4.

The problem was that numpy can create views of a matrix that do not correspond to the actual order in which the data is stored (non-contiguous), whereas the C++ type casting code assumed the matrix to be stored in row-major order.
So we were actually computing IK for a different matrix (that was not even necessarily a valid SE3 pose)

A test case has been added to verify non-contiguous numpy arrays provide the same results and to avoid future regression.

This PR also simplifies the codebase as the type casting for numpy arrays to Eigen matrices is now handled by nanobind.

PR also updates the CI workflows, as they were broken

tlpss added 2 commits December 6, 2024 12:12
works only for eigen 3.4 and >=C++11, streaming works for all versions and avoids enforcing specific version of Eigen (which is not availabel in apt)
@tlpss tlpss requested a review from Victorlouisdg December 6, 2024 12:29
Copy link
Owner

@Victorlouisdg Victorlouisdg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 🚀

@tlpss tlpss merged commit 3d9bedd into main Dec 6, 2024
3 of 4 checks passed
@tlpss tlpss deleted the contiguous-fix branch December 9, 2024 15:04
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.

Missed solutions
2 participants