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
955 stars 135 forks source link

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

Open nholthaus-units-user opened 2 years ago

nholthaus-units-user commented 2 years ago

I am using the units.h file as of commit ea6d126942cb3225a341568ab57ec52513977875. 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 commented 2 years ago

I suspect this behavior is unintended.

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

https://github.com/nholthaus/units/blob/ea6d126942cb3225a341568ab57ec52513977875/include/units.h#L4467-L4472

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:

https://github.com/nholthaus/units/blob/ea6d126942cb3225a341568ab57ec52513977875/include/units.h#L2472

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:

https://github.com/nholthaus/units/blob/ea6d126942cb3225a341568ab57ec52513977875/include/units.h#L2133-L2142

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 commented 2 years ago

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...