Eliminate redundant units from `CommonUnit<...>` #310

chiphogg · 2024-10-26T18:52:58Z

The core here is the addition to :quantity_test. Making this pass is
the goal of this PR.

To get there, we need a new trait/metafunction that can take a pack of
units, and eliminate all the "redundant" ones. This means we also need
a definition of "redundant", where the canonical example would be that
"feet" is redundant between "feet" and "inches". Here's what we came up
with:

If two units are identical (same type), then each is redundant
w.r.t. the other.
If two units are distinct, but quantity-equivalent, then the
redundant one is whichever comes later in the "standard pack
ordering". (We try to give more "recognizable" units higher
priority.)
Otherwise, for quantity-inequivalent units, a redundant unit is an
exact integer multiple of another unit.

All of this assumes that the units have the same dimension. We simply
wouldn't call this machinery in the first place if that weren't true.

Finally, we need one more piece of machinery: a simple "drop all X"
metafunction. This is because when an earlier unit makes later units
redundant, we can't easily "reach into" that "tail pack" of units and
remove them directly. Instead, we need to mark them with a "tombstone"
(void), and then remove all the tombstoned units.

Fixes #295.

The core here is the addition to `:quantity_test`. Making this pass is the goal of this PR. To get there, we need a new trait/metafunction that can take a pack of units, and eliminate all the "redundant" ones. This means we also need a definition of "redundant", where the canonical example would be that "feet" is redundant between "feet" and "inches". Here's what we came up with: - If two units are _identical_ (same type), then each is redundant w.r.t. the other. - If two units are _distinct_, but _quantity-equivalent_, then the redundant one is whichever comes later in the "standard pack ordering". (We try to give more "recognizable" units higher priority.) - Otherwise, for _quantity-inequivalent_ units, a redundant unit is an exact integer multiple of another unit. All of this assumes that the units have the same dimension. We simply wouldn't call this machinery in the first place if that weren't true. Finally, we need one more piece of machinery: a simple "drop all X" metafunction. This is because when an earlier unit makes later units redundant, we can't easily "reach into" that "tail pack" of units and remove them directly. Instead, we need to mark them with a "tombstone" (`void`), and then remove all the tombstoned units. Fixes #295.

chiphogg · 2024-10-26T18:55:06Z

au/code/au/quantity_test.cc

+TEST(Quantity, CommonUnitAlwaysCompletelyIndependentOfOrder) {
+    auto check_units = [](auto unit_a, auto unit_b, auto unit_c) {
+        const auto a = unit_a(1LL);
+        const auto b = unit_b(1LL);
+        const auto c = unit_c(1LL);
+        auto stream_to_string = [](auto x) {
+            std::ostringstream oss;
+            oss << x;
+            return oss.str();
+        };
+        std::vector<std::string> results = {
+            stream_to_string(a + b + c),
+            stream_to_string(a + c + b),
+            stream_to_string(b + a + c),
+            stream_to_string(b + c + a),
+            stream_to_string(c + a + b),
+            stream_to_string(c + b + a),
+        };
+        EXPECT_THAT(results, Each(Eq(results[0])))
+            << "Inconsistency found for (" << a << ", " << b << ", " << c << ")";
+    };
+
+    check_units(centi(meters), miles, meters);
+    check_units(kilo(meters), miles, milli(meters));
+}


Note to reviewer: if we added only this test case on main, without the fixes, here is what the failure would look like:

[ RUN ] Quantity.CommonUnitAlwaysCompletelyIndependentOfOrder au/code/au/quantity_test.cc:743: Failure Value of: results Expected: only contains elements that is equal to "805177 COM[cm, m, mi]" Actual: { "805177 COM[cm, m, mi]", "805177 COM[cm, mi]", "805177 COM[cm, m, mi]", "805177 COM[cm, m, mi]", "805177 COM[cm, mi]", "805177 COM[cm, m, mi]" }, whose element #1 doesn't match Inconsistency found for (1 cm, 1 mi, 1 m)

Instead of checking that they are all consistent with the first element, can we check that they are all equal to a specific unit? That might help test readibility.

I see the appeal of this suggestion. I struggled with this for a little bit, but I couldn't see my way to an implementation that did this while keeping the test implementation clean.

One way would be to compare to a desired unit label. But the unit label for common units will change dramatically after 0.4.0 (#105), and I'd hate to have to update this test for that change.

We could also do it by providing the expected type of the common unit. But CommonUnit<...> isn't supposed to be named directly by end users: we're supposed to use CommonUnitT<...>, which performs the de-duping and simplification that is the very thing under test!

Ultimately, I think the current approach is the best option available. Phrasing it in terms of order independence, as mentioned in #295 (comment), is the most elegant way I know to express what we're really after here. The downside, as you point out, is that a test failure can't tell you which one is correct; all it can do is point out that they should all be the same, and they're not. But I think in practice this will be good enough to point maintainers in the right direction.

chiphogg · 2024-10-26T19:19:36Z

Note: compile time impact tests came back squeaky clean. Aurora-internal AV repo tests are still running but look very promising so far and I expect them to pass as well.

geoffviola

Great change! This should make common units more understandable.

geoffviola · 2024-10-28T14:40:13Z

au/code/au/utility/type_traits.hh

+////////////////////////////////////////////////////////////////////////////////////////////////////
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+// `Prepend` implementation.


What is the point of this comment? Do we need a comment for all inline implementations?

Often, the implementation for "public"-ish interface declared up top in a file will depend on new helpers, which I will declare-and-define right next to the "core" implementation. This can get a little messy and confusing, so I use these "section comments" to group things and clarify the purpose. That said, I'm certainly rather inconsistent about using them in practice.

geoffviola · 2024-10-28T14:50:43Z

au/code/au/quantity_test.cc

+TEST(Quantity, CommonUnitAlwaysCompletelyIndependentOfOrder) {
+    auto check_units = [](auto unit_a, auto unit_b, auto unit_c) {
+        const auto a = unit_a(1LL);
+        const auto b = unit_b(1LL);
+        const auto c = unit_c(1LL);
+        auto stream_to_string = [](auto x) {
+            std::ostringstream oss;
+            oss << x;
+            return oss.str();
+        };
+        std::vector<std::string> results = {
+            stream_to_string(a + b + c),
+            stream_to_string(a + c + b),
+            stream_to_string(b + a + c),
+            stream_to_string(b + c + a),
+            stream_to_string(c + a + b),
+            stream_to_string(c + b + a),
+        };
+        EXPECT_THAT(results, Each(Eq(results[0])))
+            << "Inconsistency found for (" << a << ", " << b << ", " << c << ")";
+    };
+
+    check_units(centi(meters), miles, meters);
+    check_units(kilo(meters), miles, milli(meters));
+}


Instead of checking that they are all consistent with the first element, can we check that they are all equal to a specific unit? That might help test readibility.

chiphogg added the release notes: 🐛 lib (bugfix) PR fixing a defect in the library code label Oct 26, 2024

chiphogg requested a review from geoffviola October 26, 2024 18:52

chiphogg commented Oct 26, 2024

View reviewed changes

geoffviola approved these changes Oct 28, 2024

View reviewed changes

chiphogg merged commit 5f79db3 into main Oct 28, 2024
11 checks passed

chiphogg deleted the chiphogg/reduce-common#295 branch October 28, 2024 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eliminate redundant units from `CommonUnit<...>` #310

Eliminate redundant units from `CommonUnit<...>` #310

chiphogg commented Oct 26, 2024

chiphogg Oct 26, 2024

geoffviola Oct 28, 2024

chiphogg Oct 28, 2024

chiphogg commented Oct 26, 2024

geoffviola left a comment

geoffviola Oct 28, 2024

chiphogg Oct 28, 2024

geoffviola Oct 28, 2024

Eliminate redundant units from CommonUnit<...> #310

Eliminate redundant units from CommonUnit<...> #310

Conversation

chiphogg commented Oct 26, 2024

chiphogg Oct 26, 2024

Choose a reason for hiding this comment

geoffviola Oct 28, 2024

Choose a reason for hiding this comment

chiphogg Oct 28, 2024

Choose a reason for hiding this comment

chiphogg commented Oct 26, 2024

geoffviola left a comment

Choose a reason for hiding this comment

geoffviola Oct 28, 2024

Choose a reason for hiding this comment

chiphogg Oct 28, 2024

Choose a reason for hiding this comment

geoffviola Oct 28, 2024

Choose a reason for hiding this comment

Eliminate redundant units from `CommonUnit<...>` #310

Eliminate redundant units from `CommonUnit<...>` #310