SCIInstitute / ShapeWorks

ShapeWorks
http://sciinstitute.github.io/ShapeWorks/
Other
103 stars 32 forks source link

Use Eigen matrix library for everything matrix-related #1184

Open cchriste opened 3 years ago

cchriste commented 3 years ago

There seems to be continuing effort to do unnecessary work instead of just doing this. This class has all the desired APIs (#1054) in both Python and C++, and is directly exportable to Python, so rather than a huge amount of work to figure out how to export and/or bind all the vnl matrix calls to python classes that will just be removed (#830), just go straight to the solution and use Eigen::Matrix. This will also do everything necessary but swap out the calls for #1172.

Eigen c++ matrix library, already in use in many places" https://eigen.tuxfamily.org/index.php?title=Main_Page

To pass numpy matrices between c++ and python: https://pybind11.readthedocs.io/en/stable/advanced/cast/eigen.html

To perform matrix operations in python, use numpy.matrix, numpy.linalg, etc.: https://numpy.org/doc/stable/reference/generated/numpy.matrix.html https://numpy.org/doc/stable/reference/generated/numpy.linalg.eig.html

cchriste commented 3 years ago

Having passed a lot more Eigen matrices and numpy arrays, it's not clear this will solve these issues. I tried quite a bit on the #1495 and the geodesicDistance PR, as well as the Image.toArray and Image.init, and it turned out that the (non-modifiable?) defaults for Eigen matrices did not suffice. I might still be wrong, so still worth taking a close look.

cchriste commented 2 years ago

This may be complete. We are passing Eigen matrices, albeit more manually than desired, but with nice helper functions to wrap these steps (primarily related to safe transfer of ownership), everywhere they are used. #1174 might still need fixing (converting vnl_vectors to use Eigen matrices), but it's not the same as this issue, and once converted they'll just use the same techniques demonstrated in the Python bindings for sharing Eigen matrices.

cchriste commented 2 years ago

All the functionality for doing this between C++ and Python is in place. This issue is to ensure Eigen::Matrix is also used everywhere internally. Help is welcome to identify places it's still not used.