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

fix: add quantity_point::origin, like std::chrono::time_point::clock #288

Merged
merged 1 commit into from
Jun 29, 2021

Conversation

JohelEGP
Copy link
Collaborator

Resolves #240.
Resolves #287.
Doesn't support origins that are offset from another origin.
Doesn't support temperatures.

Comment on lines +58 to +59
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;
Copy link
Collaborator Author

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.

Comment on lines +46 to +47
template<Dimension D2>
using rebind = dynamic_origin<D2>;
Copy link
Collaborator Author

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.

Copy link
Owner

@mpusz mpusz left a 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...

@mpusz mpusz merged commit 80eefec into mpusz:master Jun 29, 2021
@JohelEGP JohelEGP deleted the point_origin branch June 29, 2021 18:44
@mpusz mpusz added the enhancement New feature or request label Jun 29, 2021
@mpusz mpusz added this to the v0.8.0 milestone Jun 29, 2021
@JohelEGP
Copy link
Collaborator Author

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.

@mpusz
Copy link
Owner

mpusz commented Jun 29, 2021

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... 😢

@mpusz
Copy link
Owner

mpusz commented Jun 29, 2021

It seems we have some compilation issues on MSVC...

@JohelEGP
Copy link
Collaborator Author

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.

@mpusz
Copy link
Owner

mpusz commented Jun 29, 2021

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.

JohelEGP added a commit to JohelEGP/units-1 that referenced this pull request Jun 29, 2021
JohelEGP added a commit to JohelEGP/units-1 that referenced this pull request Jun 29, 2021
mpusz pushed a commit that referenced this pull request Jun 30, 2021
@burnpanck
Copy link
Contributor

Hey @JohelEGP, thanks for pushing this forward!
I'm now trying to merge in the latest changes into #232. I intend to update the design to your PointOrigin concept.
I've noticed your rebind machinery, and I need some clarification regarding your PointOrigin concept and the associated equivalency.

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:

  • In general, we assume distinct types refer to unrelated abstract origins - no equivalency or convertibility is implied.
  • User-defined custom PointOrigin types are not considered equivalent or convertible (as above). We may specify a customisation mechanism that lets the user declare convertibility or equivalency to other PointOrigin types.
  • We provide one or more of the following template PointOrigin implementations with type-system provided support for "equivalence" and "convertibility":
    1. Your types dynamic_origin<D> denote an unspecified abstract origin that is equivalent only to other dynamic_origin<D2> exactly iff D is equivalent to D2.
    2. Each of my previous types default_point_origin<U> denotes an unspecified abstract origin that is equivalent or convertible only to itself.
    3. We define a new customary_point_origin<U> (name up for bike-shedding) template type, which refers to that customary origin we associate with units such as Kelvin / Fahrenheit / Celsius. Our implementation shall ensure that two customary_point_origin<U> and customary_point_origin<U2> shall be equivalent if U and U2 denote differently prefixed (or un-prefixed) versions of the same unit. A yet unspecified internal machinery makes further points "convertible" to each other when there is a clear customary notion of that reference point (i.e. only for temperatures as far as I know). Any other type is neither equivalent nor convertible (in particular, neither the corresponding dynamic_origin<D> - transitivity of the equivalence relation would make Kelvin and Celsius origins equivalent).

We probably want at least one such library-provided PointOrigin template so we can implement a CTAD-derived QuantityPoint(q). Depending on which PointOrigin type we select, we'd arrive at the following behaviour:

  1. dynamic_origin<decltype(q)::dim>: Has the effect that the origins of QuantityPoint(1_q_m) and QuantityPoint(1_q_in) are equivalent, but so are those of QuantityPoint(1_q_degC) and QuantityPoint(1_q_degF).
  2. default_point_origin<decltype(q)::dim>: Neither the origins of QuantityPoint(1_q_m) and QuantityPoint(1_q_in) are equivalent, nor those of QuantityPoint(1_q_degC) and QuantityPoint(1_q_degF).
  3. or customary_point_origin<decltype(q)::unit>. Has the effect that the origins of QuantityPoint(1_q_degC) and QuantityPoint(1_q_degF) are convertible but not equivalent, but those of QuantityPoint(1_q_m) and QuantityPoint(1_q_in) are not even convertible.

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.

@JohelEGP
Copy link
Collaborator Author

The complexity of everything "equivalent" related, including RebindablePointOriginFor, exist for the sole purpose of allowing things not to break when using equivalent dimensions, such as isq::si::dim_length and isq::si::cgs::dim_length.

@JohelEGP
Copy link
Collaborator Author

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.

@burnpanck
Copy link
Contributor

Temperatures are linearly distanced...

@JohelEGP
Copy link
Collaborator Author

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.

@JohelEGP
Copy link
Collaborator Author

I agree with your proposal, minor some bikeshedding below. I haven't looked into it nor thought too hard, but implementing default_point_origin might turn out to be complicated due to how the library has the dependency between a dimension and its units seemingly backwards.

As for default_point_origin, you first correctly give it a template argument of a unit, but then a dimension. For now, I believe its name should reflect the special case of temperatures. Maybe name it unit_point_origin.

@burnpanck
Copy link
Contributor

burnpanck commented Sep 17, 2021

t 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 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.

I haven't looked into it nor thought too hard, but implementing default_point_origin might turn out to be complicated due to how the library has the dependency between a dimension and its units seemingly backwards.

Indeed, I just realised that contrary to my implementation in #232, there is no separate dimension argument to the quantity_point template anymore. I'll have to think more about this.

As for default_point_origin, you first correctly give it a template argument of a unit, but then a dimension. For now, I believe its name should reflect the special case of temperatures. Maybe name it unit_point_origin.

In my proposal default_point_origin exactly didn't special-case for temperatures. It is simply an abstract origin that didn't relate to any other, temperature or not.

@JohelEGP
Copy link
Collaborator Author

I was under the impression that temperature conversions had some overlap with #35.

@burnpanck
Copy link
Contributor

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 quantity, not quantity_point), because they effectively describe a multiplicative factor times some reference quantity - something which has no meaning for quantity points.

@burnpanck
Copy link
Contributor

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 units library, equivalent dimensions are truly exchangeable - they carry no physical meaning. But the implicitly referred origin point on temperature scales reflects itself on this virtual dimension our units library associates with each unit. Now we have a problem: Making the PointOrigin equivalent for equivalent dimensions, we risk unexpected (wrong) conversions in the temperature case. So the safe thing is to not consider them equivalent. But since we now tie the dimension of the quantity_point to the PointOrigin, we loose the freedom of the equivalency among dimensions for the relative distance to the origin. I believe it was because of that I originally kept the PointOrigin and the Dimension argument to the quantity_point template separate.

That said, I believe it can be fixed. You currently rebind the dynamic_origin to the result dimension of an operation such as qp + q. For an implied equivalent dynamic_origin that is fine - for the safer default_point_origin or the magic customary_point_origin version where no implicit equivalency is give, that doesn't work. We would have to add a second dimension template argument, which has no impact on the equivalency relation. The fix would be the following: Don't let the qp.relative() + q operation determine the output dimension - instead, convert to the dimension implied by the original PointOrigin. Since there is always just one quantity_point in such an operation, there is no ambiguity - and the result dimension must be equivalent, otherwise the operation would be rejected even for ther .relative part.

@JohelEGP
Copy link
Collaborator Author

And these "equivalent" dimensions will cause trouble no matter what

Yes.

All over the units library, equivalent dimensions are truly exchangeable - they carry no physical meaning.

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.

[johel@sundown systems]$ find . | grep temp
./si/include/units/isq/si/thermodynamic_temperature.h
./isq/include/units/isq/dimensions/thermodynamic_temperature.h

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.

@burnpanck
Copy link
Contributor

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.
I will therefore consider just static asserting when equivalent but not same dimensions are involved if the workaround turns out to be too involved - though I believe it can be solved alright.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: quantity_point_cast constraint fix: quantity_point doesn't account for differences in origins
3 participants