Refine and document conversion factor policies (#176)

So far, we've applied our conversion factors by first breaking them into
three parts:

- `num`, the integer part of the numerator
- `den`, the integer part of the denominator
- `irr`, the "leftover" irrational parts, if any

We've applied it as `value * num / den * irr`.

This breakdown isn't really uniquely defined if `irr != 1`, of course.
But the idea was that this was "good enough to get started", and we
could count on the compiler to optimize things away in individual cases.

We now replace this with a more thoughtful, principled approach.  It
takes into account both the numeric category of conversion factor
(integer, irrational, etc.), and the type `T` of the variable we're
scaling.

This was motivated by looking at godbolt while making the slides for
CppCon 2023, and noticing that sometimes our conversion factors used two
instructions, where the equivalent "no units library" code used only
one.

The specific instance was a "crystal clear" poor case converting between
revolutions and radians, where we applied the irrational "two pi" as a
separate `2` and `pi`.  I also investigated the "harder" case of a
_rational_ factor applied to floating point, and decided that a single
instruction made sense here too.

I also included a detailed discussion doc explaining these rules and
tradeoffs.  It's an "implementation" level doc, so it gets pretty into
the weeds, but I think certain users will find it really interesting.

Fixes #173.

au/BUILD.bazel

@@ -105,6 +105,24 @@ cc_test(
 ################################################################################
 # Implementation detail libraries and tests
 
+cc_library(
+    name = "apply_magnitude",
+    hdrs = ["apply_magnitude.hh"],
+    deps = [":magnitude"],
+)
+
+cc_test(
+    name = "apply_magnitude_test",
+    size = "small",
+    srcs = ["apply_magnitude_test.cc"],
+    copts = ["-Iexternal/gtest/include"],
+    deps = [
+        ":apply_magnitude",
+        ":testing",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
 cc_library(
     name = "chrono_policy_validation",
     testonly = True,
@@ -304,6 +322,7 @@ cc_library(
     name = "quantity",
     hdrs = ["quantity.hh"],
     deps = [
+        ":apply_magnitude",
         ":conversion_policy",
         ":operators",
         ":unit_of_measure",

au/apply_magnitude.hh

@@ -0,0 +1,115 @@
+// Copyright 2023 Aurora Operations, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#pragma once
+
+#include "au/magnitude.hh"
+
+namespace au {
+namespace detail {
+
+// The various categories by which a magnitude can be applied to a numeric quantity.
+enum class ApplyAs {
+    INTEGER_MULTIPLY,
+    INTEGER_DIVIDE,
+    RATIONAL_MULTIPLY,
+    IRRATIONAL_MULTIPLY,
+};
+
+template <typename... BPs>
+constexpr ApplyAs categorize_magnitude(Magnitude<BPs...>) {
+    if (IsInteger<Magnitude<BPs...>>::value) {
+        return ApplyAs::INTEGER_MULTIPLY;
+    }
+
+    if (IsInteger<MagInverseT<Magnitude<BPs...>>>::value) {
+        return ApplyAs::INTEGER_DIVIDE;
+    }
+
+    return IsRational<Magnitude<BPs...>>::value ? ApplyAs::RATIONAL_MULTIPLY
+                                                : ApplyAs::IRRATIONAL_MULTIPLY;
+}
+
+template <typename Mag, ApplyAs Category, typename T, bool is_T_integral>
+struct ApplyMagnitudeImpl;
+
+// Multiplying by an integer, for any type T.
+template <typename Mag, typename T, bool is_T_integral>
+struct ApplyMagnitudeImpl<Mag, ApplyAs::INTEGER_MULTIPLY, T, is_T_integral> {
+    static_assert(categorize_magnitude(Mag{}) == ApplyAs::INTEGER_MULTIPLY,
+                  "Mismatched instantiation (should never be done manually)");
+    static_assert(is_T_integral == std::is_integral<T>::value,
+                  "Mismatched instantiation (should never be done manually)");
+
+    constexpr T operator()(const T &x) { return x * get_value<T>(Mag{}); }
+};
+
+// Dividing by an integer, for any type T.
+template <typename Mag, typename T, bool is_T_integral>
+struct ApplyMagnitudeImpl<Mag, ApplyAs::INTEGER_DIVIDE, T, is_T_integral> {
+    static_assert(categorize_magnitude(Mag{}) == ApplyAs::INTEGER_DIVIDE,
+                  "Mismatched instantiation (should never be done manually)");
+    static_assert(is_T_integral == std::is_integral<T>::value,
+                  "Mismatched instantiation (should never be done manually)");
+
+    constexpr T operator()(const T &x) { return x / get_value<T>(MagInverseT<Mag>{}); }
+};
+
+// Applying a (non-integer, non-inverse-integer) rational, for any integral type T.
+template <typename Mag, typename T>
+struct ApplyMagnitudeImpl<Mag, ApplyAs::RATIONAL_MULTIPLY, T, true> {
+    static_assert(categorize_magnitude(Mag{}) == ApplyAs::RATIONAL_MULTIPLY,
+                  "Mismatched instantiation (should never be done manually)");
+    static_assert(std::is_integral<T>::value,
+                  "Mismatched instantiation (should never be done manually)");
+
+    constexpr T operator()(const T &x) {
+        return x * get_value<T>(numerator(Mag{})) / get_value<T>(denominator(Mag{}));
+    }
+};
+
+// Applying a (non-integer, non-inverse-integer) rational, for any non-integral type T.
+template <typename Mag, typename T>
+struct ApplyMagnitudeImpl<Mag, ApplyAs::RATIONAL_MULTIPLY, T, false> {
+    static_assert(categorize_magnitude(Mag{}) == ApplyAs::RATIONAL_MULTIPLY,
+                  "Mismatched instantiation (should never be done manually)");
+    static_assert(!std::is_integral<T>::value,
+                  "Mismatched instantiation (should never be done manually)");
+
+    constexpr T operator()(const T &x) { return x * get_value<T>(Mag{}); }
+};
+
+// Applying an irrational for any type T (although only non-integral T makes sense).
+template <typename Mag, typename T, bool is_T_integral>
+struct ApplyMagnitudeImpl<Mag, ApplyAs::IRRATIONAL_MULTIPLY, T, is_T_integral> {
+    static_assert(!std::is_integral<T>::value, "Cannot apply irrational magnitude to integer type");
+
+    static_assert(categorize_magnitude(Mag{}) == ApplyAs::IRRATIONAL_MULTIPLY,
+                  "Mismatched instantiation (should never be done manually)");
+    static_assert(is_T_integral == std::is_integral<T>::value,
+                  "Mismatched instantiation (should never be done manually)");
+
+    constexpr T operator()(const T &x) { return x * get_value<T>(Mag{}); }
+};
+
+template <typename T, typename... BPs>
+constexpr T apply_magnitude(const T &x, Magnitude<BPs...> m) {
+    return ApplyMagnitudeImpl<Magnitude<BPs...>,
+                              categorize_magnitude(m),
+                              T,
+                              std::is_integral<T>::value>{}(x);
+}
+
+}  // namespace detail
+}  // namespace au

au/apply_magnitude_test.cc

@@ -0,0 +1,132 @@
+// Copyright 2023 Aurora Operations, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "au/apply_magnitude.hh"
+
+#include "au/testing.hh"
+#include "gtest/gtest.h"
+
+using ::testing::ElementsAreArray;
+using ::testing::Not;
+
+namespace au {
+namespace detail {
+namespace {
+template <typename T>
+std::vector<T> first_n_positive_values(std::size_t n) {
+    std::vector<T> result;
+    result.reserve(n);
+    for (auto i = 1u; i <= n; ++i) {
+        result.push_back(static_cast<T>(i));
+    }
+    return result;
+}
+}  // namespace
+
+TEST(CategorizeMagnitude, FindsIntegerMultiplyInstances) {
+    EXPECT_EQ(categorize_magnitude(mag<2>()), ApplyAs::INTEGER_MULTIPLY);
+    EXPECT_EQ(categorize_magnitude(mag<35>()), ApplyAs::INTEGER_MULTIPLY);
+
+    EXPECT_EQ(categorize_magnitude(mag<35>() / mag<7>()), ApplyAs::INTEGER_MULTIPLY);
+}
+
+TEST(CategorizeMagnitude, FindsIntegerDivideInstances) {
+    EXPECT_EQ(categorize_magnitude(ONE / mag<2>()), ApplyAs::INTEGER_DIVIDE);
+    EXPECT_EQ(categorize_magnitude(ONE / mag<35>()), ApplyAs::INTEGER_DIVIDE);
+
+    EXPECT_EQ(categorize_magnitude(mag<7>() / mag<35>()), ApplyAs::INTEGER_DIVIDE);
+}
+
+TEST(CategorizeMagnitude, FindsRationalMultiplyInstances) {
+    EXPECT_EQ(categorize_magnitude(mag<5>() / mag<2>()), ApplyAs::RATIONAL_MULTIPLY);
+}
+
+TEST(CategorizeMagnitude, FindsIrrationalMultiplyInstances) {
+    EXPECT_EQ(categorize_magnitude(sqrt(mag<2>())), ApplyAs::IRRATIONAL_MULTIPLY);
+    EXPECT_EQ(categorize_magnitude(PI), ApplyAs::IRRATIONAL_MULTIPLY);
+}
+
+TEST(ApplyMagnitude, MultipliesForIntegerMultiply) {
+    constexpr auto m = mag<25>();
+    ASSERT_EQ(categorize_magnitude(m), ApplyAs::INTEGER_MULTIPLY);
+
+    EXPECT_THAT(apply_magnitude(4, m), SameTypeAndValue(100));
+    EXPECT_THAT(apply_magnitude(4.0f, m), SameTypeAndValue(100.0f));
+}
+
+TEST(ApplyMagnitude, DividesForIntegerDivide) {
+    constexpr auto one_thirteenth = ONE / mag<13>();
+    ASSERT_EQ(categorize_magnitude(one_thirteenth), ApplyAs::INTEGER_DIVIDE);
+
+    // This test would fail if our implementation multiplied by the float representation of (1/13),
+    // instead of dividing by 13, under the hood.
+    for (const auto &i : first_n_positive_values<float>(100u)) {
+        EXPECT_THAT(apply_magnitude(i * 13, one_thirteenth), SameTypeAndValue(i));
+    }
+}
+
+TEST(ApplyMagnitude, MultipliesThenDividesForRationalMagnitudeOnInteger) {
+    // Consider applying the magnitude (3/2) to the value 5.  The exact answer is the real number
+    // 7.5, which becomes 7 when translated (via truncation) to the integer domain.
+    //
+    // If we multiply-then-divide, we get (5 * 3) / 2 = 7, which is correct.
+    //
+    // If we divide-then-multiply --- say, because we are trying to avoid overflow --- then we get
+    // (5 / 2) * 3 = 2 * 3 = 6, which is wrong.
+    constexpr auto three_halves = mag<3>() / mag<2>();
+    ASSERT_EQ(categorize_magnitude(three_halves), ApplyAs::RATIONAL_MULTIPLY);
+
+    EXPECT_THAT(apply_magnitude(5, three_halves), SameTypeAndValue(7));
+}
+
+TEST(ApplyMagnitude, MultipliesSingleNumberForRationalMagnitudeOnFloatingPoint) {
+    // Helper similar to `std::transform`, but with more convenient interfaces.
+    auto apply = [](std::vector<float> vals, auto fun) {
+        for (auto &v : vals) {
+            v = fun(v);
+        }
+        return vals;
+    };
+
+    // Create our rational magnitude, (2 / 13).
+    constexpr auto two_thirteenths = mag<2>() / mag<13>();
+    ASSERT_EQ(categorize_magnitude(two_thirteenths), ApplyAs::RATIONAL_MULTIPLY);
+
+    // Test a bunch of values.  We are hoping that the two different strategies will yield different
+    // results for at least some of these strategies (and we'll check that this is the case).
+    const auto original_vals = first_n_positive_values<float>(10u);
+
+    // Compute expected answers for each possible strategy.
+    const auto if_we_multiply_and_divide =
+        apply(original_vals, [](float v) { return v * 2.0f / 13.0f; });
+    const auto if_we_use_one_factor =
+        apply(original_vals, [](float v) { return v * (2.0f / 13.0f); });
+
+    // The strategies must be different for at least some results!
+    ASSERT_THAT(if_we_multiply_and_divide, Not(ElementsAreArray(if_we_use_one_factor)));
+
+    // Make sure we follow the single-number strategy, every time.
+    const auto results =
+        apply(original_vals, [=](float v) { return apply_magnitude(v, two_thirteenths); });
+    EXPECT_THAT(results, ElementsAreArray(if_we_use_one_factor));
+    EXPECT_THAT(results, Not(ElementsAreArray(if_we_multiply_and_divide)));
+}
+
+TEST(ApplyMagnitude, MultipliesSingleNumberForIrrationalMagnitudeOnFloatingPoint) {
+    ASSERT_EQ(categorize_magnitude(PI), ApplyAs::IRRATIONAL_MULTIPLY);
+    EXPECT_THAT(apply_magnitude(2.0f, PI), SameTypeAndValue(2.0f * static_cast<float>(M_PI)));
+}
+
+}  // namespace detail
+}  // namespace au

au/quantity.hh

@@ -16,6 +16,7 @@
 
 #include <utility>
 
+#include "au/apply_magnitude.hh"
 #include "au/conversion_policy.hh"
 #include "au/operators.hh"
 #include "au/stdx/functional.hh"
@@ -143,17 +144,11 @@ class Quantity {
               typename NewUnit,
               typename = std::enable_if_t<IsUnit<NewUnit>::value>>
     constexpr auto as(NewUnit) const {
-        constexpr auto ratio = unit_ratio(unit, NewUnit{});
-
         using Common = std::common_type_t<Rep, NewRep>;
-        constexpr auto NUM = integer_part(numerator(ratio));
-        constexpr auto DEN = integer_part(denominator(ratio));
-        constexpr auto num = get_value<Common>(NUM);
-        constexpr auto den = get_value<Common>(DEN);
-        constexpr auto irr = get_value<Common>(ratio * DEN / NUM);
+        using Factor = UnitRatioT<Unit, NewUnit>;
 
         return make_quantity<NewUnit>(
-            static_cast<NewRep>(static_cast<Common>(value_) * num / den * irr));
+            static_cast<NewRep>(detail::apply_magnitude(static_cast<Common>(value_), Factor{})));
     }
 
     template <typename NewUnit, typename = std::enable_if_t<IsUnit<NewUnit>::value>>

au/quantity_test.cc

@@ -94,6 +94,14 @@ TEST(MakeQuantity, MakesQuantityInGivenUnit) {
     EXPECT_EQ(make_quantity<Feet>(99), feet(99));
 }
 
+TEST(Quantity, RationalConversionRecoversExactIntegerValues) {
+    // This test would fail if our implementation multiplied by the float
+    // representation of (1/13), instead of dividing by 13, under the hood.
+    for (int i = 1; i < 100; ++i) {
+        EXPECT_EQ(feet(static_cast<float>(i * 13)).in(feet * mag<13>()), i);
+    }
+}
+
 TEST(QuantityMaker, CreatesAppropriateQuantityIfCalled) { EXPECT_EQ(yards(3.14).in(yards), 3.14); }
 
 TEST(QuantityMaker, CanBeMultipliedBySingularUnitToGetMakerOfProductUnit) {