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

Different results using std::exp() and units::math::exp(). #284

Open
nholthaus-units-user opened this issue Mar 3, 2022 · 2 comments
Open

Comments

@nholthaus-units-user
Copy link

nholthaus-units-user commented Mar 3, 2022

I am using the units.h file as of commit ea6d126. The issue described below can be reproduced using gcc-11.2.0 (and probably using any other C++14 compiler).

Using std::exp() and units::math::exp() on the same value can produce confusingly different results and the reason for it is non-intuitive. Here is some code that illustrates this:

#include "units.h"

#include <iostream>
#include <cmath>

using namespace units::literals;

int main()
{
    const auto a = -1.0 / 1_m ;
    const auto b = 1_um ;
    const auto c = a * b ;
    std::cout << "c                   : " << c << std::endl ;
    std::cout << "units::math::exp(c) : " << units::math::exp(c) << std::endl ;
    std::cout << "std::exp(c.value()) : " << std::exp(c.value()) << std::endl ;
    return EXIT_SUCCESS ;
}

The output looks something like the following:

c                   : -1e-06
units::math::exp(c) : 0.367879
std::exp(c.value()) : 0.999999

The value of c appears to be -1e-06 here and, hence, std::exp(c.value()) appears to produce the expected result. However, units::math::exp(c) produces a different result.

Is this intended/expected behavior?

@ts826848
Copy link
Contributor

I suspect this behavior is unintended.

units::math::exp() uses operator()():

units/include/units.h

Lines 4467 to 4472 in ea6d126

template<class ScalarUnit>
dimensionless::scalar_t exp(const ScalarUnit x) noexcept
{
static_assert(traits::is_dimensionless_unit<ScalarUnit>::value, "Type `ScalarUnit` must be a dimensionless unit derived from `unit_t`.");
return dimensionless::scalar_t(std::exp(x()));
}

return dimensionless::scalar_t(std::exp(x())); 
//                                       ^^ 

operator()() returns the "raw" underlying value, completely disregarding any information that is encoded in the unit's type:

inline constexpr T operator()() const noexcept { return m_value; } ///< returns value.

Unfortunately, there is some relevant information in the type (emphasis added using ***):

(lldb) p c
(const units::unit_t<units::unit<***std::ratio<1, 1000000>***, units::base_unit<std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1> >, std::ratio<0, 1>, std::ratio<0, 1> >, double, units::linear_scale>) $6 = {
  units::linear_scale<double> = (m_value = -1)
}

This means that operator()() is not the right choice here, as it discards information about the "real" value.

Not all is lost, though; value() delegates to conversion operators, and the conversion operators explicitly call convert() to ensure the information encoded in the type is correctly accounted for:

units/include/units.h

Lines 2133 to 2142 in ea6d126

/**
* @brief implicit type conversion.
* @details only enabled for scalar unit types.
*/
template<class Ty, std::enable_if_t<traits::is_dimensionless_unit<Units>::value && std::is_arithmetic<Ty>::value, int> = 0>
inline constexpr operator Ty() const noexcept
{
// this conversion also resolves any PI exponents, by converting from a non-zero PI ratio to a zero-pi ratio.
return static_cast<Ty>(units::convert<Units, unit<std::ratio<1>, units::category::scalar_unit>>((*this)()));
}

This means the fix should be easy. Just replace x() with x.value() in exp(), and you should get the correct answer.

Before:

c                   : -1e-06
c()                 : -1
c.value()           : -1e-06
units::math::exp(c) : 0.367879
std::exp(c.value()) : 0.999999

After:

c                   : -1e-06
c()                 : -1
c.value()           : -1e-06
units::math::exp(c) : 0.999999
std::exp(c.value()) : 0.999999

I would guess a similar change would need to be made to other functions.


Looks like the v3.x branch suffers from a similar issue:

#include <cmath>
#include <iostream>
#include <units/length.h>

using units::literals::operator""_m;
using units::literals::operator""_um;

int main()
{
	const auto a = -1.0 / 1_m;
	const auto b = 1_um;
	const auto c = a * b;
	std::cout << "c                   : " << c << std::endl;
	std::cout << "double(c)           : " << double(c) << std::endl;
	std::cout << "c.value()           : " << c.value() << std::endl;
	std::cout << "units::math::exp(c) : " << units::exp(c) << std::endl;
	std::cout << "std::exp(c.value()) : " << std::exp(c.value()) << std::endl;
	std::cout << "std::exp(double(c)) : " << std::exp(double(c)) << std::endl;
	return EXIT_SUCCESS;
}

Output:

c                   : -1e-06
double(c)           : -1e-06
c.value()           : -1
units::math::exp(c) : 3.67879e-07
std::exp(c.value()) : 0.367879
std::exp(double(c)) : 0.999999
(lldb) p c
(const units::unit<units::conversion_factor<std::ratio<1, 1000000>, units::dimension_t<>, std::ratio<0, 1>, std::ratio<0, 1> >, double, units::linear_scale>) $0 = (linearized_value = -1)

I think the fix here is a similar idea - use the conversion operator to obtain the underlying value instead of value(), since the latter appears to throw away potentially useful information.


I can't make a PR right away, but I should be able to come up with something in the coming days.

ts826848 pushed a commit to ts826848/units that referenced this issue May 25, 2022
…types

As reported in issue nholthaus#284, some functions produce incorrect results when
passed certain dimensionless quantites. For example, this program from
the issue, slightly modified for brevity and to try to make a later
point clearer:

    #include "units.h"
    #include <iostream>
    #include <cmath>
    using namespace units::literals;
    int main()
    {
        const auto c = 1.0_um * (-1 / 1.0_m);
        std::cout << "c                   : " << c << std::endl ;
        std::cout << "units::math::exp(c) : " << units::math::exp(c) << std::endl ;
        std::cout << "std::exp(c.value()) : " << std::exp(c.value()) << std::endl ;
        return EXIT_SUCCESS ;
    }

Outputs:

    c                   : -1e-06
    units::math::exp(c) : 0.367879
    std::exp(c.value()) : 0.999999

value() is basically to<>(), but with the template parameter hard-coded
to underlying_type, and to<>() is one of the documented ways to pass
something to a non-unit-enabled API, so getting different results this
way is rather surprising.

The proximate cause of this difference is the use of operator()()
instead of value() or to<>() by (some?) functions in units::math. In the
case of the example above:

    template<class ScalarUnit>
    dimensionless::scalar_t exp(const ScalarUnit x) noexcept
    {
        static_assert(traits::is_dimensionless_unit<ScalarUnit>::value, "Type `ScalarUnit` must be a dimensionless unit derived from `unit_t`.");
        return dimensionless::scalar_t(std::exp(x()));
        // operator()() instead of value()/to()  ^^
    }

The use of operator()() means that std::exp() is fed a different input
than when value() is used, as demonstrated by the output from an
appropriately modified version of the example program:

    c                   : -1e-06
    c()                 : -1
    c.value()           : -1e-06
    units::math::exp(c) : 0.367879
    std::exp(c.value()) : 0.999999

However, this problem does not for all possible inputs. If the
expression for c is changed as such:

    // const auto c = 1.0_um * (-1 / 1.0_m);
    const auto c = -1.0_um / 1.0_m;

The issue appears to vanish:

    c                   : -1e-06
    c()                 : -1e-06
    c.value()           : -1e-06
    units::math::exp(c) : 0.999999
    std::exp(c.value()) : 0.999999

I believe this is because the different calculations result in objects
with different types:

    # const auto c = 1.0_um * (-1 / 1.0_m);
    (lldb) p c
    (const units::unit_t<units::unit<std::ratio<1, 1000000>, units::base_unit<std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1> >, std::ratio<0, 1>, std::ratio<0, 1> >, double, units::linear_scale>) $0 = {
      units::linear_scale<double> = (m_value = -1)
    }

    # const auto c = 1.0_um / 1.0_m;
    (lldb) p c
    (const units::dimensionless::scalar_t) $0 = {
      units::linear_scale<double> = (m_value = 9.9999999999999995E-7)
    }

Where the latter is equivalent to:

    # const auto c = 1.0_um / 1.0_m;
    (lldb) p c
    (const units::unit_t<units::unit<std::ratio<1, 1>, units::base_unit<std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1> >, std::ratio<0, 1>, std::ratio<0, 1> >, double, units::linear_scale>) $0 = {
      units::linear_scale<double> = (m_value = 9.9999999999999995E-7)
    }

The difference in the results amounts to the "new" calculation having
the "correct" value directly in m_value, whereas the "old" calculation
has some information in the type, which could be described as the
exponent in the type and the mantissa in m_value. Floating-point errors
aside, they are equal.

This points to the ultimate cause of the issue: operator()() and
value()/to<>() appear to consider different information when producing
their results. operator()() fully discards information encoded in the
type, directly returning the "raw" underlying value (modified for the
decibel scale if needed), while for dimensionless types value() and
to<>() account for both information encoded in the type and the "raw"
underlying value in their return values. This is apparent in their
implementations, where operator()() has no awareness of the units::unit
tag, but value()/to<>() delegate to the conversion operator for
dimensionless quantities, which explicitly "normalizes" the underlying
value using units::convert<>() before returning it:

    template<class Units, typename T = UNIT_LIBDEFAULT_TYPE, template<typename> class NonLinearScale = linear_scale>
    class unit_t : public NonLinearScale<T>, units::detail::_unit_t
    {
    public:
        typedef T underlying_type;

        inline constexpr underlying_type value() const noexcept
        {
            return static_cast<underlying_type>(*this);
        }

        template<typename Ty, class = std::enable_if_t<std::is_arithmetic<Ty>::value>>
        inline constexpr Ty to() const noexcept
        {
            return static_cast<Ty>(*this);
        }

        template<class Ty, std::enable_if_t<traits::is_dimensionless_unit<Units>::value && std::is_arithmetic<Ty>::value, int> = 0>
        inline constexpr operator Ty() const noexcept
        {
            // this conversion also resolves any PI exponents, by converting from a non-zero PI ratio to a zero-pi ratio.
            return static_cast<Ty>(units::convert<Units, unit<std::ratio<1>, units::category::scalar_unit>>((*this)()));
        }
    };

    template<typename T>
    struct linear_scale
    {
        inline constexpr T operator()() const noexcept { return m_value; }
        T m_value;
    };

    template<typename T>
    struct decibel_scale
    {
        inline constexpr T operator()() const noexcept { return 10 * std::log10(m_value); }
        T m_value;
    };

As a result, the problem is clear: the functions using operator()() to
obtain a value to pass to std::math functions are incorrect and should
be changed to use value() so relevant information is not thrown away.

The functions changed in this commit are the functions grouped under the
"TRANSCEDENTAL FUNCTIONS" header in the header file. This is why modf()
is changed here despite not being a transcedental function. Some other
functions suffer from a similar issue, but those will be addressed in
different commits.
@ts826848
Copy link
Contributor

Sorry it took so long; I submitted #288 to try to address your issue.

Still have to make a version for v3.x as well, but that's something for another day...

nholthaus pushed a commit that referenced this issue Sep 26, 2022
* Fix incorrect transcedental function output for scaled dimensionless types

As reported in issue #284, some functions produce incorrect results when
passed certain dimensionless quantites. For example, this program from
the issue, slightly modified for brevity and to try to make a later
point clearer:

    #include "units.h"
    #include <iostream>
    #include <cmath>
    using namespace units::literals;
    int main()
    {
        const auto c = 1.0_um * (-1 / 1.0_m);
        std::cout << "c                   : " << c << std::endl ;
        std::cout << "units::math::exp(c) : " << units::math::exp(c) << std::endl ;
        std::cout << "std::exp(c.value()) : " << std::exp(c.value()) << std::endl ;
        return EXIT_SUCCESS ;
    }

Outputs:

    c                   : -1e-06
    units::math::exp(c) : 0.367879
    std::exp(c.value()) : 0.999999

value() is basically to<>(), but with the template parameter hard-coded
to underlying_type, and to<>() is one of the documented ways to pass
something to a non-unit-enabled API, so getting different results this
way is rather surprising.

The proximate cause of this difference is the use of operator()()
instead of value() or to<>() by (some?) functions in units::math. In the
case of the example above:

    template<class ScalarUnit>
    dimensionless::scalar_t exp(const ScalarUnit x) noexcept
    {
        static_assert(traits::is_dimensionless_unit<ScalarUnit>::value, "Type `ScalarUnit` must be a dimensionless unit derived from `unit_t`.");
        return dimensionless::scalar_t(std::exp(x()));
        // operator()() instead of value()/to()  ^^
    }

The use of operator()() means that std::exp() is fed a different input
than when value() is used, as demonstrated by the output from an
appropriately modified version of the example program:

    c                   : -1e-06
    c()                 : -1
    c.value()           : -1e-06
    units::math::exp(c) : 0.367879
    std::exp(c.value()) : 0.999999

However, this problem does not for all possible inputs. If the
expression for c is changed as such:

    // const auto c = 1.0_um * (-1 / 1.0_m);
    const auto c = -1.0_um / 1.0_m;

The issue appears to vanish:

    c                   : -1e-06
    c()                 : -1e-06
    c.value()           : -1e-06
    units::math::exp(c) : 0.999999
    std::exp(c.value()) : 0.999999

I believe this is because the different calculations result in objects
with different types:

    # const auto c = 1.0_um * (-1 / 1.0_m);
    (lldb) p c
    (const units::unit_t<units::unit<std::ratio<1, 1000000>, units::base_unit<std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1> >, std::ratio<0, 1>, std::ratio<0, 1> >, double, units::linear_scale>) $0 = {
      units::linear_scale<double> = (m_value = -1)
    }

    # const auto c = 1.0_um / 1.0_m;
    (lldb) p c
    (const units::dimensionless::scalar_t) $0 = {
      units::linear_scale<double> = (m_value = 9.9999999999999995E-7)
    }

Where the latter is equivalent to:

    # const auto c = 1.0_um / 1.0_m;
    (lldb) p c
    (const units::unit_t<units::unit<std::ratio<1, 1>, units::base_unit<std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1>, std::ratio<0, 1> >, std::ratio<0, 1>, std::ratio<0, 1> >, double, units::linear_scale>) $0 = {
      units::linear_scale<double> = (m_value = 9.9999999999999995E-7)
    }

The difference in the results amounts to the "new" calculation having
the "correct" value directly in m_value, whereas the "old" calculation
has some information in the type, which could be described as the
exponent in the type and the mantissa in m_value. Floating-point errors
aside, they are equal.

This points to the ultimate cause of the issue: operator()() and
value()/to<>() appear to consider different information when producing
their results. operator()() fully discards information encoded in the
type, directly returning the "raw" underlying value (modified for the
decibel scale if needed), while for dimensionless types value() and
to<>() account for both information encoded in the type and the "raw"
underlying value in their return values. This is apparent in their
implementations, where operator()() has no awareness of the units::unit
tag, but value()/to<>() delegate to the conversion operator for
dimensionless quantities, which explicitly "normalizes" the underlying
value using units::convert<>() before returning it:

    template<class Units, typename T = UNIT_LIBDEFAULT_TYPE, template<typename> class NonLinearScale = linear_scale>
    class unit_t : public NonLinearScale<T>, units::detail::_unit_t
    {
    public:
        typedef T underlying_type;

        inline constexpr underlying_type value() const noexcept
        {
            return static_cast<underlying_type>(*this);
        }

        template<typename Ty, class = std::enable_if_t<std::is_arithmetic<Ty>::value>>
        inline constexpr Ty to() const noexcept
        {
            return static_cast<Ty>(*this);
        }

        template<class Ty, std::enable_if_t<traits::is_dimensionless_unit<Units>::value && std::is_arithmetic<Ty>::value, int> = 0>
        inline constexpr operator Ty() const noexcept
        {
            // this conversion also resolves any PI exponents, by converting from a non-zero PI ratio to a zero-pi ratio.
            return static_cast<Ty>(units::convert<Units, unit<std::ratio<1>, units::category::scalar_unit>>((*this)()));
        }
    };

    template<typename T>
    struct linear_scale
    {
        inline constexpr T operator()() const noexcept { return m_value; }
        T m_value;
    };

    template<typename T>
    struct decibel_scale
    {
        inline constexpr T operator()() const noexcept { return 10 * std::log10(m_value); }
        T m_value;
    };

As a result, the problem is clear: the functions using operator()() to
obtain a value to pass to std::math functions are incorrect and should
be changed to use value() so relevant information is not thrown away.

The functions changed in this commit are the functions grouped under the
"TRANSCEDENTAL FUNCTIONS" header in the header file. This is why modf()
is changed here despite not being a transcedental function. Some other
functions suffer from a similar issue, but those will be addressed in
different commits.

* Fix incorrect trig function output for scaled dimensionless types

Similar to the previous commit, but for certain trigonometric functions.
I don't think that explicitly using value() is strictly necessary for
the other trig functions since they already use convert<>() to
normalize, so I don't think normalization through value() is necessary.
I have not tested it, though.

Not entirely sure the modifications to the test cases are the best way
to make the desired changes. The idea is to use the same inputs/outputs,
but to change the unit type to have a conversion factor not equal to 1.

Co-authored-by: Alex Wang <[email protected]>
ts826848 pushed a commit to ts826848/units that referenced this issue Oct 6, 2022
…types

This is a port of 47de749 to the v3.x
branch. The v3.x branch has some differences, though, which are detailed
below.

The first difference of note is that the implementation of value() has
changed. Instead of essentially being to<>() with the template parameter
hard-coded to underlying_type, value() now discards type information and
returns the raw underlying value. For example, this program:

    #include <cmath>
    #include <iostream>
    #include <units.h>
    using namespace units::literals;
    int main()
    {
        const auto c = 5.0_m * (2.0 / 1000.0_mm);
        std::cout << "c.to<double>() : " << c.to<double>() << std::endl;
        std::cout << "c.value()      : " << c.value() << std::endl;
        return EXIT_SUCCESS;
    }

Produces the following output in the v2.x branch:

  c.to<double>() : 10
  c.value()      : 10

But produces the following output in the v3.x branch:

  c.to<double>() : 10
  c.value()      : 0.01

The way that value() drops type information is apparent when
inspecting the value of c under a debugger:

    (lldb) p c
    (const units::unit<units::conversion_factor<std::ratio<1000, 1>, units::dimension_t<>, std::ratio<0, 1>, std::ratio<0, 1> >, double, units::linear_scale>) $0 = (linearized_value = 0.01)

In any case, the bug displayed in issue nholthaus#284 is present in the v3.x
branch, even if to<>() is used instead of value(). This program:

    #include <cmath>
    #include <iostream>
    #include <units.h>
    using namespace units::literals;
    int main()
    {
        const auto c = 5.0_m * (2.0 / 1000.0_mm);
        std::cout << "c                        : " << c << std::endl;
        std::cout << "c.to<double>()           : " << c.to<double>() << std::endl;
        std::cout << "units::exp(c)            : " << units::exp(c) << std::endl;
        std::cout << "std::exp(c.to<double>()) : " << std::exp(c.to<double>()) << std::endl;
        return EXIT_SUCCESS;
    }

Produces the following output:

    c                        : 10
    c.to<double>()           : 10
    units::exp(c)            : 1010.05
    std::exp(c.to<double>()) : 22026.5

This leads into the second difference. The transcedental functions no
longer use operator()(), which appears to have been removed; instead,
they use value() to obtain the value to pass to the standard library
math function:

    template<class dimensionlessUnit, std::enable_if_t<traits::is_dimensionless_unit_v<dimensionlessUnit>, int> = 0>
    detail::floating_point_promotion_t<dimensionlessUnit> exp(const dimensionlessUnit x) noexcept
    {
        return std::exp(x.value());
    }

As detailed above, the use of value() results in an incorrect value
being passed to the underlying function. Fixing this is simple, albeit
somewhat verbose - use to<underlying_type>() instead of value() to
ensure information in the value's type is taken into account.

    template<class dimensionlessUnit, std::enable_if_t<traits::is_dimensionless_unit_v<dimensionlessUnit>, int> = 0>
    detail::floating_point_promotion_t<dimensionlessUnit> exp(const dimensionlessUnit x) noexcept
    {
        return std::exp(x.template to<typename dimensionlessUnit::underlying_type>());
    }

(This change can be simplified and/or obviated if value() is changed to
not discard information in the type.)

However, unlike in the v2.x branch, this change is not sufficient to
obtain the expected results:

    c                        : 10
    c.to<double>()           : 10
    units::exp(c)            : 2.20265e+07
    std::exp(c.to<double>()) : 22026.5

This appears to be due to the change to the return type. In the v2.x
branch, the transcedental functions returned dimensionless::scalar_t,
which I believe is equivalent to dimensionless<> in the v3.x branch.
However, in the v3.x branch these functions all return
floating_point_promotion_t<dimensionlessUnit>, which is more or less the
argument's unit with a (potentially) promoted underlying_type. This,
however, happens to preserve the argument unit's conversion factor.
When the conversion factor is not 1, this appears to result in the
output of the underlying standard library function being scaled,
resulting in an incorrect output. Changing the return type to be a
promoted dimensionless type with a conversion factor of 1 appears to
solve the issue, though it does add a fair amount of clutter:

    template<class dimensionlessUnit, std::enable_if_t<traits::is_dimensionless_unit_v<dimensionlessUnit>, int> = 0>
    dimensionless<detail::floating_point_promotion_t<typename dimensionlessUnit::underlying_type>> exp(
    {
        return std::exp(x.template to<typename dimensionlessUnit::underlying_type>());
    }

This results in the expected output:

    c                        : 10
    c.to<double>()           : 10
    units::exp(c)            : 22026.5
    std::exp(c.to<double>()) : 22026.5
ts826848 pushed a commit to ts826848/units that referenced this issue Oct 6, 2022
…types

This is a port of 47de749 to the v3.x
branch. The v3.x branch has some differences, though, which are detailed
below.

The first difference of note is that the implementation of value() has
changed. Instead of essentially being to<>() with the template parameter
hard-coded to underlying_type, value() now discards type information and
returns the raw underlying value. For example, this program:

    #include <cmath>
    #include <iostream>
    #include <units.h>
    using namespace units::literals;
    int main()
    {
        const auto c = 5.0_m * (2.0 / 1000.0_mm);
        std::cout << "c.to<double>() : " << c.to<double>() << std::endl;
        std::cout << "c.value()      : " << c.value() << std::endl;
        return EXIT_SUCCESS;
    }

Produces the following output in the v2.x branch:

    c.to<double>() : 10
    c.value()      : 10

But produces the following output in the v3.x branch:

    c.to<double>() : 10
    c.value()      : 0.01

The way that value() drops type information is apparent when
inspecting the value of c under a debugger:

    (lldb) p c
    (const units::unit<units::conversion_factor<std::ratio<1000, 1>, units::dimension_t<>, std::ratio<0, 1>, std::ratio<0, 1> >, double, units::linear_scale>) $0 = (linearized_value = 0.01)

In any case, the bug displayed in issue nholthaus#284 is present in the v3.x
branch, even if to<>() is used instead of value(). This program:

    #include <cmath>
    #include <iostream>
    #include <units.h>
    using namespace units::literals;
    int main()
    {
        const auto c = 5.0_m * (2.0 / 1000.0_mm);
        std::cout << "c                        : " << c << std::endl;
        std::cout << "c.to<double>()           : " << c.to<double>() << std::endl;
        std::cout << "units::exp(c)            : " << units::exp(c) << std::endl;
        std::cout << "std::exp(c.to<double>()) : " << std::exp(c.to<double>()) << std::endl;
        return EXIT_SUCCESS;
    }

Produces the following output:

    c                        : 10
    c.to<double>()           : 10
    units::exp(c)            : 1010.05
    std::exp(c.to<double>()) : 22026.5

This leads into the second difference. The transcedental functions no
longer use operator()(), which appears to have been removed; instead,
they use value() to obtain the value to pass to the standard library
math function:

    template<class dimensionlessUnit, std::enable_if_t<traits::is_dimensionless_unit_v<dimensionlessUnit>, int> = 0>
    detail::floating_point_promotion_t<dimensionlessUnit> exp(const dimensionlessUnit x) noexcept
    {
        return std::exp(x.value());
    }

As detailed above, the use of value() results in an incorrect value
being passed to the underlying function. Fixing this is simple, albeit
somewhat verbose - use to<underlying_type>() instead of value() to
ensure information in the value's type is taken into account.

    template<class dimensionlessUnit, std::enable_if_t<traits::is_dimensionless_unit_v<dimensionlessUnit>, int> = 0>
    detail::floating_point_promotion_t<dimensionlessUnit> exp(const dimensionlessUnit x) noexcept
    {
        return std::exp(x.template to<typename dimensionlessUnit::underlying_type>());
    }

(This change can be simplified and/or obviated if value() is changed to
not discard information in the type.)

However, unlike in the v2.x branch, this change is not sufficient to
obtain the expected results:

    c                        : 10
    c.to<double>()           : 10
    units::exp(c)            : 2.20265e+07
    std::exp(c.to<double>()) : 22026.5

This appears to be due to the change to the return type. In the v2.x
branch, the transcedental functions returned dimensionless::scalar_t,
which I believe is equivalent to dimensionless<> in the v3.x branch.
However, in the v3.x branch these functions all return
floating_point_promotion_t<dimensionlessUnit>, which is more or less the
argument's unit with a (potentially) promoted underlying_type. This,
however, happens to preserve the argument unit's conversion factor.
When the conversion factor is not 1, this appears to result in the
output of the underlying standard library function being scaled,
resulting in an incorrect output. Changing the return type to be a
promoted dimensionless type with a conversion factor of 1 appears to
solve the issue, though it does add a fair amount of clutter:

    template<class dimensionlessUnit, std::enable_if_t<traits::is_dimensionless_unit_v<dimensionlessUnit>, int> = 0>
    dimensionless<detail::floating_point_promotion_t<typename dimensionlessUnit::underlying_type>> exp(
    {
        return std::exp(x.template to<typename dimensionlessUnit::underlying_type>());
    }

This results in the expected output:

    c                        : 10
    c.to<double>()           : 10
    units::exp(c)            : 22026.5
    std::exp(c.to<double>()) : 22026.5
nholthaus pushed a commit that referenced this issue Oct 7, 2022
…types

This is a port of 47de749 to the v3.x
branch. The v3.x branch has some differences, though, which are detailed
below.

The first difference of note is that the implementation of value() has
changed. Instead of essentially being to<>() with the template parameter
hard-coded to underlying_type, value() now discards type information and
returns the raw underlying value. For example, this program:

    #include <cmath>
    #include <iostream>
    #include <units.h>
    using namespace units::literals;
    int main()
    {
        const auto c = 5.0_m * (2.0 / 1000.0_mm);
        std::cout << "c.to<double>() : " << c.to<double>() << std::endl;
        std::cout << "c.value()      : " << c.value() << std::endl;
        return EXIT_SUCCESS;
    }

Produces the following output in the v2.x branch:

    c.to<double>() : 10
    c.value()      : 10

But produces the following output in the v3.x branch:

    c.to<double>() : 10
    c.value()      : 0.01

The way that value() drops type information is apparent when
inspecting the value of c under a debugger:

    (lldb) p c
    (const units::unit<units::conversion_factor<std::ratio<1000, 1>, units::dimension_t<>, std::ratio<0, 1>, std::ratio<0, 1> >, double, units::linear_scale>) $0 = (linearized_value = 0.01)

In any case, the bug displayed in issue #284 is present in the v3.x
branch, even if to<>() is used instead of value(). This program:

    #include <cmath>
    #include <iostream>
    #include <units.h>
    using namespace units::literals;
    int main()
    {
        const auto c = 5.0_m * (2.0 / 1000.0_mm);
        std::cout << "c                        : " << c << std::endl;
        std::cout << "c.to<double>()           : " << c.to<double>() << std::endl;
        std::cout << "units::exp(c)            : " << units::exp(c) << std::endl;
        std::cout << "std::exp(c.to<double>()) : " << std::exp(c.to<double>()) << std::endl;
        return EXIT_SUCCESS;
    }

Produces the following output:

    c                        : 10
    c.to<double>()           : 10
    units::exp(c)            : 1010.05
    std::exp(c.to<double>()) : 22026.5

This leads into the second difference. The transcedental functions no
longer use operator()(), which appears to have been removed; instead,
they use value() to obtain the value to pass to the standard library
math function:

    template<class dimensionlessUnit, std::enable_if_t<traits::is_dimensionless_unit_v<dimensionlessUnit>, int> = 0>
    detail::floating_point_promotion_t<dimensionlessUnit> exp(const dimensionlessUnit x) noexcept
    {
        return std::exp(x.value());
    }

As detailed above, the use of value() results in an incorrect value
being passed to the underlying function. Fixing this is simple, albeit
somewhat verbose - use to<underlying_type>() instead of value() to
ensure information in the value's type is taken into account.

    template<class dimensionlessUnit, std::enable_if_t<traits::is_dimensionless_unit_v<dimensionlessUnit>, int> = 0>
    detail::floating_point_promotion_t<dimensionlessUnit> exp(const dimensionlessUnit x) noexcept
    {
        return std::exp(x.template to<typename dimensionlessUnit::underlying_type>());
    }

(This change can be simplified and/or obviated if value() is changed to
not discard information in the type.)

However, unlike in the v2.x branch, this change is not sufficient to
obtain the expected results:

    c                        : 10
    c.to<double>()           : 10
    units::exp(c)            : 2.20265e+07
    std::exp(c.to<double>()) : 22026.5

This appears to be due to the change to the return type. In the v2.x
branch, the transcedental functions returned dimensionless::scalar_t,
which I believe is equivalent to dimensionless<> in the v3.x branch.
However, in the v3.x branch these functions all return
floating_point_promotion_t<dimensionlessUnit>, which is more or less the
argument's unit with a (potentially) promoted underlying_type. This,
however, happens to preserve the argument unit's conversion factor.
When the conversion factor is not 1, this appears to result in the
output of the underlying standard library function being scaled,
resulting in an incorrect output. Changing the return type to be a
promoted dimensionless type with a conversion factor of 1 appears to
solve the issue, though it does add a fair amount of clutter:

    template<class dimensionlessUnit, std::enable_if_t<traits::is_dimensionless_unit_v<dimensionlessUnit>, int> = 0>
    dimensionless<detail::floating_point_promotion_t<typename dimensionlessUnit::underlying_type>> exp(
    {
        return std::exp(x.template to<typename dimensionlessUnit::underlying_type>());
    }

This results in the expected output:

    c                        : 10
    c.to<double>()           : 10
    units::exp(c)            : 22026.5
    std::exp(c.to<double>()) : 22026.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants