gul-cpp / gul14

General Utility Library for C++14
https://gul14.info/
GNU Lesser General Public License v2.1
2 stars 1 forks source link

Allow custom numeric types in abs(), within_abs() #61

Open alt-graph opened 9 months ago

alt-graph commented 9 months ago

For a well-formed numerical type like "Meters", it was previously not possible to call gul14::abs() or gul14::within_abs(). This can be done with this PR.

How: In the implementation of abs(), try to use std::abs(). If it is not available, fall back to calling a user-defined abs() via ADL.

This is a draft so far to see if an extension in this direction makes sense. We should discuss if a user-supplied implementation of is_floating_point<T> is needed.

Finii commented 9 months ago

Well, that STILL fails (it compiles but the resulting code is wrong), because

template<typename NumT>
bool within_abs(NumT a, NumT b, NumT tol) noexcept {
    tol = gul14::abs(tol); // Negative diff does not make sense
    bool ret{};
    if (a > b) {
        if (std::is_floating_point<NumT>::value)
            ret = a - tol <= b; // different formula needed because of inf/-inf and subnormal values
        else
            ret = a - b <= tol;

std::is_floating_point<Meters>::value is false and we do not take the appropriate comparison branch.

We should discuss if a user-supplied implementation of is_floating_point<T> is needed

As std::is_floating_point<> is no customization point we may not [1] specialize it (unfortunately?)

But we can specialize std::numeric_limits<Meters> [2], and use the info given therein. Like for example this other units library (first search hit): https://github.com/nholthaus/units/blob/3ca0e22c5e5088a93f930617d193e895eb538bca/include/units.h#L4842

But then, the problem occurs at a lot other GUL functions as well (I already pointed out the statistics functions), and it also happens at cmath functions (that is probably the reason Meters has the atan friend).

[1] https://en.cppreference.com/w/cpp/types/is_arithmetic [2] https://en.cppreference.com/w/cpp/types/numeric_limits


I do not really understand this. The Meters is not an arithmetic type. We could add to the docu of GUL that we do only take arithmetic types, I always assumed so much. As usual if you want to use math functions an arithmetic type has to be used.


Rolling back my thoughts.

  1. Why does Meter not just have an ordinary cast operator (see diff below)
    • That would solve atan without a friend and is also otherwise usually what one wants
    • Obviously you want that people explicitly cast out of the Meter for some reason?
  2. If explicitness is the goal, why have an atan2 specialization? Other libraries / stuff that take a floating point number must always use casted values, for example doocs_prop.set_value(distance). A more clean solution would be to either allow Meters directly everywhere (implicit cast) or nowhere (also no atan and no max)
  3. I think this is not a defect or deficiency of GUL, but a target conflict in Meters and should be solved there as done with atan2 and abs

And

  1. If we fix it here we would need a more complete Meter including the numeric_limits. It's unclear why it does not have a specialization.
  2. If we start expanding stuff here to more generic types we would need to fix it for all functions in GUL which are a lot more
  3. If compatibility of within_abs with Meters is important then within_ulb and within_orders has the same importance. They both also reject Meters at the moment.
--- a/include/hlc/units/Quantity.h
+++ b/include/hlc/units/Quantity.h
@@ -124,7 +126,7 @@ public:
     /// Explicitly cast this quantity to a floating-point value.
     template <typename FloatT,
               std::enable_if_t<std::is_floating_point<FloatT>::value, bool> = true>
-    explicit constexpr operator FloatT() const noexcept
+    constexpr operator FloatT() const noexcept
     {
         return value_;
     }
soerengrunewald commented 9 months ago

I'm not sure if I understand this all correctly. To make this work, Meter needs a specialization of abs, so why go through the indirection of gul in the first place? Sure is somewhat convenient to use the same function for all types and the outcome should be the same, but still I wonder.

I also though, why not have implicit conversion and be done. But this will not fly, since you can not answer the question to what quantity you should convert without explicitly ask for the desired dimension.

alt-graph commented 9 months ago

I'm not sure if I understand this all correctly. To make this work, Meter needs a specialization of abs, so why go through the indirection of gul in the first place? Sure is somewhat convenient to use the same function for all types and the outcome should be the same, but still I wonder.

Meter has an associated free function Meter abs(Meter), but there is no way of making std::abs(Meter) work. Instead, this MR makes gul14::abs() fall back to that free function in the case that there is no eligible std::abs() call. In turn, this makes gul14::within_abs() work with arguments of type Meter. It would just be convenient if the various within_* functions would work on physical unit types and not just on stock integers and floats.

I also though, why not have implicit conversion and be done. But this will not fly, since you can not answer the question to what quantity you should convert without explicitly ask for the desired dimension.

Exactly. We use physical unit types to avoid unit errors, so implicit conversions are a no-go.

alt-graph commented 9 months ago
  1. Why does Meter not just have an ordinary cast operator (see diff below)

    • That would solve atan without a friend and is also otherwise usually what one wants

    • Obviously you want that people explicitly cast out of the Meter for some reason?

Yes. The very point of using a physical unit type is to avoid erroneous conversions, so having both Meters and Kilograms convert implicitly to double defeats the purpose.

alt-graph commented 9 months ago
  • If we fix it here we would need a more complete Meter including the numeric_limits. It's unclear why it does not have a specialization.

That could be added. The point of this issue and MR is just to understand if it makes sense to continue working in this direction.

  • If we start expanding stuff here to more generic types we would need to fix it for all functions in GUL which are a lot more

IMO we would also gain from implementing this for some subset, e.g. the within_* functions.

  • If compatibility of within_abs with Meters is important then within_ulb and within_orders has the same importance. They both also reject Meters at the moment.

Absolutely, I just wanted to sketch the general idea first.

Finii commented 9 months ago

In turn, this makes gul14::within_abs() work with arguments of type Meter.

Well, that (i.e. gul::abs() using the free function) is necessary but not sufficient.

I guess we can rework this (i.e. GUL) after Meters et al got a std::numeric_limits<> specialization.