Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ambiguous overload for 'operator*' (linear_algebra) #88

Closed
rbrugo opened this issue Mar 30, 2020 · 12 comments
Closed

Ambiguous overload for 'operator*' (linear_algebra) #88

rbrugo opened this issue Mar 30, 2020 · 12 comments
Labels
not a bug Shit happens...

Comments

@rbrugo
Copy link
Contributor

rbrugo commented Mar 30, 2020

The following code was compiled correctly:

#include <units/physical/si/length.h>
#include <units/math.h>

#include <linear_algebra.hpp>

namespace la = std::experimental::math;

template <typename ET, typename OT>
constexpr
auto norm(la::vector<ET, OT> const & v) noexcept
    -> typename la::vector<ET,OT>::value_type
{
    return units::sqrt(v * v);
}

template <typename ET, typename OT>
constexpr
auto unit(la::vector<ET, OT> const & v) noexcept
{
    return 1./norm(v) * v;
}

int main()
{
    using units::si::operator""q_m;

    auto vector = la::fs_vector<decltype(1.q_m), 3>{};
    auto unit   = ::unit(vector);
    auto scalar = 1.q_m;
    auto result = unit * scalar;
}

But after one of the last updates, the compilation fails with the following error:

$ g++ -std=c++2a -fconcepts units_la.cpp
units_la.cpp: In function ‘int main()’:
units_la.cpp:30:24: error: ambiguous overload for ‘operator*’ (operand types are ‘std::experimental::math::vector<std::experimental::math::fs_vector_engine<long double, 3>, std::experimental::math::matrix_operation_traits>’ and ‘units::quantity<units::si::dim_length, units::si::metre, long double>’)
   30 |     auto result = unit * scalar;
      |                   ~~~~ ^ ~~~~~~
      |                   |      |
      |                   |      units::quantity<units::si::dim_length, units::si::metre, long double>
      |                   std::experimental::math::vector<std::experimental::math::fs_vector_engine<long double, 3>, std::experimental::math::matrix_operation_traits>
In file included from /usr/local/include/units/physical/dimensions.h:27,
                 from /usr/local/include/units/physical/si/length.h:25,
                 from units_la.cpp:1:
/usr/local/include/units/quantity.h:335:39: note: candidate: ‘constexpr units::Quantity units::operator*(const Value&, const units::quantity<D, U, Rep>&) requires  regular_invocable<std::multiplies<void>, Value, Rep> [with Value = std::experimental::math::vector<std::experimental::math::fs_vector_engine<long double, 3>, std::experimental::math::matrix_operation_traits>; D = units::si::dim_length; U = units::si::metre; Rep = long double]’
  335 | [[nodiscard]] constexpr Quantity AUTO operator*(const Value& v, const quantity<D, U, Rep>& q)
      |                                       ^~~~~~~~
In file included from /usr/local/include/linear_algebra.hpp:86,
                 from units_la.cpp:4:
/usr/local/include/linear_algebra/arithmetic_operators.hpp:104:1: note: candidate: ‘auto std::experimental::math::operator*(const std::experimental::math::vector<ET1, OT1>&, const S2&) [with ET1 = std::experimental::math::fs_vector_engine<long double, 3>; OT1 = std::experimental::math::matrix_operation_traits; S2 = units::quantity<units::si::dim_length, units::si::metre, long double>]’
  104 | operator *(vector<ET1, OT1> const& v1, S2 const& s2)
      | ^~~~~~~~
@mpusz
Copy link
Owner

mpusz commented Apr 5, 2020

Both of the overloads are valid.

From mp-units point of view std::experimental::math::vector<std::experimental::math::fs_vector_engine<long double, 3>, std::experimental::math::matrix_operation_traits> storing long double is just a Scalar (not a quantity) and every quantity can be multiplied by the scalar.

From linear-algebra point of view units::quantity<units::si::dim_length, units::si::metre, long double> is a scalar type (not a vector or matrix) and every vector or matrix can be multiplied by a scalar.

Please note that I already did the best I could and introduced a WrappedQuantity concept. This disambiguates those overloads when a vector or matrix stores quantities. This is not the case here.

As both overloads are valid which of those operators would you like to choose and why? How to deduce it in a generic way at compile time so it works with every library (not only with this particular LA one)?

I think the only way to make it work is to define your own operators that will explicitly say on how to multiply those specific types based on your specific use case.

@mpusz mpusz added the not a bug Shit happens... label Apr 5, 2020
@kwikius
Copy link
Contributor

kwikius commented Apr 5, 2020

I resolved this using 4 concepts (N.B not same semantics if similar named std versions)
(These also work with the binary_op metafunction which provides a return type of undefined for invalid ops)

Firstly Numeric<T> which is basically floats and ints (though user defined like angles, rational etc also can be model). N.B Vectors ,Quantities are specifically not Numeric

Second Quantity<T> which is physical quantity with Numeric valuetype
N.B Quantities with non-Numeric valuetypes are specifically not defined ( so no quantity<complex> quantity<vector> etc. (angle is allowed though!) )

Thirdly Scalar<T> which is basically Numeric + Quantities

Fourth Vector<T>

Multiply ops are defined on generic models of these concepts one one side angainst the particular user defined type on the other and unambiguous once so constrained

myQuantity * Numeric -> myQuantity // defined by myQuantity library
Numeric * myQuantity -> myQuantity // defined by myQuantity library

myVector * Scalar -> myVector // defined by myVector library
Scalar * mVector -> myVector //defined by myVector Library

Numeric<T> = false

Numeric<int> = true;
Numeric<float> = true;

Scalar<T> = false;

Scalar<Numeric> = true
Scalar<myQuantity> = true

Vector<T> = false

Vector<myVector> == true

https://github.com/kwikius/quan-trunk/blob/master/quan/fixed_quantity/operations/scalar_multiply.hpp#L96

https://github.com/kwikius/quan-trunk/blob/master/quan/three_d/vect.hpp#L244

related
#28

@kwikius
Copy link
Contributor

kwikius commented Apr 5, 2020

Both of the overloads are valid.

From mp-units point of view std::experimental::math::vector<std::experimental::math::fs_vector_engine<long double, 3>, std::experimental::math::matrix_operation_traits> storing long double is just a Scalar (not a quantity) and every quantity can be multiplied by the scalar.

If Scalar is defined as ! Quantity This is the problem. Every type needs to provide strong constraints on its operations to prevent wipeing out other types useful operations.

Wikipedia has a good definition,:

Scalar (physics), a physical quantity that can be described by a single element of a number field such as a real number
https://en.wikipedia.org/wiki/Scalar

@mpusz
Copy link
Owner

mpusz commented Apr 5, 2020

Term "Scalar" definition depends on a domain. In maths it is the description you provided and this is the one used by the LA. In Physical Units domain a Scalar is not a quantity. Both definitions are and should be different (depending on a domain).

When we talk about physical units a scalar can be any user-defined type that provides basic arithmetic operations (i.e. complex number or a vector or matrix). See the following example:

The above would be invalid with your overconstrained Scalar<T> concept. Overconstraining the concepts is a big problem and we should really try to avoid it.

@kwikius
Copy link
Contributor

kwikius commented Apr 5, 2020

We could argue what Scalar means but it doesnt solve your problem ;)

Surely this should 'just work' without any WrappedQuantity or other mischief?

https://github.com/kwikius/units/blob/kwikius_vector_mul_q/example/linear_algebra.cpp#L269

At least it does work for me on Arduino. Nice and neat

https://github.com/kwikius/ArduIMU/blob/master/ArduIMU/MPU6000.cpp#L173

The linear algebra lib operator function is entirely S2 unconstrained so
template<class ET1, class OT1, class S2>
inline auto
operator *(vector<ET1, OT1> const& v1, S2 const& s2);

Best to be explicit
concept AnyType<T> = true

template<class ET1, class OT1, AnyType S2>
inline auto
operator *(vector<ET1, OT1> const& v1, S2 const& s2);

But actually should be more constrained...

@mpusz
Copy link
Owner

mpusz commented Apr 6, 2020

We could argue what Scalar means but it doesnt solve your problem ;)

Agree, if you have a better name for the "Scalar" term in a physical units domain please let me know.

I cannot influence other libraries to constrain their operators. I can do everything with mine. For now, I found out that "WrappedQuantity" works well to disambiguate the case of vector<length> * length. However, it does not work with vector<double> * length. I do not know how to disambiguate it.

There are 2 solutions here:

  1. Leave it to the user to write his/her own operator for those concrete types.
  2. Introduce a type trait (ie. not_scalar<T>) where the user could explicitly disable some type to be a Scalar type in a physical Units library.

@kwikius
Copy link
Contributor

kwikius commented Apr 16, 2020

We could argue what Scalar means but it doesnt solve your problem ;)

Agree, if you have a better name for the "Scalar" term in a physical units domain please let me know.

I cannot influence other libraries to constrain their operators. I can do everything with mine. For now, I found out that "WrappedQuantity" works well to disambiguate the case of vector<length> * length. However, it does not work with vector<double> * length. I do not know how to disambiguate it.

There are 2 solutions here:

1. Leave it to the user to write his/her own operator for those concrete types.

2. Introduce a type trait (ie. `not_scalar<T>`) where the user could explicitly disable some type to be a Scalar type in a physical Units library.

Sorry but those are not the only choices and this view is not just mine. See also here.

See #23 (comment)

And please re-read the excellent paper cited there about representing matrices which describes using quantities in matrices, since this https://github.com/mpusz/units/blob/82f493f9ceee08df9e5d589546fdf6ecef77d53a/example/linear_algebra.cpp#L158 is not how matrices work with quantities.

The other choice reqarding a physical quantity type for c++ is to restrict the valuetype of the quantity to being a scalar type( According to the widely respected standard definitions https://en.wikipedia.org/wiki/Scalar) as I and https://github.com/dwith-ts said previously and then allow other libraries to provide the vector complex and matrix functionality. This relies on both mpusz/units and a linear algebra library being specified in terms of a common set of concepts, which surely is something that WG21 members understand and can agree on?

Alternatively If you allow these types in the standard quantities library then you must also provide the full suite of operations if it is not to be a toy library

For complex real imag arg trig array access. etc etc...
For vector dot norm(magnitude) cross etc etc
For matrix access transpose inverse det etc.

which ultimately is wasted duplicated effort.

My Library constrains its valuetypes to scalar types and works well. converting from a vector of numeric types is a basic op.
https://github.com/kwikius/ArduIMU/blob/master/visualisation/mag_acc/mag_acc_algorithm.cpp#L70

Mpusz/units fails in a simple case as shown by this issue.

@mpusz
Copy link
Owner

mpusz commented Apr 17, 2020

I think I do not fully agree here.

since this is not how matrices work with quantities.

There is more than one way to do it. I think that in case someone wants a matrix of a specific quantity type it is a valid use case. If I forbid it, I will have a new Issue opened soon by someone wanting such a use case to work. There are no good reasons to disallow it. Sure it can be optimized by the techniques described in the paper. Sure it does not work with matrices of different quantity types. But for this specific use case, it is fine.

According to the widely respected standard definitions https://en.wikipedia.org/wiki/Scalar

I do not see anywhere in this document that Scalar is only an int or float like your library assumes. Why wide_integer, non_truncating<int>, or any type from Boost.Multiprecision should be disallowed to work with this library? Did you see my and boost example with the measurement representation type? Why it is wrong in your opinion? Does your library fail in those simple real-life cases? 😉

@kwikius
Copy link
Contributor

kwikius commented Apr 17, 2020

I think I do not fully agree here.

since this is not how matrices work with quantities.

There is more than one way to do it. I think that in case someone wants a matrix of a specific quantity type it is a valid use case. If I forbid it, I will have a new Issue opened soon by someone wanting such a use case to work. There are no good reasons to disallow it. Sure it can be optimized by the techniques described in the paper. Sure it does not work with matrices of different quantity types. But for this specific use case, it is fine.

According to the widely respected standard definitions https://en.wikipedia.org/wiki/Scalar

I do not see anywhere in this document that Scalar is only an int or float like your library assumes. Why wide_integer, non_truncating<int>, or any type from Boost.Multiprecision should be disallowed to work with this library? Did you see my and boost example with the measurement representation type? Why it is wrong in your opinion? Does your library fail in those simple real-life cases? wink

"_I do not see anywhere in this document that Scalar is only an int or float like your library assumes."

I cannot find where I said that? I refer you to #88 (comment) para4 which specifically states that a quantity is a Scalar

https://github.com/kwikius/quan-trunk/blob/master/quan/meta/is_scalar.hpp#L43
https://github.com/kwikius/quan-trunk/blob/master/quan/fixed_quantity/fixed_quantity.hpp#L463

I also specifically made mpusz/units quantity a model here to work with my 2d 3d vector library
https://github.com/kwikius/quan-trunk/blob/master/quan/compatibility/mpusz_units.hpp#L16

In my library, a quantity is specifically a scalar type. I do however restrict to not allowing a quantity to have vector value_type because it causes confusion and ambiguous overloads exactly as pointed out in this issue. Vector is much better handles by its own library, but it needs to be able to work with quantity library, which one would assume might be a goal of WG21 meetings!

I do not see anywhere in this document that Scalar is only an int or float like your library assumes. Why wide_integer, non_truncating<int>, or any type from Boost.Multiprecision should be disallowed to work with this library? Did you see my and boost example with the measurement representation type? Why it is wrong in your opinion? Does your library fail in those simple real-life cases? wink

Again I refer you to the above link. I have not restricted Scalar to Int or Float. These types, measure, rational may be models of Numeric concept in which case they are also probably models of Scalar concept.

@kwikius
Copy link
Contributor

kwikius commented Apr 17, 2020

There is more than one way to do it. I think that in case someone wants a matrix of a specific quantity type it is a valid use case. If I forbid it, I will have a new Issue opened soon by someone wanting such a use case to work. There are no good reasons to disallow it. Sure it can be optimized by the techniques described in the paper. Sure it does not work with matrices of different quantity types. But for this specific use case, it is fine.

matrix<R,C,T> wont work in most cases. You need matrix<R,C,T...>
A quantity capable matrix requires a tuple of elements in the simple case I implemented ( or some alternative behind the scenes as hinted in the above paper)
Here is an example
https://github.com/kwikius/quan-trunk/blob/master/quan_matters/examples/fusion/kalman1.cpp#L48
m1 holds types of real, time, time^2, 1/t, 1/t^2 etc.
x holds 3d vectors of length, velocity, acceleration

@kwikius
Copy link
Contributor

kwikius commented Jul 2, 2020

BobSteagall/wg21#58

@mpusz
Copy link
Owner

mpusz commented Jun 15, 2023

Does not reproduce in V2 anymore.

@mpusz mpusz closed this as completed Jun 15, 2023
Issues Kanban Board automation moved this from To do to Done Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not a bug Shit happens...
Projects
Development

Successfully merging a pull request may close this issue.

3 participants