nholthaus / units

a compile-time, header-only, dimensional analysis and unit conversion library built on c++14 with no dependencies.
http://nholthaus.github.io/units/
MIT License
934 stars 134 forks source link

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

Closed ts826848 closed 1 year ago

ts826848 commented 1 year ago

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 https://github.com/nholthaus/units/issues/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() 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
ts826848 commented 1 year ago

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 commented 1 year ago

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.