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

[Question] Why was .as<Unit, Rep>() deprecated? #121

Closed
avrahamshukron opened this issue Mar 14, 2023 · 3 comments
Closed

[Question] Why was .as<Unit, Rep>() deprecated? #121

avrahamshukron opened this issue Mar 14, 2023 · 3 comments

Comments

@avrahamshukron
Copy link

In our code base we pass around temperatures in uint32_t deci-kelvins since it gives us the right amount of precision we want without requiring floating point type, and the relevant range is contained within uint32_t, so we defined:

using DeciKelvins = au::QuantityPointU32<au::Deci<au::Kelvins>>;
constexpr auto decikelvins = au::deci(au::kelvins_pt);

But most of the temperature constants are expressed in degrees-Celsius since that is what we usually receive from EE teams and in general more human friendly.
This creates a bit of annoyance when wanting to convert these Celsius constant to DeciKelvins, since a lot of data needs to be repeated:

constexpr DeciKelvins MAX_CHARGING_THRESHOLD = celsius(43).as<DeciKelvins::Rep>(decikelvins);

Ideally I wish I could just write:

constexpr auto MAX_CHARGING_THRESHOLD = celsius(43).as<DeciKelvins>();

Since DeciKelvins already contains both the desired representation and the unit, everything is explicit and to point.
Also the use of actual type feels more natural with as and in as they are both act as some kind of casting mechanism.

Was there a specific reason to deprecate as<Unit, Rep>?
Would it be possible to bring it back, potentially with as<Quantity<Unit,Rep>>?
Or maybe I'm looking at this all wrong and the idiomatic usage of this library avoids this problem altogether?

@chiphogg
Copy link
Contributor

Thanks for asking!

First, I wanted to check something. Converting integral celsius temperatures to integral decikelvins is lossy, right? Since the additive constant is 273.15 K, the largest unit that could work would be [1/20] K. (I think we actually use centi(kelvins) currently, although we might change that to kelvins / mag<20>() later if we do.)

So, were you OK with this lossy conversion? And if not, can you simply switch to centi(kelvins)? Then you would be able to write:

constexpr CentiKelvins MAX_CHARGING_THRESHOLD = celsius(43).as(centikelvins);

// Or, more simply:
constexpr CentiKelvins MAX_CHARGING_THRESHOLD = celsius(43);

(I'm assuming that your celsius is the same as au::celsius_pt.)


By the way --- if this was a lossy conversion, why did Au allow it? Answer: because the "explicit-Rep" form is morally equivalent to static_cast, and has "forcing" semantics. Here's the relevant section of the conversion tutorial. As a matter of habit, you're better off writing, celsius(43).as(decikelvins) (or centikelvins), and letting the compiler tell you which ones are dangerous.

I've only recently begun to ponder whether we should someday separate the notion of "forcing" from the ability to specify a given Rep. I have always conflated these ideas so far, probably because static_cast does both, but really they're separate concepts. It could be useful to be able to say "give me the answer in this Rep, but only if it's safe". And it could also be useful to be able to write inches(50).force_as(feet). I filed #122 to track this.


Now for the stated question. 🙂 We deprecated the "CppCon style" interfaces, such as .as<Unit, Rep>(), because they have exact equivalents in the "Au style" interface, and we didn't want to have two ways to spell the same thing. There was no loss in functionality, just a change in spelling.

Let's say that unit is some instance that represents Unit (i.e., either QuantityMaker<Unit>{} or Unit{}). Then these are the replacements:

  • .as<Unit, Rep>() -> .as<Rep>(unit)
  • .as<Unit>() -> .as(unit)

It's the same for replacing as with in, and also the same for QuantityPoint instead of Quantity (although then unit would be either QuantityPointMaker<Unit>{} or Unit{}).

@avrahamshukron
Copy link
Author

Thank you @chiphogg that makes a lot of sense.
To clarify: Yes I was OK with the precision loss from C to dK, and yes celsius was supposed to be celsius_pt.
I'm intrigued now to go over the entire codebase and change every DeciKelvin to CentiKelvin because it will cover the relevant range even in uint16_t (which some of our legacy code requires for some reason).

Personally I liked the CppCon style better because of my example above but it comes with a tradeoff as always.

@chiphogg
Copy link
Contributor

I wanted to say a little more for posterity about why we switched (especially for folks in the future who find this issue via search). "We didn't want to have two ways to spell the same thing" --- that's easy to see, but why pick one vs. the other? It turns out the new syntax actually has some concrete advantages.

Better composability

With the old syntax, you need to have a type handy. Composing types is just generally really awkward (unfortunately, this is still true today). However, values --- whether QuantityMaker or unit --- compose really nicely!

// Old way: need to create some ad hoc types to represent unusual units.
{
    using MetersPerSecondSquared = UnitPowerT<UnitQuotientT<Meters, Seconds>, 2>;
    const auto vel_sq_m2 = vel_sq.in<MetersPerSecondSquared>();
}

// New way: compose the units on the fly!
{
    const auto vel_sq_m2 = vel_sq.in(squared(meters / second));
}

Note that this approach is also clearer: the name MetersPerSecondSquared is a little ambiguous between $\text{m} \cdot \text{s}^{-2}$ and $(\text{m} / \text{s})^2$.

Less need for .template keyword

When the Quantity itself is a templated type, we need to replace calls to .in<...>() with .template in<...>(). (This tends to happen in generic code.) The error message is hard to interpret unless you've seen it many times before. But many of these examples go away when we turn the template parameter into a "regular" parameter!

// Old syntax:
template <typename... Ts>
constexpr std::array<double, sizeof...(Ts)> values_in_radians(Ts... ts) {
    return {rep_cast<double>(ts).template in<Radians>()...};
}   //                           ^^^^^^^^^ Ugly, distracting keyword

// New syntax:
template <typename... Ts>
constexpr std::array<double, sizeof...(Ts)> values_in_radians(Ts... ts) {
    return {rep_cast<double>(ts).in(radians)...};
}

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