Operators should not be in the yarp::math namespace

[drdanz]

Vector and Matrix operators are defined inside the yarp::math namespace.

yarp/src/libYARP_math/include/yarp/math/Math.h

Lines 16 to 27 in 29e9108

	namespace yarp
	{
	/**
	* Mathematical operations.
	*/
	namespace math
	{
	/**
	* Addition operator between a scalar and a vector (defined in Math.h).
	* Sum the scalar to all the elements of the vector.
	*/
	YARP_math_API yarp::sig::Vector operator+(const yarp::sig::Vector &a, const double &s);

This means that, in order to perform an operation (for example Vector b = a + s;), the user has to add using namespace yarp::math; or to call the operator explicitly, i.e. (I think) something like this: Vector b = yarp::math::operator+(a, s);
The first option is considered bad practice, and the second option is just wrong.

I think we could just move them outside of the namespace, the only effect is if someone is using the second syntax, but I don't think anyone would be doing that...

[drdanz]

By the way, why are they in math? I thought they were using gsl/eigen, but most of them are not...

yarp/src/libYARP_math/src/math.cpp

Lines 24 to 148 in 29e9108

	Vector yarp::math::operator+(const Vector &a, const double &s)
	{
	Vector ret(a);
	return ret+=s;
	}
	Vector yarp::math::operator+(const double &s, const Vector &a)
	{
	return a+s;
	}
	Vector& yarp::math::operator+=(Vector &a, const double &s)
	{
	size_t l = a.size();
	for(size_t i=0; i<l; i++)
	a[i] += s;
	return a;
	}
	Vector yarp::math::operator+(const Vector &a, const Vector &b)
	{
	Vector ret(a);
	return ret+=b;
	}
	Vector& yarp::math::operator+=(Vector &a, const Vector &b)
	{
	size_t s=a.size();
	yAssert(s==b.size());
	for (size_t k=0; k<s;k++)
	a[k]+=b[k];
	return a;
	}
	Matrix yarp::math::operator+(const Matrix &a, const Matrix &b)
	{
	Matrix ret(a);
	return ret+=b;
	}
	Matrix& yarp::math::operator+=(Matrix &a, const Matrix &b)
	{
	int n=a.cols();
	int m=a.rows();
	yAssert(m==b.rows() && n==b.cols());
	for (int r=0; r<m;r++)
	for (int c=0; c<n;c++)
	a(r,c)+=b(r,c);
	return a;
	}
	Vector yarp::math::operator-(const Vector &a, const double &s)
	{
	Vector ret(a);
	return ret-=s;
	}
	Vector yarp::math::operator-(const double &s, const Vector &a)
	{
	size_t l = a.size();
	Vector ret(l);
	for(size_t i=0; i<l; i++)
	ret[i] = s-a[i];
	return ret;
	}
	Vector& yarp::math::operator-=(Vector &a, const double &s)
	{
	size_t l = a.size();
	for(size_t i=0; i<l; i++)
	a[i] -= s;
	return a;
	}
	Vector yarp::math::operator-(const Vector &a, const Vector &b)
	{
	Vector ret(a);
	return ret-=b;
	}
	Vector& yarp::math::operator-=(Vector &a, const Vector &b)
	{
	size_t s=a.size();
	yAssert(s==b.size());
	for (size_t k=0; k<s;k++)
	a[k]-=b[k];
	return a;
	}
	Matrix yarp::math::operator-(const Matrix &a, const Matrix &b)
	{
	Matrix ret(a);
	return ret-=b;
	}
	Matrix& yarp::math::operator-=(Matrix &a, const Matrix &b)
	{
	int n=a.cols();
	int m=a.rows();
	yAssert(m==b.rows());
	yAssert(n==b.cols());
	for (int r=0; r<m;r++)
	for (int c=0; c<n;c++)
	a(r,c)-=b(r,c);
	return a;
	}
	Vector yarp::math::operator*(double k, const Vector &b)
	{
	return operator*(b,k);
	}
	Vector yarp::math::operator*(const Vector &a, double k)
	{
	Vector ret(a);
	return ret*=k;
	}
	Vector& yarp::math::operator*=(Vector &a, double k)
	{
	int size=a.size();
	for(int i=0;i<size;i++)
	a[i]*=k;
	return a;
	}

[lornat75]

I am ok moving the operators outside yarp::math. I would keep however the current implementation when it relies on linear algebra lib, and keep them on libYARP_math.

[claudiofantacci]

I would be super happy to see math operations moved outside yarp::math namespace.

We could take the chance to update the implementation (if possible, but I don't see any problem in doing that) of those operations that are not implemented with Eigen.

[jgvictores]

Has certain implications on #1603 and #1559.

Moreover, where do they go? yarp::sig:: ...maybe simply yarp::?

Edit: a yarp::sig::math could look nice, but would end up in same situation.

[lornat75]

My understanding was that the operators would be moved outside any namespace. From what I know it is the only way to solve the problem raised by @drdanz.

[pattacini]

@drdanz said

I think we could just move them outside of the namespace, the only effect is if someone is using the second syntax (i.e. Vector b = yarp::math::operator+(a, s);) but I don't think anyone would be doing that...

I'm not aware of anyone resorting to the second syntax, but, actually, there's the risk it is being used in header files, that is where declaring using namespace yarp::math represents even a worse practice. Then, using math in header files can be considered questionable, I know, but I just wanted to point out this possible use case.

[PeterBowman]

the user has to add using namespace yarp::math; or to call the operator explicitly

Somewhere between:

#include <iostream>

#include <yarp/sig/Vector.h>
#include <yarp/math/Math.h>

int main()
{
    yarp::sig::Vector v1, v2;
    v1.push_back(1);
    v1.push_back(2);
    v2.push_back(3);
    v2.push_back(4);
    
    using yarp::math::operator+;
    yarp::sig::Vector v3 = v1 + v2;
    
    std::cout << "[" << v3[0] << ", " << v3[1] << "]" << std::endl;
    
    return 0;
}

Output: [4, 6].

[drdanz]

[...]

using yarp::math::operator+;

Right, but still ugly...

[jgvictores]

My understanding was that the operators would be moved outside any namespace.

After a close encounter with a Pythonic error, where at least two different math implementations exist, and forcing one of them into the "global namespace" makes the same program line break at runtime:

>>> pow(1+1j, 1)
(1+1j)
>>> from math import *
>>> pow(1+1j,2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can't convert complex to float

I'm definitely more inclined to keep a the status quo, promoting the proposal by @PeterBowman above:

{
using yarp::math::operator+;
yarp::sig::Vector v3 = v1 + v2;
}

Additional/related benefits:

1. Different internal implementations of the same operation, e.g. gsl vs eigen vs pcl implementations for matrix multiplication (in line with this comment by @pattacini on math implementations: available, yet-to-come, won't-provide), maintaining flexibility/modularitity.
2. Different operations implemented in completely different use cases that still use the same types, e.g. a matrix yarp::morphology::operator+ providing dilatation (perhaps a bit far-fetched, but insert your more robotic examples here, xD).

[lornat75]

@jgvictores in the case of YARP potential clashes would only be with YARP types, in case somebody wants to provide his own set of operators. In this case it would be enough to NOT include headers from libYARP_math, and actually only the headers that define the operators.

[jgvictores]

Please PM me if this itch is bugging too much, no intention to start a 🔥-war over this or any other topic. I'm a big fan of retracting my comments. 😂

Just a fun guessing game: If using namespace yarp::math is bad practice... then hard-coding the equivalent to including this line within the math headers is...

Edit 1: It would actually not be the equivalent to using namespace yarp::math, but to using yarp::math::operator+; etc within the math headers.
Edit 2: An std:: case (see line 16 of example): http://www.cplusplus.com/reference/utility/rel_ops/