Yarp: libYARP_math: replacing GSL with eigen

[randaz81]

Open discussion: we'd like to refactor libYARP_math, deprecating its dependency from GSL (and using Eigen instead). This would solve various issues, many of them are license-related.
@drdanz @lornat75 @traversaro @francesco-romano

Related topics:

786

646

266

[drdanz]

:+1:

[lornat75]

:+1: but we need to talk...

[francesco-romano]

Reporting an "offline" discussion with @traversaro

The following points should be considered during the migration:

1. Different performances between GSL and Eigen (e.g. SVD)
2. GSL dependencies in iCub not coming from yarp::math

Regarding these issues, probably the best thing is a transition period when we have both math libraries and the user can choose which one to compile.

[drdanz]

At the moment we have a release blocking issue because since #797 was merged, YARP_dev became dependent on YARP_math, if it is available.
This means that if we release the binaries building YARP_math, then the YARP_dev dll is GPL as well.
Also we should check the code that uses YARP_math inside YARP_dev, since this part should be GPL and not LGPL.

[pattacini]

@francesco-romano indeed, there is a number of modules in iCub that depend on some specific GSL components I'm not really sure are also provided by Eigen. In the end, Eigen is just for linear algebra, whereas we sometimes need more advanced tools.

However, I don't think it would be a problem to remove GSL as dependency from yarp and keep it for iCub.

[lornat75]

See my comments on #797 and libYARP_math: let's think many many times before introducing such a dependency.

[lornat75]

I agree GSL can be used in iCub for more complex stuff than linear algebra. As far as I can tell in YARP it was used for linear algebra and random number generation, which seem easy to substitute.

[traversaro]

I think this iDynTree header : https://github.com/robotology/idyntree/blob/master/src/yarp/include/iDynTree/yarp/YARPEigenConversions.h can be helpful, it contains inline functions to "view" (i.e. zero copy/overhead) yarp vectors and matrices as Eigen::Map objects:

**
 * Convert a yarp::sig::Vector to a Eigen::Map<Eigen::VectorXd> object
 * @param yarpVector yarp::sig::Vector input
 * @return a Eigen::Map vector that points to the data contained in the yarp vector
 */
inline Eigen::Map<Eigen::VectorXd> toEigen(yarp::sig::Vector & yarpVector)
{
    return Eigen::Map<Eigen::VectorXd>(yarpVector.data(),yarpVector.size());
}

/**
 * Convert a yarp::sig::Matrix to a Eigen::Map< Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic,Eigen::RowMajor> > object
 * @param yarpVector yarp::sig::Matrix input
 * @return a Eigen::Map vector that points to the data contained in the yarp matrix
 */
inline Eigen::Map< Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic,Eigen::RowMajor> > toEigen(yarp::sig::Matrix & yarpMatrix)
{
    return Eigen::Map< Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic,Eigen::RowMajor> >(yarpMatrix.data(),yarpMatrix.rows(),yarpMatrix.cols());
}

/**
 * Convert a const yarp::sig::Vector to a Eigen::Map<const Eigen::VectorXd> object
 * @param yarpVector yarp::sig::Vector input
 * @return a Eigen::Map vector that points to the data contained in the yarp vector
 */
inline Eigen::Map<const Eigen::VectorXd> toEigen(const yarp::sig::Vector & yarpVector)
{
    return Eigen::Map<const Eigen::VectorXd>(yarpVector.data(),yarpVector.size());
}

/**
 * Convert a const yarp::sig::Matrix to a Eigen::Map< const Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic,Eigen::RowMajor> > object
 * @param yarpVector yarp::sig::Matrix input
 * @return a Eigen::Map vector that points to the data contained in the yarp matrix
 */
inline Eigen::Map<const Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic,Eigen::RowMajor> > toEigen(const yarp::sig::Matrix & yarpMatrix)
{
    return Eigen::Map<const Eigen::Matrix<double,Eigen::Dynamic,Eigen::Dynamic,Eigen::RowMajor> >(yarpMatrix.data(),yarpMatrix.rows(),yarpMatrix.cols());
}

[lornat75]

First steps towards this. The PR #858 removes GSL dependency from libYARP_sig. Support for GSL is now in a separate library (libYARP_gsl).

[traversaro]

@lornat75 :
I created a structure for removing GSL dependencies, see https://github.com/robotology/yarp/commit/1e98d29ad0c8ce9f4ca7aecc21be7e5585cd1f18 :

• Create libYARP_eigen (similar to libYARP_gsl) to provide an header-only
  compatibility interface between Eigen and YARP.
  Note that while Eigen headers are directly included in it, being an header-only
  library there is no problem in distributing it: however users that use it need to
  make sure that the Eigen include dirs are found while compiling their own code.
• Added CREATE_LIB_MATH_USING_GSL option dependent on CREATE_LIB_MATH, for now
  enabled by default: by disambling this option, the new Eigen-based backend of
  libYARP_math will be compiled.
• Create stubs for the functions that need to be implemented in src/libYARP_math/src/eigen
• Enable C++11 only for libYARP_math if CREATE_LIB_MATH_USING_GSL is set to OFF,
  to use C++11 random generators in place of GSL's ones. Note that this should not
  create problems even on system in which GCC's C++98 and C++11 ABI are incompatible, because
  no std containers are used in this library. See https://gcc.gnu.org/wiki/Cxx11AbiCompatibility
  for more info.

It should be "sufficient" to fill the stubs in src/libYARP_math/src/eigen to finish.

[traversaro]

About the C++11 part:
I just notice that both Matrix and Vector have a toString method returning a yarp::os::ConstString. However if only libYARP_math is compiled in C++11 and if it never calls toString I think we should be safe.

[traversaro]

@lornat75 To get this merged, we need to (at least temporarly) reintroduce the getGslVector and getGslMatrix methods of libYARP_sig . The problem here is that until we do a new release, the master branch of icub-main needs to compile with both master and devel branches of YARP.

[drdanz]

Or as an alternative, we could just split master/devel on icub-main, but we are waiting for the robot configuration to be split in a separate repository...

[pattacini]

@drdanz yes I'd wait until we'll use robotology/robots-configuration before branching icub-main. This way we'll avoid headaches to productions guys :wink:

[traversaro]

@drdanz I noticed that at this moment CREATE_LIB_MATH_USING_GSL is set to ON by default ( https://github.com/robotology/yarp/blob/devel/cmake/YarpFindDependencies.cmake#L237 ).
Is this intentional? In practice, this means that nobody is actually testing YARP with Eigen.

[drdanz]

@traversaro Actually you did it in d1683780cfd76900b6088a86064c70d8d3f1307b, so you have to tell me whether it is intentional or not :laughing:
I'm ok with changing the default value...

[traversaro]

Argh! This is terrible... it means that basically _nobody_ ever tested Eigen, not even CI. :'''''( .
Opening a PR now.

[traversaro]

Good news... apparently AppVeyor was manually setting CREATE_LIB_MATH_USING_GSL to OFF, see https://github.com/robotology/yarp/blob/devel/scripts/admin/initial-cache.cmake#L5 .

[drdanz]

Good news... apparently AppVeyor was manually setting CREATE_LIB_MATH_USING_GSL to OFF, see https://github.com/robotology/yarp/blob/devel/scripts/admin/initial-cache.cmake#L5 .

Actually you enabled it even before I moved everything to the initial-cache.cmake file, see https://github.com/robotology/yarp/blob/37fdf22270998f08bd3622f56c607109f4173c43/.appveyor.yml#L41 :smile:

[traversaro]

Fixed, as stated in https://github.com/robotology/yarp/issues/266#issuecomment-264462541 .