From d7681e188e5298c37b46d26656db0bd46a929d73 Mon Sep 17 00:00:00 2001 From: Chip Hogg Date: Mon, 21 Feb 2022 01:44:26 +0000 Subject: [PATCH 1/7] Support seamless interop between `ratio` and rational `Magnitude` We provide two new functions, `numerator(m)` and `denominator(m)`, for a Magnitude `m`. They fulfill the following conditions: 1. `numerator(m)` and `denominator(m)` are always integer Magnitudes. 2. If `m` is rational, then `m == numerator(m) / denominator(m)`. If `m` is _not_ rational, then the numerator and denominator are not especially meaningful (there is no uniquely defined "leftover irrational part"). However, we choose a convention that matches how humans would write a mixed number. For example, sqrt(27/16) would have a numerator of 3, denominator of 4, and a "leftover part" of sqrt(3), matching the "human" way of writing this as [(3 * sqrt(3)) / 4]. This has no use yet, but it may later be useful in printing the Magnitude of an anonymous Unit for end users. To further reduce friction for the upcoming migration, we provide an implicit conversion from a Magnitude to a `ratio`. We restrict this operation to rational Magnitudes, and guard this with a `static_assert`. --- src/core/include/units/magnitude.h | 52 ++++++++++++++- src/core/include/units/ratio.h | 19 ++++++ test/unit_test/runtime/magnitude_test.cpp | 78 +++++++++++++++++++++++ test/unit_test/static/ratio_test.cpp | 6 ++ 4 files changed, 154 insertions(+), 1 deletion(-) diff --git a/src/core/include/units/magnitude.h b/src/core/include/units/magnitude.h index 94140397..eae55206 100644 --- a/src/core/include/units/magnitude.h +++ b/src/core/include/units/magnitude.h @@ -368,6 +368,9 @@ struct magnitude { // Whether this magnitude represents a rational number. friend constexpr bool is_rational(const magnitude&) { return (detail::is_rational(BPs) && ...); } + + // Implicit conversion to ratio. + constexpr explicit(false) operator ratio() const; }; // Implementation for Magnitude concept (below). @@ -392,7 +395,7 @@ template constexpr T get_value(const magnitude&) { // Force the expression to be evaluated in a constexpr context, to catch, e.g., overflow. - constexpr auto result = detail::checked_static_cast((detail::compute_base_power(BPs) * ...)); + constexpr auto result = detail::checked_static_cast((detail::compute_base_power(BPs) * ... * 1)); return result; } @@ -480,6 +483,53 @@ constexpr auto operator*(magnitude, magnitude) constexpr auto operator/(Magnitude auto l, Magnitude auto r) { return l * pow<-1>(r); } +//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// +// Magnitude numerator and denominator implementation. + +namespace detail { + +// The largest integer which can be extracted from any magnitude with only a single basis vector. +template +constexpr auto integer_part(magnitude) +{ + constexpr auto power_num = numerator(BP.power); + constexpr auto power_den = denominator(BP.power); + + if constexpr (std::is_integral_v && (power_num >= power_den)) { + constexpr auto largest_integer_power = [power_num, power_den](BasePower auto bp) { + bp.power = (power_num / power_den); // Note: integer division intended. + return bp; + }(BP); // Note: lambda is immediately invoked. + + return magnitude{}; + } else { + return magnitude<>{}; + } +} + +} // namespace detail + +template +constexpr auto numerator(magnitude) +{ + return (detail::integer_part(magnitude{}) * ... * magnitude<>{}); +} + +constexpr auto denominator(Magnitude auto m) { return numerator(pow<-1>(m)); } + +// Implementation of implicit conversion to ratio goes here, because it needs `numerator()` and `denominator()`. +template +constexpr magnitude::operator ratio() const +{ + static_assert(is_rational(magnitude{})); + + return ratio{ + get_value(numerator(*this)), + get_value(denominator(*this)), + }; +} + + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // `as_magnitude()` implementation. diff --git a/src/core/include/units/ratio.h b/src/core/include/units/ratio.h index 1e35b47e..d892825a 100644 --- a/src/core/include/units/ratio.h +++ b/src/core/include/units/ratio.h @@ -87,8 +87,27 @@ struct ratio { } [[nodiscard]] friend constexpr ratio operator/(const ratio& lhs, const ratio& rhs) { return lhs * inverse(rhs); } + + [[nodiscard]] friend constexpr std::intmax_t numerator(const ratio& r) + { + std::intmax_t num = r.num; + for (auto i = r.exp; i > 0; --i) { + num *= 10; + } + return num; + } + + [[nodiscard]] friend constexpr std::intmax_t denominator(const ratio& r) + { + std::intmax_t den = r.den; + for (auto i = r.exp; i < 0; ++i) { + den *= 10; + } + return den; + } }; + [[nodiscard]] constexpr ratio inverse(const ratio& r) { return ratio(r.den, r.num, -r.exp); } [[nodiscard]] constexpr bool is_integral(const ratio& r) diff --git a/test/unit_test/runtime/magnitude_test.cpp b/test/unit_test/runtime/magnitude_test.cpp index 9db7b696..b7a75b65 100644 --- a/test/unit_test/runtime/magnitude_test.cpp +++ b/test/unit_test/runtime/magnitude_test.cpp @@ -63,6 +63,18 @@ void check_same_type_and_value(T actual, U expected) CHECK(actual == expected); } +template +void check_ratio_round_trip_is_identity() +{ + constexpr Magnitude auto m = as_magnitude(); + constexpr ratio round_trip = ratio{ + get_value(numerator(m)), + get_value(denominator(m)), + }; + CHECK(round_trip == R); +} + + TEST_CASE("base_power") { SECTION("base rep deducible for integral base") @@ -156,6 +168,15 @@ TEST_CASE("make_ratio performs prime factorization correctly") as_magnitude(); } + SECTION("Can handle prime number which would exceed GCC iteration limit") + { + // GCC 10 has a constexpr loop iteration limit of 262144. A naive algorithm, which performs trial division on 2 and + // all odd numbers up to sqrt(N), will exceed this limit for the following prime. Thus, for this test to pass, we + // need to be using a more efficient algorithm. (We could increase the limit, but we don't want users to have to + // mess with compiler flags just to compile the code.) + as_magnitude<334'524'384'739>(); + } + SECTION("Can bypass computing primes by providing known_first_factor") { // Sometimes, even wheel factorization isn't enough to handle the compilers' limits on constexpr steps and/or @@ -351,6 +372,35 @@ TEST_CASE("can distinguish integral, rational, and irrational magnitudes") } } +TEST_CASE("Constructing ratio from rational magnitude") +{ + SECTION("Round trip is identity") + { + // Note that not every Magnitude can be represented as a ratio. However, if we _start_ with a + // ratio, we must guarantee to recover the same ratio in a round trip. + check_ratio_round_trip_is_identity<1>(); + check_ratio_round_trip_is_identity<9>(); + check_ratio_round_trip_is_identity(); + } + + SECTION("Rational magnitude implicitly converts to ratio") + { + constexpr ratio r = as_magnitude(); + CHECK(r == ratio{22, 7}); + } + + SECTION("Irrational magnitude does not convert to ratio") + { + // The following code should not compile. + // constexpr ratio radical = pow(as_magnitude<2>()); + // (void)radical; + + // The following code should not compile. + // constexpr ratio degrees_per_radian = as_magnitude<180>() / pi_to_the<1>(); + // (void)degrees_per_radian; + } +} + //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // Detail function tests below. @@ -374,6 +424,34 @@ TEST_CASE("int_power computes integer powers") } } +TEST_CASE("integer_part picks out integer part of single-basis magnitude") +{ + SECTION("integer_part of non-integer base is identity magnitude") + { + CHECK(integer_part(pi_to_the<1>()) == magnitude<>{}); + CHECK(integer_part(pi_to_the<-8>()) == magnitude<>{}); + CHECK(integer_part(pi_to_the()) == magnitude<>{}); + } + + SECTION("integer_part of integer base to negative power is identity magnitude") + { + CHECK(integer_part(magnitude{}) == magnitude<>{}); + CHECK(integer_part(magnitude{}) == magnitude<>{}); + } + + SECTION("integer_part of integer base to fractional power is identity magnitude") + { + CHECK(integer_part(magnitude{}) == magnitude<>{}); + } + + SECTION("integer_part of integer base to power at least one takes integer part") + { + CHECK(integer_part(magnitude{}) == magnitude{}); + CHECK(integer_part(magnitude{}) == magnitude{}); + CHECK(integer_part(magnitude{}) == magnitude{}); + } +} + TEST_CASE("Prime helper functions") { SECTION("multiplicity") diff --git a/test/unit_test/static/ratio_test.cpp b/test/unit_test/static/ratio_test.cpp index 9eac9963..4abe5094 100644 --- a/test/unit_test/static/ratio_test.cpp +++ b/test/unit_test/static/ratio_test.cpp @@ -102,4 +102,10 @@ static_assert(common_ratio(ratio(100, 1), ratio(1, 10)) == ratio(1, 10)); static_assert(common_ratio(ratio(1), ratio(1, 1, 3)) == ratio(1)); static_assert(common_ratio(ratio(10, 1, -1), ratio(1, 1, -3)) == ratio(1, 1, -3)); +// numerator and denominator +static_assert(numerator(ratio(3, 4)) == 3); +static_assert(numerator(ratio(3, 7, 2)) == 300); +static_assert(denominator(ratio(3, 4)) == 4); +static_assert(denominator(ratio(3, 7, -2)) == 700); + } // namespace From 746aa34fccb20151327979113fbc7d0926c91f5d Mon Sep 17 00:00:00 2001 From: Chip Hogg Date: Wed, 6 Apr 2022 00:18:52 +0000 Subject: [PATCH 2/7] Tweak unwise variable names --- src/core/include/units/ratio.h | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/core/include/units/ratio.h b/src/core/include/units/ratio.h index d892825a..0fb2f428 100644 --- a/src/core/include/units/ratio.h +++ b/src/core/include/units/ratio.h @@ -90,24 +90,23 @@ struct ratio { [[nodiscard]] friend constexpr std::intmax_t numerator(const ratio& r) { - std::intmax_t num = r.num; + std::intmax_t true_num = r.num; for (auto i = r.exp; i > 0; --i) { - num *= 10; + true_num *= 10; } - return num; + return true_num; } [[nodiscard]] friend constexpr std::intmax_t denominator(const ratio& r) { - std::intmax_t den = r.den; + std::intmax_t true_den = r.den; for (auto i = r.exp; i < 0; ++i) { - den *= 10; + true_den *= 10; } - return den; + return true_den; } }; - [[nodiscard]] constexpr ratio inverse(const ratio& r) { return ratio(r.den, r.num, -r.exp); } [[nodiscard]] constexpr bool is_integral(const ratio& r) From 0f776b5b5bd8b5fe2297db423b53e44ab0b469e9 Mon Sep 17 00:00:00 2001 From: Chip Hogg Date: Wed, 6 Apr 2022 00:31:18 +0000 Subject: [PATCH 3/7] Remove incorrectly included test We had removed this upstream, but apparently it snuck back in from a merge/rebase conflict. --- test/unit_test/runtime/magnitude_test.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/unit_test/runtime/magnitude_test.cpp b/test/unit_test/runtime/magnitude_test.cpp index b7a75b65..038aff8a 100644 --- a/test/unit_test/runtime/magnitude_test.cpp +++ b/test/unit_test/runtime/magnitude_test.cpp @@ -168,15 +168,6 @@ TEST_CASE("make_ratio performs prime factorization correctly") as_magnitude(); } - SECTION("Can handle prime number which would exceed GCC iteration limit") - { - // GCC 10 has a constexpr loop iteration limit of 262144. A naive algorithm, which performs trial division on 2 and - // all odd numbers up to sqrt(N), will exceed this limit for the following prime. Thus, for this test to pass, we - // need to be using a more efficient algorithm. (We could increase the limit, but we don't want users to have to - // mess with compiler flags just to compile the code.) - as_magnitude<334'524'384'739>(); - } - SECTION("Can bypass computing primes by providing known_first_factor") { // Sometimes, even wheel factorization isn't enough to handle the compilers' limits on constexpr steps and/or From f77a92ca043a0b460fc7ff910ab008ed47fc90af Mon Sep 17 00:00:00 2001 From: Chip Hogg Date: Wed, 6 Apr 2022 00:36:18 +0000 Subject: [PATCH 4/7] Reproduce requires clause clang appears to care about this. --- src/core/include/units/magnitude.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/include/units/magnitude.h b/src/core/include/units/magnitude.h index eae55206..d65c9ad1 100644 --- a/src/core/include/units/magnitude.h +++ b/src/core/include/units/magnitude.h @@ -519,6 +519,7 @@ constexpr auto denominator(Magnitude auto m) { return numerator(pow<-1>(m)); } // Implementation of implicit conversion to ratio goes here, because it needs `numerator()` and `denominator()`. template + requires detail::is_base_power_pack_valid constexpr magnitude::operator ratio() const { static_assert(is_rational(magnitude{})); From b616e5821646db2d8995b0fc95c41205754a9315 Mon Sep 17 00:00:00 2001 From: Chip Hogg Date: Wed, 6 Apr 2022 01:40:17 +0000 Subject: [PATCH 5/7] Give up on implicit conversion I don't understand what MSVC is complaining about, and it may end up being easier to just make the conversion explicit. --- src/core/include/units/magnitude.h | 12 +++++------- test/unit_test/runtime/magnitude_test.cpp | 4 ++-- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/core/include/units/magnitude.h b/src/core/include/units/magnitude.h index d65c9ad1..7656a348 100644 --- a/src/core/include/units/magnitude.h +++ b/src/core/include/units/magnitude.h @@ -517,16 +517,14 @@ constexpr auto numerator(magnitude) constexpr auto denominator(Magnitude auto m) { return numerator(pow<-1>(m)); } -// Implementation of implicit conversion to ratio goes here, because it needs `numerator()` and `denominator()`. -template - requires detail::is_base_power_pack_valid -constexpr magnitude::operator ratio() const +// Implementation of conversion to ratio goes here, because it needs `numerator()` and `denominator()`. +constexpr ratio as_ratio(Magnitude auto m) { - static_assert(is_rational(magnitude{})); + static_assert(is_rational(m)); return ratio{ - get_value(numerator(*this)), - get_value(denominator(*this)), + get_value(numerator(m)), + get_value(denominator(m)), }; } diff --git a/test/unit_test/runtime/magnitude_test.cpp b/test/unit_test/runtime/magnitude_test.cpp index 038aff8a..22a3dd4b 100644 --- a/test/unit_test/runtime/magnitude_test.cpp +++ b/test/unit_test/runtime/magnitude_test.cpp @@ -374,9 +374,9 @@ TEST_CASE("Constructing ratio from rational magnitude") check_ratio_round_trip_is_identity(); } - SECTION("Rational magnitude implicitly converts to ratio") + SECTION("Rational magnitude converts to ratio") { - constexpr ratio r = as_magnitude(); + constexpr ratio r = as_ratio(as_magnitude()); CHECK(r == ratio{22, 7}); } From 2c31bf83ec65bfac05fc7184f1a95a259398ba32 Mon Sep 17 00:00:00 2001 From: Chip Hogg Date: Wed, 6 Apr 2022 02:01:23 +0000 Subject: [PATCH 6/7] Clean up --- src/core/include/units/magnitude.h | 5 +---- test/unit_test/runtime/magnitude_test.cpp | 6 ++---- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/core/include/units/magnitude.h b/src/core/include/units/magnitude.h index 7656a348..fe814dd2 100644 --- a/src/core/include/units/magnitude.h +++ b/src/core/include/units/magnitude.h @@ -368,9 +368,6 @@ struct magnitude { // Whether this magnitude represents a rational number. friend constexpr bool is_rational(const magnitude&) { return (detail::is_rational(BPs) && ...); } - - // Implicit conversion to ratio. - constexpr explicit(false) operator ratio() const; }; // Implementation for Magnitude concept (below). @@ -395,7 +392,7 @@ template constexpr T get_value(const magnitude&) { // Force the expression to be evaluated in a constexpr context, to catch, e.g., overflow. - constexpr auto result = detail::checked_static_cast((detail::compute_base_power(BPs) * ... * 1)); + constexpr auto result = detail::checked_static_cast((detail::compute_base_power(BPs) * ... * T{1})); return result; } diff --git a/test/unit_test/runtime/magnitude_test.cpp b/test/unit_test/runtime/magnitude_test.cpp index 22a3dd4b..e5d46f26 100644 --- a/test/unit_test/runtime/magnitude_test.cpp +++ b/test/unit_test/runtime/magnitude_test.cpp @@ -383,12 +383,10 @@ TEST_CASE("Constructing ratio from rational magnitude") SECTION("Irrational magnitude does not convert to ratio") { // The following code should not compile. - // constexpr ratio radical = pow(as_magnitude<2>()); - // (void)radical; + // as_ratio(pow(as_magnitude<2>())); // The following code should not compile. - // constexpr ratio degrees_per_radian = as_magnitude<180>() / pi_to_the<1>(); - // (void)degrees_per_radian; + // as_ratio(as_magnitude<180>() / pi_to_the<1>()); } } From 44cccfc743bde1810346bcd7263b9b7a96ce089c Mon Sep 17 00:00:00 2001 From: Chip Hogg Date: Sat, 9 Apr 2022 15:39:29 +0000 Subject: [PATCH 7/7] Address review comments --- src/core/include/units/magnitude.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/core/include/units/magnitude.h b/src/core/include/units/magnitude.h index fe814dd2..88d003e7 100644 --- a/src/core/include/units/magnitude.h +++ b/src/core/include/units/magnitude.h @@ -493,7 +493,7 @@ constexpr auto integer_part(magnitude) constexpr auto power_den = denominator(BP.power); if constexpr (std::is_integral_v && (power_num >= power_den)) { - constexpr auto largest_integer_power = [power_num, power_den](BasePower auto bp) { + constexpr auto largest_integer_power = [=](BasePower auto bp) { bp.power = (power_num / power_den); // Note: integer division intended. return bp; }(BP); // Note: lambda is immediately invoked. @@ -516,9 +516,8 @@ constexpr auto denominator(Magnitude auto m) { return numerator(pow<-1>(m)); } // Implementation of conversion to ratio goes here, because it needs `numerator()` and `denominator()`. constexpr ratio as_ratio(Magnitude auto m) + requires(is_rational(decltype(m){})) { - static_assert(is_rational(m)); - return ratio{ get_value(numerator(m)), get_value(denominator(m)),