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

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

Closed
chiphogg opened this issue Feb 10, 2023 · 4 comments
Closed

Comments

@chiphogg
Copy link
Collaborator

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.

@chiphogg chiphogg changed the title Surprising result from subtracting MPH and KPH Incorrect result, with surprising unit, when subtracting MPH and KPH Feb 20, 2023
@mpusz
Copy link
Owner

mpusz commented Jun 15, 2023

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.

@mpusz mpusz closed this as completed Jun 15, 2023
@chiphogg
Copy link
Collaborator Author

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

@mpusz
Copy link
Owner

mpusz commented Jun 15, 2023

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

@chiphogg
Copy link
Collaborator Author

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

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