Feature/312 eigen swig int support

[juan-g-bonilla]

• Tickets addressed: Closes Support other scalar types for Eigen with SWIG #312
• Review: By commit
• Merge strategy: Merge (no squash)

Description

SWIG typemaps are how we tell SWIG to translate C++ objects to Python objects and vice-versa. swig_eigen.i is a file that provides these typemaps for Eigen matrices (Eigen::Vector3d, Eigen::MatrixXd, etc.). These objects are translated from and to Python lists (both pre this PR and after).

This PR, however, makes important changes on how this file is organized. %fragments are used to define C++ functions that implement most of the behaviour, which allow us to somewhat reduce codebloat in SWIG wrappers. Typemaps then use these functions to perform their transformations and checks.

In terms of functionality, the most important changes comes from the use of type information to inform transformations. This is, when we want to translate a MatrixX3i, we now from this type that the input/output must have size -1x3 and must be integer typed. Using this information, we can perform size checks on inputs (see checkPyObjectIsMatrixLike), as well as asserting that the types are correct.

Transformation between C and Python types is done in castPyToC and castCToPy. They make use of the type information of the matrix to select the appropriate Python/C API function (PyLong_FromLong, PyFloat_AsDouble, etc.). They also perform sanity checks on the inputs (mainly asserting that integral types are within bounds).

In terms of the typemaps themselves, there are some miscellaneous improvements:

• All operations are slightly faster: the previous typemaps performed several unnecessary copies of the data, which have been refactored out (by building in place instead of through matrixAssemble, using optimal="1", using std::move, etc.)
• Removed a data leak: an old typemap was creating a type on the heap without deallocating it after use.
• Clearer error messages
• Stricter typecheck with appropriate precedence, which enables function overloading*.
• Support for rvalue references (think Eigen::Vector3d&&)

Rotation classes (MRPd, Quaterniond) are handled slightly differently, as they are always double-based and can be set from three different formats (MRP, Quaternion, Rotation Matrix), but are always output as as MRPs. This is legacy behaviour and has remained unchanged. However, they use essentially the same functions as the rest of Eigen types. The main difference is the use of rotationConversion, which will intelligently choose an appropriate transformation between any rotation format to another.

Verification

A new test suit has been included: src\architecture\utilitiesSelfCheck\_UnitTest\test_swigEigen.py. This checks that input-output works as expected, that error messages are thrown as expected, that function overloading work with the intended precedence, and that all qualifier combinations are supported (reference, const reference, const, and rvalues).

Documentation

User-facing interfaces are preserved. No changes necessary.

Function overloading*

Now, it is possible to overload functions with different Eigen types, and SWIG will be able to call the "most specific" function. For a python input [1, 2, 3], the following overloads would be called in this order of precedence:

void foo(std::array<int, 3>)
void foo(Vector3i)
void foo(Vector3d)
void foo(std::vector<int>)
void foo(VectorXi)
void foo(VectorXd)
void foo(MatrixXi)
void foo(MatrixXd)

The order is therefore: fixed-size before dynamically sized, boolean before int before double, and std before Eigen.

Future work

Expand type maps for boolean types and unsigned integer as they are needed. Should be as easy as adding another EIGEN_MAT_WRAP statement to swig_eigen.

[schaubh]

I'm good with these changes. I started to modify existing example script to better make use of this, but it is less trivial than thought. Using getState(), for example, returns a 3x1 dumpy array, where the EigenVector3d2np() method returns a 1x3 list. I would leave it as is for now. We can improve with time if we see a need.

Please do add release notes to this change.

[juan-g-bonilla]

I'm good with these changes. I started to modify existing example script to better make use of this, but it is less trivial than thought. Using getState(), for example, returns a 3x1 dumpy array, where the EigenVector3d2np() method returns a 1x3 list. I would leave it as is for now. We can improve with time if we see a need.

Please do add release notes to this change.

@schaubh As far as I know, getState() always returned a list in the format: [[a], [b], [c]], so there are no changes in how things are output into Python. There are also no significant changes in how things can be input into C++.

If anything, the only change users might experience is that function overloading works a bit different now. Because now we provide better typecheck and precedence, overloads that failed in the past should now work. For example, for the following overload:

void foo(Eigen::MatrixXd);
void foo(Eigen::Vector3d);

In the past, calling foo([1, 2, 3, 4]) might have failed because SWIG tried to use foo(Eigen::Vector3d) instead of foo(Eigen::MatrixXd). Now it should work as expected.

Is this a change worth documenting in the release notes? I'll my opinion about these helper functions in a separate comment.

[schaubh]

.getState() returns [[a],[b],[c]], but the Eigen to NP conversion macro returned [a, b, c]. I was looking at getting rid of the use of the conversion macro and in some places it wasn’t a straight replacement.

If you feel like it, you can updated a few script to use the newer method if you want. I don’t think we have to depreciate the conversion macros yet at this stage.

[juan-g-bonilla]

Moreover, I've taken a look into the unitTestSupport functions and found that:

• unitTestSupport.np2EigenMatrix3d(my_matrix) can be replaced by np.array(my_matrix).reshape(3,3).
• unitTestSupport.np2EigenVectorXd(my_vector) can be replaced by my_vector (handled automatically by the typemaps).
• unitTestSupport.EigenVector3d2np(my_vector) can be replaced by np.array(my_vector).squeeze().

I would be partial to removing all use of these functions in our codebase and use the numpy functions. New users to the code will immediately recognize what these functions are doing (assuming some experience with numpy), but the unitTestSupport are harder to understand just by reading them.

However, would you recommend we deprecate them? It might seem pedantic to users to ask them to change their code just because we are not very happy with some utility functions (which we dont really need to remove in the future).

EDIT: Following @schaubh 's comment above, I think there is no harm done in leaving the utility functions for those users who prefer to use those.

[schaubh]

@juan-g-bonilla , you can remove the use of unitTestSupport.np2EigenVectorXd if you want if your new swig'ing approach will make this not needed, but let's keep this in the unitTestSupport toolbox of methods. I don't see a need here to break other code just for this change.

To be frank, we never required people use unitTestSupport.np2EigenMatrix3d . Each coder is free to make conversion routines they like. However, just speaking for myself, I'll remember how to use unitTestSupport.np2EigenMatrix3d, I'll never remember how to do np.array(my_matrix).reshape(3,3). I see nothing wrong with letting people writing python use their own preference.

As branches are committed and we see people using methods that could be improved, we are free to point these out as options to consider.

[joaogvcarneiro]

Great PR. I can't give feedback on the SWIG implementation since I'm unfamiliar with it, but I appreciate the introduction of tests of Python code.

[juan-g-bonilla]

@juan-g-bonilla , you can remove the use of unitTestSupport.np2EigenVectorXd if you want if your new swig'ing approach will make this not needed, but let's keep this in the unitTestSupport toolbox of methods. I don't see a need here to break other code just for this change.

To be frank, we never required people use unitTestSupport.np2EigenMatrix3d . Each coder is free to make conversion routines they like. However, just speaking for myself, I'll remember how to use unitTestSupport.np2EigenMatrix3d, I'll never remember how to do np.array(my_matrix).reshape(3,3). I see nothing wrong with letting people writing python use their own preference.

As branches are committed and we see people using methods that could be improved, we are free to point these out as options to consider.

Good points. I will undo the last two commits and just remove the use of unitTestSupport.np2EigenVectorXd.