Integral representations for irrational canonical units

[nebkat]

(Apologies if I am too early reporting issues on 2.0 branch!)

RepSafeConstructibleFrom prevents quantities from being constructed with integral representations if the desired unit has an irrational magnitude relative to its base unit (since 05ed7dae88e6c0b3201e86585738664742c68879).

For example, angular::revolution is defined as mag<2> * mag_pi * radian, leading to the following:

auto ok = 5.0 * angular::revolution;
auto fail = 5 * angular::revolution;

candidate template ignored: constraints not satisfied [with Value = int] because 
    'detail::RepSafeConstructibleFrom<mp_units::quantity<scaled_unit<mag, mp_units::angular::revolution>{}, int>::rep, int &&, unit>' evaluated to false because 
        'treat_as_floating_point<int>' evaluated to false and
        'is_rational(get_canonical_unit(scaled_unit<mag, mp_units::angular::revolution>{}).mag)' evaluated to false

Example

// helper used to constrain `make_quantity()` and `operator*(Representation, Unit)`
static constexpr bool _rep_safe_constructible_ = detail::RepSafeConstructibleFrom<Rep, Rep&&, unit>;

Seems to me like make_quantity() should never be constrained in this way, but rather only in a conversion when the relative magnitude between two units is irrational - and this is currently handled correctly by QuantityConvertibleTo.

[mpusz]

(Apologies if I am too early reporting issues on 2.0 branch!)

No need to apologize 😄 The V2 is already on the mainline, so it should be steady and robust.

Yes, I agree this feature is tricky, and this design decision resulted from the fact that value_cast<si::radian>(5 * angular::revolution) returned an integral representation type that was off a lot from the expected value. This is why I disallowed it in the first approach and was waiting for the feedback from the community 😉 Thank you very much for bringing this up!

Now, we have a design question to solve. If the conversion between two units has an irrational factor and the quantity has an integral representation type, what should we do when we do value_cast<U> to change the unit (note that the quantity.in(U) will not compile as the operation is not value-preserving):

• allow such a cast and return a quantity of integral representation type
• allow such a cast and return a quantity with representation being the result of conversion (i.e. int * long double)
• disallow such a cast at all (before that a value_cast<long double>(q) will be needed to change the representation type).

[mpusz]

Please note that we have a similar problem in trigonometric functions:

https://github.com/mpusz/mp-units/blob/c6e9a9caf23472dc5ae98190286a5459e2c0bb84/src/utility/include/mp-units/math.h#L357-L364

For now, we require a floating-point representation there as well, even though the user could argue that an angle in degrees makes a lot of sense. How should we handle this as well?

[nebkat]

disallow such a cast at all (before that a value_cast(q) will be needed to change the representation type).

For me this would be the most sensible option:

• In the trivial example value_cast<si::radian>(5 * angular::revolution) the fix is of course easy - replace 5 with 5.0.
• If we return integral, there can be huge loss of precision as you mentioned, so no good.
• Often if I am using integral types in the first place it is to avoid expensive floating point operations (or some other specific reason), so I wouldn't want to unexpectedly jump to float (in a sea of auto defs 😀 ).
  • double is a good default representation, but shouldn't be assumed from the unit if unspecified (unlike a normal int * double = double scenario where it is specified).
• Perhaps a value_cast<si::radian, long double> could be introduced?
  • Useful even in rational conversions where the original representation type can become too big/small after scaling, e.g. value_cast<km, uint8_t>(uint16_t { 12345 } * m).
• Or better yet a quantity.in<Rep, U>(), which is already protected by QuantityConvertibleTo for int to float conversions.

For now, we require a floating-point representation there as well, even though the user could argue that an angle in degrees makes a lot of sense. How should we handle this as well?

The std::sin() mapping is:

float -> float
double -> double
Integer -> double

So I think an implicit cast to std::conditional_t<std::is_floating_point_v<Rep>, Rep, double> would work:

template<ReferenceOf<angle> auto R, typename Rep, typename Rep2 = std::conditional_t<std::is_floating_point_v<Rep>, Rep, double>>
 [[nodiscard]] inline quantity<one, Rep2> sin(const quantity<R, Rep>& q) noexcept 
   requires requires { sin(q.value()); } || requires { std::sin(q.value()); } 
 { 
   using std::sin; 
   return make_quantity<one>(static_cast<Rep2>(sin(q.in<Rep2>(radian).value()))); 
 } 

[JohelEGP]

The std::sin() mapping is:

This is a nice find. If std::sin already does that, or the other constraint is satisfied (requires { sin(q.value()); }), then there's no need for the constraint requires treat_as_floating_point<Rep>. This last constraint should be dropped and the return type not fixed to use Rep (i.e., quantity<one, Rep>), but whatever sin returns.

[mpusz]

disallow such a cast at all (before that a value_cast(q) will be needed to change the representation type).

For me this would be the most sensible option: If we return integral, there can be huge loss of precision as you mentioned, so no good.

I am not sure if this exception is much different from the value_cast<ms>(2 * s) case. I think that irrational factors and rational ones with denominator != 1 are similar scenarios in this regard. In both cases, we risk losing precision and that is why we actually introduced value_cast. So maybe we should allow such cast and return the type as provided by the user in the input argument?

[mpusz]

The std::sin() mapping is: Integer -> double

This is true. However, std::sin() always takes a value in radians, and we allow any unit here (i.e. degrees) and we need to convert to radian before calling std::sin(). We can do something like the below but I am not sure if it also is not surprising:

template<ReferenceOf<angular_measure> auto R, typename Rep>
[[nodiscard]] inline QuantityOf<kind_of<dimensionless>> auto sin(const quantity<R, Rep>& q) noexcept
  requires requires { sin(q.value()); } || requires { std::sin(q.value()); }
{
  using std::sin;
  if constexpr (!treat_as_floating_point<Rep>)
    return make_quantity<one>(sin(value_cast<double>(q).value_in(si::radian)));
  else
    return make_quantity<one>(sin(q.value_in(si::radian)));
}

And what about asin(). std::asin() also supports integral types but the only possible values here are -1, 0, and 1. Again we allow any unit here (i.e. percent) and conversion to one could also be truncating.

[chiphogg]

Very amusing... it would appear that GitHub sees @mpusz as a proper subset of @JohelEGP? :laughing:

[JohelEGP]

Now: