Yarp: Operators should not be in the yarp::math namespace

[drdanz]

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

https://github.com/robotology/yarp/blob/29e9108a52713dbe737d9a9c44a62da3adc3ce51/src/libYARP_math/include/yarp/math/Math.h#L16-L27

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...

https://github.com/robotology/yarp/blob/29e9108a52713dbe737d9a9c44a62da3adc3ce51/src/libYARP_math/src/math.cpp#L24-L148

[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 :fire:-war over this or any other topic. I'm a big fan of retracting my comments. :joy:

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/

[lornat75]

@jgvictores I see your point.

I have no strong opinions, when I first added the operators in the math namespace I decided to follow the strict guidelines to have all functions within the namespace. I must have considered (it has been a long time now) that it was not a big deal limiting the scope of the using directive to c/cpp files and inline functions in header files.

However, it seems also acceptable to have operators in the global namespace, if this makes many people happier...

[jgvictores]

I guess I'm just worried about establishing a precedent for bad practices. Even very good projects can get contaminated by ill-posed design decisions, and later suffer consequences such as breaking changes as-a-habit.

Perhaps introducing a yarp::math::operators namespace for lazy/q&d programmers to include a using namespace yarp::math::operators; line would be a sensible solution?

[Nicogene]

Closed (for now) by #1700

[jgvictores]

The origin of the inclusion of the operators in the namespace, cited above, was follow the strict guidelines.

Could the Issue type: Bug label please be removed?