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

Fix incorrect math function output for scaled dimensionless types, v3.x edition #295

Merged
merged 2 commits into from
Oct 7, 2022

Conversation

ts826848
Copy link
Contributor

@ts826848 ts826848 commented Oct 6, 2022

More or less a port of #288 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, 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

Alex Wang added 2 commits October 6, 2022 19:51
…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
This is a port of the analogous changes made to the v2.x branch.

Similar to the previous commit, but for certain trigonometric functions.
I don't think that explicitly calling to<>() is strictly necessary for
the other trig functions since they already use convert<>() to
normalize, so I don't think normalization through to<>() 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.
@ts826848 ts826848 force-pushed the v3.x-fix-math-issue-284 branch from 8f7949b to 9f966ca Compare October 6, 2022 23:52
@ts826848
Copy link
Contributor Author

ts826848 commented Oct 7, 2022

Took a quick glance at the CI failures - seems it's stuff unrelated to this PR? There's a runtime library mismatch, some duplicate iostream symbols, and what appears to be some locale-related stuff?

@nholthaus
Copy link
Owner

Thanks for the work on this and other PRs! The thoroughness is incredibly helpful. I really can't overstate how much I appreciate

The CI hasn't been updated since 2020, I need to give it some TLC today. This can be merged in the meantime.

I had originally intended for value() and to<double>() to do the same thing and have the same result, and I'd expect the same behavior you would. I'm not sure if somewhere in the 3.x refactors it became necessary for value() to provide direct access to the raw storage and if some other behavior depends on it (decibels, etc), but I'd like to think it's a bug. I'll have to investigate a bit.

The return types definitely need to be dimensionless units as you discovered, otherwise there's nothing to hold the conversion factors.

@nholthaus nholthaus merged commit 9e6d64b into nholthaus:v3.x Oct 7, 2022
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

Successfully merging this pull request may close these issues.

2 participants