-
Notifications
You must be signed in to change notification settings - Fork 95
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
fix: add quantity_point::origin, like std::chrono::time_point::clock #288
Conversation
template<typename C, typename Rep, typename Period> | ||
inline constexpr bool is_quantity_point_like<std::chrono::time_point<C, std::chrono::duration<Rep, Period>>> = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why this was missing, as is is_quantity_like<duration>
, or why it worked before. But now this one didn't, so I added this.
template<Dimension D2> | ||
using rebind = dynamic_origin<D2>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything "rebind" is to support equivalent dimensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thanks!!!
BTW, it would be great to apply this to temperature conversion...
I'll leave that to #232. Hopefully it's easier to review now that this is in. |
My understanding is that this feature is still WIP and should not be merged in the current state. The more time we wait the harder will be to merge it as the master branch changed a lot since then... 😢 |
It seems we have some compilation issues on MSVC... |
You're right. Since other compiler/library combinations were failing due to an issue unrelated to this PR, I skipped checking MSVC. Shall I factor out these _requires-expression_s as concepts to please MSVC and submit a PR? Hopefully that's all that's preventing it from passing. |
Yes, please. BTW, sorry that the CI did not work for such a long time but I was pushing LA guys several times and only today it got fixed. |
Hey @JohelEGP, thanks for pushing this forward! Specifically, to my understanding, a point origin is a fundamental abstract object. It encodes the notion of "which (abstract) point" on the underlying dimension/scale the origin refers to. As in e.g. the abstract concept of "the temperature where water freezes at standard conditions" (0 °C, the temperature origin point relative to which temperatures expressed in degree Celsius are customarily referring to). In order to distinguish different such abstract origins, we use a type: We associate one specific abstract origin with each PointOrigin type, similar to std::chrono, which associates a distinct specific origin with each clock. Contrary to std::chrono however, you implemented a notion of "equivalent" PointOrigin types (different types referring to the same abstract origin), and in #232, where we start to implement conversion between points expressed with respect to different origins (i.e. temperature conversions), we also need a notion of "convertible" origins (origins expressed on equivalent dimensions, with the associated abstract origins being separated by an offset known to the type system). By definition "equivalent" types are automatically "convertible" (zero offset), but the converse is not true. In order to implement those two notions, we need have an understanding which abstract origin each specific PointOrigin type refers to. I therefore propose the following:
We probably want at least one such library-provided PointOrigin template so we can implement a CTAD-derived
Personally, I believe option 2. is the safest, but I believe option 3 might be desired too - then we should make a big disclaimer in the documentation that, all of a sudden, the units of the argument start to carry a physical meaning - contrary to the rest of the library. |
The complexity of everything "equivalent" related, including |
Before diving into temperatures, consider first implementing origins that are linearly distanced. The glider example, for example, has a notion of sea level and some other linearly distanced level above the sea level. |
Temperatures are linearly distanced... |
Sorry, I meant that the distance between the origins is a constant, such that the number isn't scaled. Changes between temperature origins don't have this property, which I think can complicate things. So it'd be better to start simple. |
I agree with your proposal, minor some bikeshedding below. I haven't looked into it nor thought too hard, but implementing As for |
I don't agree. The difference between mean sea level and the runway threshold of ZRH airport is a constant - approximately 400 meters or 1300 feet. The difference between the fahrenheit origin and the celsius origin is also ca constant: approximately 18 °C or exactly 32 °F (temperature distances, not points!). What makes temperatures more complicated is that we implicitly associate a temperature origin - which depends on the temperature unit.
Indeed, I just realised that contrary to my implementation in #232, there is no separate dimension argument to the
In my proposal |
I was under the impression that temperature conversions had some overlap with #35. |
It's not, at least not for the typical Kelvin, Fahrenheit and Celsius temperature. I believe that is a somewhat orthogonal question: log-scale units like dbm (usually short for db mW) only apply to relative quantities (that is |
But back to temperatures and "default" or "dynamic" origin points, it all boils down to the fundamental question which abstract origin each type shall denote. And these "equivalent" dimensions will cause trouble no matter what, whenever we do include them at all in a template parameter of a PointOrigin type: All over the That said, I believe it can be fixed. You currently rebind the |
Yes.
Ideally. Look at all the open issues on equivalent dimensions. Maybe you don't have to worry about equivalent dimensions. Temperatures only exist in the main SI namespace, so there are no equivalent to worry about for now.
Of course, a general solution would have to worry. But I really don't want to indulge on something seemingly so broken. Ask @mpusz if they'd be OK with ignoring the issue of equivalent dimensions if it'd be too much of a hurdle on you. |
Ah, I haven't fully followed the recent developments around equivalent dimensions. In particular, I didn't realise yet that now, imperial units and metric units share the same (not just equivalent) dimensions. |
Resolves #240.
Resolves #287.
Doesn't support origins that are offset from another origin.
Doesn't support temperatures.