mp-units icon indicating copy to clipboard operation
mp-units copied to clipboard

[WIP] Identify reference/origin point in quantity_point -> enable temperature conversions

Open burnpanck opened this issue 3 years ago • 16 comments

Overview

This is a design study to represent the origin/reference point with respect to which quantity_points measure their value. If there is a well-defined relation between origins of scales of compatible dimensions, the library can be enabled to convert between such quantity points that are specified with respect to different origins. The most common example are the three temperature scales Kelvin, degree Celsius and degree Fahrenheit. (Personally, my favourite use is to represent sensor readings from electronic sensors in embedded systems as physical quantities without any runtime conversions; such sensors may represent results with respect to some arbitrary, but fixed origin).

Changes to quantity_point

This PR implements a concept PointOrigin, and extends the template arguments of quantity_point to template <Dimension D, UnitOf<D> U, QuantityValue Rep = double, PointOrigin Org = default_point_origin<typename dimension_unit<D>::type>> (and similarly for quantity_point_kind) . In particular, a default_point_origin is defined to allow existing code to continue to work.

PointOrigin concept

Currently, the PointOrigin concept does not relate to a specific dimension, it is simply a tag type that references an abstract origin. The underlying notion however is usually tied tied to a dimension (or even a kind of quantity - I'm not that experienced with that notion). I had initially designed the concept as PointOrigin<D>, but that caused trouble with the tests where quantity points are liberally converted between kinds with differing dimensions but compatible units (e.g. cgd::dim_length vs. si::dim_length). Further exploration might be needed here.

Also, in this PR, PointOrigin are compares through type identity (std::is_same_v) though there might be a need for something slightly more lenient.

std::chrono::time_point

std::chrono::time_point has an implicit notion of the origin / reference too, through the time_point::clock member type: "Clock, the clock on which this time point is measured". Therefore, this PR handles std::chrono interoperability through the introduction of a chrono_clock_point_origin<Clock>.

Compatibility between quantity points measured against different references

In this PR, quantity_points with differing origins are incompatible and cannot be assigned / constructed from each other, not even explicitly. Imho, that is the sensible thing to do, as in most cases that would result in a change of the value. If one really needs to do that, one can do so explicitly using quantity_point_2(qp1.relative()).

Casting / converting between quantity points measured against different references (temperatures!)

This PR does not yet implement any explicit casts or conversion routines. The example temperature.cpp does highlight the three temperature scales, and implements a simple quantity_point_offset_cast, correctly converting between those temperatures. This could be implemented into quantity_cast and it's relatives, though there is ambiguity if that should cast by just copying the numeric value, converting the relative part properly, or fully convert the scale.

Side remark (rant?) on quantity_cast

In general, I think the quantity_cast and friends are already somewhat mis-designed, in that their names say to what they apply rather than what they do: They convert representations imprecisely, but also replace quantity kinds with unrelated ones, and potentially more. This is somewhat akin to a C-style cast, which "magically" selects what to do, rather than the C++ casts which all just do one thing which is clearly and explicitly stated by the name. It's unfortunate that there is already a bit of precedent in the standard library with std::chrono::duration_cast. Maybe I should open a separate issue (see also a previous comment to another issue).

Tests

The existing tests have been adapted and pass on my machine (somehow they fail in CI in Debug mode - unable to reproduce yet). No explicit tests on compatibility / incompatibility between unrelated origins yet.

Documentation

Not in the PR yet - sorry.

Bike-shedding

Feel free.

burnpanck avatar Feb 21 '21 17:02 burnpanck

Compatibility between quantity points measured against different references

In this PR, quantity_points with differing origins are incompatible and cannot be assigned / constructed from each other, not even explicitly. Imho, that is the sensible thing to do, as in most cases that would result in a change of the value. If one really needs to do that, one can do so explicitly using quantity_point_2(qp1.relative()).

Tests

[...] No explicit tests on compatibility / incompatibility between unrelated origins yet.

Documentation

Not in the PR yet - sorry.

If anything's missing, it's the tests to back this quote and everything else it recognizes as missing as-of-yet. Otherwise, it looks good, besides the other points raised by @mpusz.

JohelEGP avatar Feb 21 '21 19:02 JohelEGP

Well, we also need some docs ;-) But it can be added in the following PR if you prefer. I just prefer authors of big features to provide the docs as they better understand the rationale for the design than I. In case you will have any issues with docs generation please do not hesitate to contact me.

mpusz avatar Feb 21 '21 19:02 mpusz

I'll start with the quick fixes. Then I guess I should strive for a proper implementation of the temperature things, as it may impact the internals of the point_origin a bit still. I'll try to add in at least some minimal documentation as I go too. I really posted the PR now to get some feedback on the general design and interest of such a feature, before I invest too much time.

burnpanck avatar Feb 21 '21 19:02 burnpanck

@mpusz and @johelegp: I'd like to hear your input regarding "identity" and "equivalence" of different point_origin. The reason is, I really would like to associate each point_origin with a dimension, so I can require that a DerivedPointOrigin has a reference_origin set, that their two associated dimensions are equivalent and that the offset_to_reference(previously reference_offset) of the derived origin is compatible with it's dimension.

Technically, that makes PointOrigin very similar to Kind. However, there is a significant difference: Currently, quantity_cast happily converts between unrelated kinds as long as the dimensions are equivalent by copying the physical quantity, replacing the kind. However, for quantity_points with different origins, the only physical correct action is to account for the "offset" between those origins if it is known, or otherwise refuse the conversion.

The problem then comes from the fact that separate systems have distinct dimensions, where corresponding dimensions are equivalent however, e.g. cgs::dim_length and si::dim_length. Now if we want to allow points to be measured with respect to a specific abstract origin (say mean_sea_level) in units of both systems and them to be comparable, we will either need to allow mixing different but equivalent dimensions in a quantity and it's origin, or we need some way to automatically create separate versions of the same abstract origin with different associated dimensions. The first approach will not help for default_point_origin which would always pick the corresponding dimension and thus create non-equivalent origins. The second approach would require a third notion to identify "the same abstract origin", cause neither he dimension, it's equivalence class nor the specific point-origin type would match that notion.

The current work-around is that instead of associating a dimension, I associate the reference unit of the dimension with the point-origin. Ultimately, the reference unit determines equivalency of dimensions, as far as I understand. This way, I end up with a single type for the whole equivalence class of dimensions, allowing me to use the type of the point-origin itself to distinguish abstract origins. However, I'm not sure if that doesn't even break completely on dimensions with compound units.

The final alternative would of course be to not associate point-origins with any dimension at all, but that also kind of feels wrong: All examples of abstract origins I can think of are clearly associated with a dimension, so my gut tells me that this connection should be somehow encoded in the type system.

burnpanck avatar Feb 23 '21 09:02 burnpanck

Ultimately, the reference unit determines equivalency of dimensions, as far as I understand.

Please note that in some systems the same unit can be used for different dimensions: https://github.com/mpusz/units/blob/master/src/include/units/physical/natural/bits/dimensions.h.

mpusz avatar Feb 23 '21 15:02 mpusz

However, I'm not sure if that doesn't even break completely on dimensions with compound units.

I'm not sure, either. I've opened quite a few issues on equivalent-but-not-same dimensions, so I think you shouldn't worry. Do include some tests, and comment them out or add // TODOs as necessary.

JohelEGP avatar Feb 23 '21 18:02 JohelEGP

Hey guys, just to let you know, this is not forgotten! A project has just jumped in priority, so I probably won't be able to work on this for the next 10 days. I hope to implement the requested/promised changes after that.

burnpanck avatar Mar 09 '21 17:03 burnpanck

I am not rushing you at all. Just want to check if you still plan to work on this feature or maybe someone else should take ownership of this subject?

mpusz avatar May 09 '21 17:05 mpusz

Hey @mpusz, no worries - we al know how it goes. This feature is dear to my heart (as is this library in general), so I definitely intend to bring it into master eventually. However, I am strongly time-constrained, and I am aware that a half-finished feature does not help the community much. So if somebody is interested and able to push the feature faster than I am, I am happy to hand over.

burnpanck avatar May 09 '21 20:05 burnpanck

Thanks for the info. Yeah, I know how it is to have a limited bandwidth too. For now, I also do not have time to take this task. However, if someone would like to work on this, please let us know. I am sure that at some point we will have it done 😃

mpusz avatar May 09 '21 20:05 mpusz

It seems there are still conflicts in this. This will have to wait until another day though as it's almost midnight over here.

burnpanck avatar Sep 17 '21 21:09 burnpanck

Here we go again. I believe with this, the PR is now fully merged with master, and all previous functionality back there, minus the "zero point constants" (due to the change from unit constants to unit references). What I still intend to do:

  • [ ] Review the glide_computer example for opportunities to highlight both known fixed origin offsets, and unknown origin offsets.
  • [ ] Find a fix for the -Werror,-Wunused-const-variable with Clang 12.
  • [ ] Update the documentation on the various ways to construct a QuantityPoint - however, see below:

Before we finalise this, I think we should review the design choices, especially around the implicit and explicit ways to determine a PointOrigin to be used with a given unit (e.g. with unit/zero-point constants, CTAD, PointKind from Kind, etc...). I think there is still room for improvement, and I intend to write-up the potential design-choices in poll format (potentially useful for a future LEWG meeting too).

burnpanck avatar Sep 18 '21 08:09 burnpanck

Hey all (especially @mpusz, @JohelEGP), I added the promised poll-style design discussion (temporarily within the source tree: https://github.com/burnpanck/units/blob/feature/quantity-point-origin/docs/design/quantity_point.md). Please have a look, and if you generally agree, I post this as a poll under "Discussions".

burnpanck avatar Sep 19 '21 13:09 burnpanck

1.1) Should re-expression with respect to a shifted origin be considered a safe conversion, and thereby implicitly allowed?

Looks at std::chrono. Looks like you couldn't convert time points between clocks before C++20. And even now, it involves going through sys_time, which is a relatively expensive runtime operation.

  • [x] N) 🔘 None at all. All conversions with non-equivalent origins should be explicit.

2.2) Explicit constructor with unspecified PointOrigin using CTAD, when the argument is a Quantity.

I'm starting to believe that dynamic_origin was another mistake.

2.3) absolute free-standing factory function with explicit PointOrigin

What we probably need is partial CTAD. I'm neutral.

3.2) API to declare PointOrigins explicitly with a fixed offset to other PointOrigins

  • [x] T) Declare the reference origin and the offset as template arguments to the base PointOrigin; e.g. struct zero_deg_C_point : point_origin<dim_thermodynamic_temperature, absolute_zero_point, ratio(27315,100), kelvin> {};.

4.1) Should we provide a public API to declare implicit PointOrigins?

  • [x] N) 🔘🤳 Implicit origins are a constant source of confusion - we should not make it easy to implement such horrific things and therefore do not specify a public API.

JohelEGP avatar Sep 19 '21 14:09 JohelEGP

@burnpanck, it has been a while and this PR still has [WIP] in its name. Is it ready to merge? Do you intend to work on it in the future?

mpusz avatar May 09 '22 17:05 mpusz

Ah, time passes so quickly. Unfortunately, I'm overloaded with work again, so I don't want to promise too much. But unless there have been major vision changes to the library in the mean time which would ask for a complete redesign (I haven't followed), I would at least attempt to rebase to the current head and try to dig out what was still open. As far as I recall, the PR was "working for me" (we're actually using it in production), but there were open bike-shedding points, potentially around the API for the specification of custom reference points.

burnpanck avatar Jul 20 '22 08:07 burnpanck

gitpod-io[bot] avatar Dec 13 '22 16:12 gitpod-io[bot]

... and time passed quickly some more. I may have some time to work on this in the next few days and weeks. However, given the current focus on V2, what do you think is a good way forward? Given that the current state of this PR is at least "works for us" (in production on an embedded system) but potential for bike-shedding of the API, and that V2 is first an foremost an API refactor, maybe it would make sense to simply try to fix the CI failures and do not bother to polish the API independent from V2?

burnpanck avatar Dec 30 '22 17:12 burnpanck

V2 heavily refactored quantity_point and I believe the interface now is able to provide all the needed features. We just have to improve the implementation and answer the question on how deg_C -> K quantity_point conversion should look like.

mpusz avatar Jan 03 '23 10:01 mpusz

However, I do not want to close that PR for now as there is a lot of great stuff here (including the documentation), so we can still benefit from its parts in the V2 design.

mpusz avatar Jan 03 '23 10:01 mpusz

Superceeded with the V2 design.

mpusz avatar Sep 18 '23 08:09 mpusz