mpusz / mp-units

The quantities and units library for C++
https://mpusz.github.io/mp-units/
MIT License
1.07k stars 85 forks source link

Incorrect result, with surprising unit, when subtracting MPH and KPH #438

Closed chiphogg closed 1 year ago

chiphogg commented 1 year ago

I wanted to understand how "common units" worked, so I created this (purposely failing) test case:

CHECK(UNITS_STD_FMT::format("{}", ((1.0 * (mi / h)) - (1.0 * (km / h)))) == "X");

The result:

/home/chogg/github/units/test/unit_test/runtime/fmt_units_test.cpp:89: FAILED:
  CHECK( fmt::format("{}", ((1.0 * (mi / h)) - (1.0 * (km / h)))) == "X" )
with expansion:
  "2399 [5/70866] m/s" == "X"

OK, so I assumed that [5/70866] m/s is the common unit of mi/h and km/h, expressed in m/s. (By "common unit," I mean the largest magnitude unit that evenly divides all input units.) But it isn't: the common unit turns out to be [1/56250] m/s.

So, first question: where did [5/70866] come from?

I also think the value is a little bit inaccurate. Here's some Au code to compute the value in those units (regardless of where they came from):

constexpr auto diff = (miles / hour)(1) - (kilo(meters) / hour)(1);
std::cout.precision(15);
std::cout << "Value: " << diff.in<double>(meters * mag<5>() / mag<70866>() / second)
          << " [5/70866] m/s\n";

(I made the unit label manually because we don't have printing for magnitudes yet; see aurora-opensource/au#85.)

Output:

Value: 2398.987328 [5/70866] m/s

Looks like a discrepancy of only about 5 parts in 10^6. So, maybe mp-units is making some approximation somewhere? I worried that Au was wrong, but I checked very carefully and I'm convinced it's right.

mpusz commented 1 year ago

In V2 the following:

#include <mp-units/iostream.h>
#include <mp-units/systems/international/international.h>
#include <mp-units/systems/si/si.h>
#include <iostream>

using namespace mp_units::si::unit_symbols;
using namespace mp_units::international::unit_symbols;

int main()
{
  std::cout << 1.0 * (mi / h) - (1.0 * (km / h));
}

prints:

9521 [1/5625 × 10⁻¹] m/s

so it seems that the issue is fixed.

chiphogg commented 1 year ago

I was really curious about what the root cause was going to be! Oh well, chalk it up to "V2 just makes everything better". :grin:

mpusz commented 1 year ago

I really do not know which change made it work. I rewrote nearly the entire code base.

chiphogg commented 1 year ago

Yep... once I saw the resolution, I realized I'd never know. :slightly_smiling_face: