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

Collect all scale factors for a unit #382

Open
chiphogg opened this issue Jan 21, 2025 · 1 comment
Open

Collect all scale factors for a unit #382

chiphogg opened this issue Jan 21, 2025 · 1 comment
Labels
⬇️ affects: code (implementation) Affects implementation details of the code 📁 kind: cleanup Making the library nicer in some way, without affecting functionality much or at all 💪 effort: small
Milestone

Comments

@chiphogg
Copy link
Contributor

We have anonymous scaled units, which don't get a special name, but do get an automatic label which is just that scale factor plus the unscaled unit's symbol. In these instances, we shouldn't be treating the number as part of the "name" of the unit. Instead, we should gather all numbers outside and simplify, and perform products and powers on the unscaled versions of the units.

We already do this to some extent with the new common unit formulation. For example, the common unit of feet * mag<10>() and feet * mag<6>() is simply feet * mag<2>(). Bringing this same approach to products and powers will just make us more consistent.

For example, consider this code, where we first define a few custom symbols, and then execute some unit expressions:

using symbols::ft;
using symbols::L;
using symbols::s;
constexpr auto km = kilo(symbols::m);
constexpr auto L_per_100_km = L / (km * mag<100>());

std::cout << "1) " << (10 * L_per_100_km) << "\n";
std::cout << "2) " << (10 * L_per_100_km * (20 * km)) << "\n";
std::cout << "3) " << (55 * cubed(ft * mag<10>())) << "\n";
std::cout << "4) " << (60 * (ft * mag<5'280>()) / (s * mag<3'600>())) << "\n";

This currently prints:

1) 10 L / [100 km]
2) 200 (km * L) / [100 km]
3) 55 [10 ft]^3
4) 60 [5280 ft] / [3600 s]

I think it should print:

1) 10 [(1 / 100) L / km]
2) 200 [(1 / 100) L]
3) 55 [1000 ft^3]
4) 60 [(22 / 15) ft / s]

The first case, I actually like a little less:

OLD 1) 10 L / [100 km]
NEW 1) 10 [(1 / 100) L / km]

"Liters per hundred km" is a good and understandable unit of measure, where as "one hundredth liter per kilometer" is not. However, if we stick with this, then we get the second case:

OLD 2) 200 (km * L) / [100 km]
NEW 2) 200 [(1 / 100) L]

This feels pretty bad. "km liters per hundred km" should just be "one hundredth liter" --- we clearly do want to cancel here. (This is distinct from something like (cm / m), where applying a prefix changes the name of the unit, and we don't want to try to get clever.)

If we did this change, and gave up getting the "nice" behavior of the first one by default, we could still recover it by making a new strong-typed unit for the "100 km" part, whose label is [100 km]. This would have the same pitfall of not cancelling when we multiply by km, but I think that's fine... especially because these are all just anonymous intermediate results anyway, and users who cared about the units would assign to a quantity of Liters or something.

The third case shows how it works in the case of powers:

OLD 3) 55 [10 ft]^3
NEW 3) 55 [1000 ft^3]

The old one might be slightly nicer here, in that it expresses the side length of the cube with this volume unit, which makes it easier to picture. But I think that (a) the sort of "ideological consistency" that numbers are not part of names is worth giving this up to get; (b) the "new" way is likely to perform better when we combine it with other units; and, (c) there is still the same workaround of making a strong-typed unit [10 ft] for users who really want to preserve this automatic behavior.

Finally, the fourth case is a fanciful construction of MPH in terms of ft/sec:

OLD 4) 60 [5280 ft] / [3600 s]
NEW 4) 60 [(22 / 15) ft / s]

Here, too, there's no "slam dunk" best. The old way makes it easy to guess the source units, if someone is really familiar with units. But the new way is surely more concise and illuminating.


Overall, I'm going to get the implementation landable (I did a proof of concept late last year), double check the compile time impact, and plan to land it unless I find a compelling reason not to.

@chiphogg chiphogg added this to the 0.4.2 milestone Jan 21, 2025
@chiphogg chiphogg added 📁 kind: cleanup Making the library nicer in some way, without affecting functionality much or at all 💪 effort: small ⬇️ affects: code (implementation) Affects implementation details of the code labels Jan 21, 2025
chiphogg added a commit to chiphogg/au that referenced this issue Jan 22, 2025
Scale factors are not part of the "name" of a unit.  Therefore, we
should gather and combine them every time we form products and powers of
units.  This will also make us consistent with the way we handle
anonymous scaled units in the _common unit_ use case.

Fixes aurora-opensource#382.
@chiphogg
Copy link
Contributor Author

Overnight measurements were not encouraging. It looks like we'd be incurring a roughly constant 25 ms penalty for adding this feature. I don't think the cost/benefit adds up here.

It might still be worth adding a gather_scale_factors(...) utility, where users can request this operation. I'll think about that some more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⬇️ affects: code (implementation) Affects implementation details of the code 📁 kind: cleanup Making the library nicer in some way, without affecting functionality much or at all 💪 effort: small
Projects
None yet
Development

No branches or pull requests

1 participant