Simplify common point unit implementation #421
Conversation
Now, the common point unit is just the _common unit_, except that instead of just looking at all of the input units, we also consider their mutual origin displacements (treated as new ad hoc units). And, we set an origin that is equal to the lowest origin among those inputs. That's it! No more ugly hacks... goodbye `TypeHoldingCommonOrigin`! This simplifies our logic for conversion checking in `QuantityPoint`, and maximally expands the set of allowed operations. This is made possible because the origin displacement is now always a shapeshifter type, which has a perfect conversion policy, known at compile time, to every destination type. I believe this is a first among units libraries. Some people might have been using the `OriginDisplacement` trait, so we retain it, but it has to move from `unit_of_measure.hh` to `quantity_point.hh`, because now it depends on `:constant`. We document this. We did need to refine the ordering by adding another tiebreaker based on the origin displacement unit, too. Fixes #369.
| template <typename U, typename R, typename ConstUnit> | ||
| constexpr Quantity<U, R> coerce_as_quantity(Constant<ConstUnit> c) { | ||
| return c.template coerce_as<R>(U{}); | ||
| } | ||
| template <typename U, typename R> | ||
| constexpr Quantity<U, R> coerce_as_quantity(Zero z) { | ||
| return z; | ||
| } |
There was a problem hiding this comment.
I'd like to note that I could see wanting this as part of our API, but I wouldn't want it as part of our API.
Specifically, when using a Constant, I'd love to be able to just use .as() more often, or even as<Rep>(), but I find myself having to do similar to as<Rep>(c::unit) in most contexts because things can't be inferred. Basically, it'd be nice to get a Constant in a Rep in whatever Unit the constant is in naturally.
There was a problem hiding this comment.
I certainly wouldn't have written something as clunky as these functions unless I felt forced to do so. 🙂
In this case, the driving factor is that we need to treat Zero the same way as Constant<U>. For the "forcing" conversions, we need to overrule the perfect conversion policy of Constant<U>. detail::coerce_as_quantity is an OK way to spell that. This just means we need to provide the same function for Zero.
Note, too, that the fact that it's in the detail namespace (soon to be auimpl) means that it's not part of our API! I agree with you that we wouldn't want to promote this interface broadly. This is just a helper for implementing QuantityPoint.
Also:
Basically, it'd be nice to get a
Constantin aRepin whateverUnitthe constant is in naturally.
Just for clarity, this is what c.as<Rep>() does today; it just so happens that it always stores Rep{1} under the hood.
Also also: I hope once we have #387, it could unlock more fluent interfaces for Constant<U>. Maybe we could have a second template parameter that governs the strictness of the conversion policy, and we can provide helper member functions that convert the constant into one with a different policy strictness!
There was a problem hiding this comment.
Just for clarity, this is what c.as() does today; it just so happens that it always stores Rep{1} under the hood.
I see now that it works. Maybe it's the other way I'm thinking about. c.as(u). In those cases, it's almost never able to infer the Rep so I end up having to do c.as<Rep>(u). Which currently disables our checks, which #387 should help us with. So that was a roundabout way to get to the same place. 😁
| The reason we moved it was because `"au/constant.hh"` (which defines `Constant`) depends on | ||
| `"au/quantity.hh"`, which in turn depends on `"au/unit_of_measure.hh"`. Therefore, we could | ||
| never have used `Constant` inside of `"au/unit_of_measure.hh"`. However, | ||
| `"au/quantity_point.hh"` _could_ depend on `"au/constant.hh"`. Origin displacements aren't very | ||
| useful without `QuantityPoint` anyway, so this new home is acceptable. |
There was a problem hiding this comment.
Almost seems more than acceptable? Seems like an inherent trait of a QuantityPoint, so maybe makes more sense there? Should we move this documentation to quantity_point.md?
There was a problem hiding this comment.
Good question! On reflection, I think the documentation should stay here, because OriginDisplacement is a trait on units. It's true that most of the use cases for this happen in a QuantityPoint context, but the trait itself still operates on units, so I think we'd probably keep it in unit_of_measure.hh if we could.
There was a problem hiding this comment.
To clarify, I'm fine where it is.
Oh, I see. I think. Would it be fair to say that Origin is a trait for a QuantityPoint and OriginDisplacement is a trait on the Unit?
There was a problem hiding this comment.
I would say that Origin is a trait for a unit, and OriginDisplacement is a trait on a pair of units.
That's in the current setup of the library. I definitely don't think it's optimal. It would be good to enable custom origins that could be associated with the QuantityPoint. Someday I hope we can find the right balance between supporting that kind of use case, and also supporting units with very clear intrinsic non-"zero" origins such as Celsius and Fahrenheit.
Co-authored-by: Michael Hordijk <hordijk@aurora.tech>
This code was written in December or January, and I forgot to check for this when I put it out for review just now in May! Co-authored-by: Michael Hordijk <hordijk@aurora.tech>
| template <typename U, typename R, typename ConstUnit> | ||
| constexpr Quantity<U, R> coerce_as_quantity(Constant<ConstUnit> c) { | ||
| return c.template coerce_as<R>(U{}); | ||
| } | ||
| template <typename U, typename R> | ||
| constexpr Quantity<U, R> coerce_as_quantity(Zero z) { | ||
| return z; | ||
| } |
There was a problem hiding this comment.
Just for clarity, this is what c.as() does today; it just so happens that it always stores Rep{1} under the hood.
I see now that it works. Maybe it's the other way I'm thinking about. c.as(u). In those cases, it's almost never able to infer the Rep so I end up having to do c.as<Rep>(u). Which currently disables our checks, which #387 should help us with. So that was a roundabout way to get to the same place. 😁
| The reason we moved it was because `"au/constant.hh"` (which defines `Constant`) depends on | ||
| `"au/quantity.hh"`, which in turn depends on `"au/unit_of_measure.hh"`. Therefore, we could | ||
| never have used `Constant` inside of `"au/unit_of_measure.hh"`. However, | ||
| `"au/quantity_point.hh"` _could_ depend on `"au/constant.hh"`. Origin displacements aren't very | ||
| useful without `QuantityPoint` anyway, so this new home is acceptable. |
There was a problem hiding this comment.
To clarify, I'm fine where it is.
Oh, I see. I think. Would it be fair to say that Origin is a trait for a QuantityPoint and OriginDisplacement is a trait on the Unit?
Now, the common point unit is just the common unit, except that
instead of just looking at all of the input units, we also consider
their mutual origin displacements (treated as new ad hoc units). And,
we set an origin that is equal to the lowest origin among those inputs.
That's it! No more ugly hacks... goodbye
TypeHoldingCommonOrigin!This simplifies our logic for conversion checking in
QuantityPoint,and maximally expands the set of allowed operations. This is made
possible because the origin displacement is now always a shapeshifter
type, which has a perfect conversion policy, known at compile time, to
every destination type. I believe this is a first among units
libraries.
Some people might have been using the
OriginDisplacementtrait, so weretain it, but it has to move from
unit_of_measure.hhtoquantity_point.hh, because now it depends on:constant. We documentthis.
We did need to refine the ordering by adding another tiebreaker based on
the origin displacement unit, too.
Fixes #369.