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

burnpanck · 2021-02-21T17:29:52Z

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.

…ons yet

example/temperature.cpp

src/include/units/origin.h

@JohelEGP

Fix downcast facility for point origin, thanks to @JohelEGP. Co-authored-by: Johel Ernesto Guerrero Peña <johelegp@gmail.com>

mpusz

This is really good work. Thank you!
Please fix according to comments and I would also like @JohelEGP to provide additional comments before I accept this change.

example/temperature.cpp

src/include/units/bits/common_quantity.h

src/include/units/quantity_cast.h

JohelEGP · 2021-02-21T19:11:38Z

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.

mpusz · 2021-02-21T19:19:13Z

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.

burnpanck · 2021-02-21T19:49:21Z

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.

JohelEGP · 2021-02-22T04:12:23Z

test/unit_test/static/quantity_point_kind_test.cpp

@@ -305,7 +306,7 @@ static_assert(!constructible_or_convertible_from<nth_apple<one, int>>(quantity_p
 static_assert(!constructible_or_convertible_from<nth_apple<one, int>>(quantity_point(dimensionless<percent, double>(1))));
 static_assert(!constructible_or_convertible_from<nth_apple<one, double>>(quantity_point(1.0 * s)));

-static_assert(construct_from_only<quantity_point_kind<time_point_kind, second, int>>(sys_seconds{42s}).relative().common() == 42 * s);
+static_assert(construct_from_only<quantity_point_kind<time_point_kind, second, int, sys_clock_origin>>(sys_seconds{42s}).relative().common() == 42 * s);


I'm sure the tests will make this evident, but there's no reason for a quantity_point with the default origin to not be constructible from a std::chrono::time_point.

Actually, a quantity_point with default origin should not be constructible from a std::chrono::time_point, as that is not one type but a template on the Clock (+ the duration type). That Clock type performs the role of the point_origin here. You can verify e.g. from operator- that indeed, std::chrono::time_points referring to different Clocks are not compatible with each other.

Of course however, we should be able to deduce the correct point_origin specific to the Clock (not the default point_origin), when constructing from a std::chrono::time_point using CTAD. That should already be in the PR, which of course should be tested (modulo GCC ICE). I would assume that the GCC ICE is the reason why these tests do not do CTAD? Maybe we should introduce such tests and just leave them out using preprocessor selection for affected compilers.

I think you're right. After all, a units::quantity_point doesn't carry a Clock. Before this PR, I'm pretty sure you could do static_assert(quantity_point{sys_seconds{0}} == quantity_point{after_christ_second{0}}); and have that assertion pass, even though sys_seconds{0} is 1970 A.C. and after_christ_second{0} is 0 A.C. With this PR, it'd become a compile-time error.

As you can glean from my example here, you can do some kind of CTAD in GCC for types in this library (look for "CTAD" in the tests). Although not generally. as in the GCC bug I pointed out in other comment. For the case you mention, you should be able to use CTAD.

I think that static_assert(quantity_point{sys_seconds{0}} == quantity_point{after_christ_second{0}}); should be allowed. There seems to be no good excuse to not compare quantity points with different orgins?

Probably. It's just that in master, the assertion passes. With this PR, it will fail at compile time. Which is better, as master gives the wrong result.

In the future, that can be made to work again. But you have to decide how to handle the comparison. For example, quantity comparison will convert both arguments to the common type, which should result in a lossless conversion that preserves the value and thus gives the correct result. But there's no precedent as to how to do that with quantity points and their origins.

For construction, there's only one way. Convert the argument to the type, adjusting it's value by the difference of origins. While it sounds simple, its solution could be involved, like quantity_cast is. Otherwise quantity_point<dim_time, second, int8_t, after_christ_origin>{sys_seconds{years{-1970}}} could overflow.

…ck_t and std::time_t

…uantity-point-origin

… formatting

…units into feature/quantity-point-origin

burnpanck · 2021-02-23T09:52:36Z

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

mpusz · 2021-02-23T15:07:46Z

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.

src/include/units/bits/common_quantity.h

example/temperature.cpp

src/include/units/quantity_point.h

JohelEGP · 2021-02-23T18:20:38Z

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.

Co-authored-by: Mateusz Pusz <mateusz.pusz@gmail.com>

…units into feature/quantity-point-origin

burnpanck · 2021-02-23T18:35:38Z

src/include/units/bits/point_origin_reference.h

+
+template<PointOrigin Orig>
+struct reference_origin {
+  using type = typename Orig::reference_origin;


No, don't, seen it just now :-)! That is likely again one of those fewer typenames, right? Have to find that paper...

It's https://wg21.link/P0634R3. https://en.cppreference.com/w/cpp/compiler_support is a good place to look for them.

…°C and °F

JohelEGP · 2021-02-23T19:39:09Z

src/include/units/physical/si/base/thermodynamic_temperature.h

+template<UnitOf<dim_thermodynamic_temperature> U = degree_celsius, QuantityValue Rep = double>
+using celsius_temperature_point = quantity_point<si::dim_thermodynamic_temperature, U, Rep, celsius_temperature_origin>;


Why allow specifying the unit if it's supposed to always be degree_celsius?

Same as above, with kelvin_temperature_point.

millikelvin?

I was also thinking along the lines of allowing to specify the ratio. For example, for celsius_temperature_point, currently, you can specify an UnitOf<dim_thermodynamic_temperature>, like kelvin, which is not a celsius unit.

celsius_temperature_point<kelvin> looks like lying, which is what I'm getting at. However, it might make sense. That's quantity_point<si::dim_thermodynamic_temperature, kelvin, double, celsius_temperature_origin>.

Are you sure? Please be aware, that currently thermotdynamic_temperature_potin<>(0 * deg_C) == thermodynamic_temperature_point<>(0 * K) (all with the same dimension-specific default origin). There is no such thing as a unit-specific default origin.

I think this not true. The offset between the beginning of the scales for Celsius and Kelvin is well defined and the library should always use it when comparing or doing arithmetic over different units. Please read more in the review to the last commit.

It is definitely true in the current implementation. I understand the desire for magic identification of origin points based on units to work, because in every-day use, that is what we do. This behaviour is currently implemented in interpret_as_temperature_point. But in everyday use, you never deal with expressions of temperatures, only concrete values.

To do magic identification of origin points by default, I think is very dangerous. It doesn't respect the semantics of usual physical quantities: When dealing with differences, 1 * K and 1 * deg_C are physically equivalent. If that behaviour is implemented, all of a sudden, the physical equivalence is erased: There is a way to express equivalent temperature points in Celsius and Kelvin, but the temperature points you get from equivalent temperature differences will not be equivalent.

The behaviour provided by the quantity type in this library (and for that matter most other unit libraries): It always represents differences, where the exact representation (i.e. unit) doesn't matter. All of a sudden, the equivalency is erased. That may look innocent with examples like the above, but consider the following:

thermodynamic_temperature_point( 1_q_deg_C * (1_q_km / 1_q_m)): Should unknown_unit be forbidden?

thermodynamic_temperature_point( 1_q_Del): User defined the Delisle, so it's not unknown_unit, but she didn't tell the library what reference point to take. Should it be Kelvin? Should the library refuse?

quantity_point(1_q_ft): No specific reference point defined: Should the library refuse?

thermodynamic_temperature_point(1 * deg_C + 1 * deg_F)

thermodynamic_temperature_point(1 * deg_F + 1 * deg_C): Is that the same/physically equivalent temperature point as in 4)? Which point is it?

Note that there is no way for the library to refuse 4) and 5).

In our library quantities are modeled as "relative". So yes, 1 * K and 1 * deg_C are physically equivalent. However, if you work on quantity points they should not be equivalent and I still hope it is possible to make it work with "identification of origin points by default" when casting between those units (because you have to cast in order to compare).

However, you are right with your 1-5. Life is brutal 😉.

…ert between related origins

src/include/units/physical/si/base/thermodynamic_temperature.h

src/include/units/physical/si/imperial/base/thermodynamic_temperature.h

mpusz · 2021-02-23T21:45:09Z

src/include/units/quantity.h

+  template <PointOrigin Orig>
+  [[nodiscard]] constexpr quantity_point<dimension, unit, rep, Orig> absolute() const noexcept;


I am not sure if we want quantity to be aware of quantity_point?

Up to bike-shedding too.

Pros:

Symmetry with quantity_point::relative

It is otherwise non-trivial to instantiate quantity_point from a quantity with matching units and a specific origin: instead of radar_distance_reading.template absolute<ground_level>() in generic code, I would have to write

units::quantity_point<typename decltype(radar_distance_reading)::dimension, typename decltype(radar_distance_reading)::unit, typename decltype(radar_distance_reading)::rep, ground_level>(radar_distance_reading)

Con:

There is some circular dependency here, with the implementation of that method deferred to quantity_point.h.

Alternatives/questions:

We could instead provide a free-standing function to that end

We could make the point_origins to be constants so that the template keyword can be dropped and provided as method argument instead

We could provide suitable constructors with the right deduction guides

quantity_point{1 * m} already works and uses the default origin.
How about

template<PointOrigin> QuantityPoint auto absolute(Quantity auto); QuantityPoint auto absolute(Quantity auto); // default origin template<PointOrigin> QuantityPointKind auto absolute(QuantityKind auto); QuantityPointKind auto absolute(QuantityKind auto); // default origin

Free-standing absolute function is fine with me. I'm wary of default origins though:

Default origins only works when there is a single unambiguous origin. In those cases, half the reason to use quantity points at all is gone. (the other half is to make it clear that one is talking about points, not distances - which is still useful).

But one of our prime use-cases is one where there are different origins (temperatures): Currently, you would pass the following: assert(quantity_point(1 * deg_C) == quantity_point(1 * K)), which is most likely not what you want. You could argue that this is in fact correct. Alternatively the constructor could be adjusted (with some effort) so that instead the following would pass: assert(quantity_point(0 * deg_C) == quantity_point(273.15 * K)). That is potentially less surprising. But exactly that ambiguity which of those two asserts even should be correct is the reason why I really would like to avoid default origins at all. In generic code, that becomes a nightmare, where the result of a piece of code becomes physically different dependent on which of those physically equivalent units are used.

I suppose it'd be fine to leave them out until someone needs them.

Should we disable CTAD for quantity_point from a quantity?

Should we provide the non-member relative() function as well (while still keeping the member relative() one)?

assert(quantity_point(1 * deg_C) == quantity_point(1 * K)) is obviously bad and should fail.

So then I am in favour of disabling CTAD for quantity_point from quantity.

Please read my notes in the review to the last commit.

(1) and (2) sound good to me.

mpusz · 2021-02-23T21:47:35Z

src/include/units/quantity_cast.h

+ * auto q1 = units::quantity_point_cast<decltype(0_qp_deg_F)>(1_qp_deg_C);
+ * auto q1 = units::quantity_point_cast<units::physical::si::celsius_temperature_origin>(1_qp_deg_F);


I thought that we are not providing UDLs for quantity points in the library (https://mpusz.github.io/units/framework/quantity_points.html#differences-to-quantity)?

Well, I added that because you said this: #1 (comment).

Yeah, and it still stands. Although, I must admit that 1_q_deg_C and 1_qp_deg_C look pretty distinct to me. Unit constants are going to prove worse here... 😞

Maybe it does now. But the difference is just 1 letter. I switched to l and r from lhs and rhs due to how often I confused them.

This is why I think that quantity_points should be explicit in the code. Most regular users will have problems in understanding the difference. Also, UDLs are only for compile-time constants. The more I work with the library the more I think that unit constants is the way to go (https://mpusz.github.io/units/framework/quantities.html#udls-vs-unit-constants) and I do not see an easy way to make quantity points distinct enough here.

First I thought that quantity constants do not work for quantity points, because they cannot be multiplied. However, Now that I think about it, I think there is an elegant solution: 1*m + mean_sea_level!

The other thing that could be considered would be to use CTAD again to let quantity_point(1 * deg_C) magically pick the right origin point, here celsius_temperature_origin. I don't like that because it would be far too risky for the magic kicking in in generic code, leading to silently wrong origins being picked.

The more I think about quantity + origin as a generic way of constructing a quantity point, the more I like it :-). With that, the need for other methods to construct quantity points is less pressing, and I leave it up to you/the general UDL discussion if we should provide them literals for quantity points, and how they should be named.

I like that. It's a very convenient syntax to construct quantity_points.

I like it too! 👍

src/include/units/quantity_kind.h

…isit that 'rebind' business.

JohelEGP · 2021-09-17T21:33:37Z

src/core/include/units/bits/equivalent.h

@@ -81,7 +81,7 @@ template<Unit U1, Dimension D1, Unit U2, Dimension D2>
 struct equivalent_unit : std::disjunction<equivalent_impl<U1, U2>,
                                          std::bool_constant<U1::ratio / dimension_unit<D1>::ratio == U2::ratio / dimension_unit<D2>::ratio>> {};

-// point origins
+// point origins [JohelEGP]


test/unit_test/static/quantity_point_test.cpp

burnpanck · 2021-09-17T21:47:03Z

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

…l no manual review done yet though

burnpanck · 2021-09-18T08:57:16Z

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

JohelEGP · 2021-09-18T14:17:07Z

docs/framework/quantity_points.rst

+    QuantityPoint d(32 * deg_F);
+
+In this case, if the library is aware of a specific customary origin that is implied by the unit, it will select
+the corresponding `customaery_origin_for_unit<Unit>`. Otherwise it will select the `unspecified_origin<Dimension>`.


Suggested change

the corresponding `customaery_origin_for_unit<Unit>`. Otherwise it will select the `unspecified_origin<Dimension>`.

the corresponding `customary_origin_for_unit<Unit>`. Otherwise it will select the `unspecified_origin<Dimension>`.

JohelEGP · 2021-09-18T14:17:32Z

docs/framework/quantity_points.rst

+
+.. warning::
+   Be very careful with that machinery in generic code! Normally, generic code does not need to care about the units
+   of it's arguments. Arithmetic summation for exampel will produce the same physically correct result (up to rounding),


Suggested change

of it's arguments. Arithmetic summation for exampel will produce the same physically correct result (up to rounding),

of it's arguments. Arithmetic summation for example will produce the same physically correct result (up to rounding),

JohelEGP · 2021-09-18T14:21:17Z

docs/framework/quantity_points.rst

+ - `units::physical::si::zp_kelvin`: The point of 0 K.
+ - `units::physical::si::zp_deg_celsius`: The point of 0 °C.
+ - `units::physical::si::imperial::zp_deg_fahrenheit`: The point of 0 °F.


The library currently only offers Kelvin and a K reference. Maybe the quantity point version could be named zeroth_K.
The suggestions above depart from the convention of having the same base name for aliases, references and UDLs.
According to the UDLs below, the others should be named zeroth_deg_C and zeroth_deg_F.

JohelEGP · 2021-09-18T14:26:11Z

example/CMakeLists.txt

@@ -34,6 +34,7 @@ add_example(conversion_factor mp-units::core-fmt mp-units::core-io mp-units::si)
 add_example(custom_systems mp-units::core-io mp-units::si)
 add_example(hello_units mp-units::core-fmt mp-units::core-io mp-units::si mp-units::si-international)
 add_example(measurement mp-units::core-io mp-units::si)
+add_example(temperature mp-units::core-fmt mp-units::core-io mp-units::si mp-units::si-imperial)


It seems like the timer example below is not added.

JohelEGP · 2021-09-18T14:31:37Z

src/core/include/units/bits/basic_concepts.h

+ * A point-origin can further be made fixed offset with respect to another origin if a nested
+ * type `reference_point_origin` is present, which also needs to be a PointOrigin.
+ * Then a nested static constant `offset_to_reference` needs to be present as-well,
+ * unless `referende_point_origin` is the same type as the parent origin.


Suggested change

* unless `referende_point_origin` is the same type as the parent origin.

* unless `reference_point_origin` is the same type as the parent origin.

Can you clarify what you mean by "parent" as well?

JohelEGP · 2021-09-18T16:43:57Z

src/core/include/units/quantity_point.h

+template<PointOrigin O, UnitOf<typename O::dimension> U, Representation Rep>
+inline constexpr quantity_point<O, U, Rep> absolute(const quantity<typename O::dimension,U,Rep> &q) noexcept {
+  return quantity_point<O, U, Rep>(q);
+}


Suggested change

template<PointOrigin O, UnitOf<typename O::dimension> U, Representation Rep>

inline constexpr quantity_point<O, U, Rep> absolute(const quantity<typename O::dimension,U,Rep> &q) noexcept {

return quantity_point<O, U, Rep>(q);

}

template<PointOrigin Orig, UnitOf<typename Orig::dimension> U, Representation Rep>

[[nodiscard]] constexpr quantity_point<Orig, U, Rep> absolute(const quantity<typename Orig::dimension,U,Rep> &q) {

return quantity_point<Orig, U, Rep>(q);

}

isn't the inline still needed to prevent duplicate instantiations? I was under the assumption only class methods are implicitly inline...

Function templates are, too. As are all templated entities other than non-member variable templates.

JohelEGP · 2021-09-18T16:50:51Z

src/core/include/units/quantity_point_kind.h

+template <PointOrigin Orig, Kind K, UnitOf<typename K::dimension> U, Representation Rep>
+inline constexpr auto
+  absolute(const quantity_kind<K,U,Rep> &qk) noexcept {
+  return quantity_point_kind<downcast_point_kind<K, typename Orig::template rebind<typename K::dimension>>,U, Rep>(qk);
+}


Suggested change

template <PointOrigin Orig, Kind K, UnitOf<typename K::dimension> U, Representation Rep>

inline constexpr auto

absolute(const quantity_kind<K,U,Rep> &qk) noexcept {

return quantity_point_kind<downcast_point_kind<K, typename Orig::template rebind<typename K::dimension>>,U, Rep>(qk);

}

template <PointOrigin Orig, Kind K, UnitOf<typename K::dimension> U, Representation Rep>

[[nodiscard]] constexpr auto

absolute(const quantity_kind<K,U,Rep> &qk) {

return quantity_point_kind<downcast_point_kind<K, typename Orig::template rebind<typename K::dimension>>,U, Rep>(qk);

}

JohelEGP · 2021-09-18T17:01:22Z

src/systems/si-imperial/include/units/isq/si/imperial/thermodynamic_temperature.h

+template <> struct customary_origin_spec_for_unit<units::isq::si::imperial::degree_fahrenheit> {
+  using type = units::isq::si::imperial::fahrenheit_temperature_origin;
+};


This manually separate specification seems to go against the design of the library.

JohelEGP · 2021-09-18T17:10:10Z

src/systems/si-imperial/include/units/isq/si/imperial/thermodynamic_temperature.h

+namespace units {
+
+template <Representation Rep>
+inline constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::imperial::degree_fahrenheit, Rep> &t) {


Suggested change

inline constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::imperial::degree_fahrenheit, Rep> &t) {

[[nodiscard]] constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::imperial::degree_fahrenheit, Rep> &t) {

JohelEGP · 2021-09-18T17:11:47Z

src/systems/si/include/units/isq/si/thermodynamic_temperature.h

+inline constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::kelvin, Rep> &t) {
+  return units::isq::si::kelvin_temperature_point<units::isq::si::kelvin, Rep>(t);
+}
+template <Representation Rep>
+inline constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::millikelvin, Rep> &t) {
+  return units::isq::si::kelvin_temperature_point<units::isq::si::millikelvin, Rep>(t);
+}
+
+template <Representation Rep>
+inline constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::degree_celsius, Rep> &t) {


Suggested change

inline constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::kelvin, Rep> &t) {

return units::isq::si::kelvin_temperature_point<units::isq::si::kelvin, Rep>(t);

}

template <Representation Rep>

inline constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::millikelvin, Rep> &t) {

return units::isq::si::kelvin_temperature_point<units::isq::si::millikelvin, Rep>(t);

}

template <Representation Rep>

inline constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::degree_celsius, Rep> &t) {

[[nodiscard]] constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::kelvin, Rep> &t) {

return units::isq::si::kelvin_temperature_point<units::isq::si::kelvin, Rep>(t);

}

template <Representation Rep>

[[nodiscard]] constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::millikelvin, Rep> &t) {

return units::isq::si::kelvin_temperature_point<units::isq::si::millikelvin, Rep>(t);

}

template <Representation Rep>

[[nodiscard]] constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::degree_celsius, Rep> &t) {

JohelEGP · 2021-09-18T17:57:33Z

docs/framework/basic_concepts.rst

@@ -23,7 +23,7 @@ The most important concepts in the library are `Unit`, `Dimension`,
    ]

    [<abstract>QuantityPoint|
-    [quantity_point<PointOrigin, Unit, Rep>]
+    [quantity_point<Dimension, Unit, Rep>]


Changes to this file are a regression in the conflict resolution of the merge.

JohelEGP · 2021-09-18T18:01:00Z

src/systems/si/include/units/isq/si/thermodynamic_temperature.h

+ * WARNING: The returned value's physical meaning depends on the unit of the input quantity, which has no physical relevance on it's own.
+ * That is, 1 Kelvin and 1 degree Celsius are physically equivalent quantities (as differences between thermodynamic temperatures),
+ * yet the returned temperature points are physically different. This is in stark contrast to the usual behaviour of
+ * this library, where unit conversions. For example, for the following holds:


Suggested change

* this library, where unit conversions. For example, for the following holds:

* this library, where unit converts. For example, for the following holds:

Is this what you meant?

…ussion about the design choices

burnpanck · 2021-09-19T13:48:36Z

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

JohelEGP · 2021-09-19T14:47:58Z

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.

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

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?

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.

mpusz · 2022-05-09T17:23:43Z

@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?

docs/framework/quantity_points.rst

burnpanck · 2022-07-20T08:20:57Z

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.

gitpod-io · 2022-12-13T16:36:52Z

…uantity-point-origin

burnpanck · 2022-12-30T17:30:27Z

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

mpusz · 2023-01-03T10:31:03Z

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 · 2023-01-03T10:32:21Z

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 · 2023-09-18T08:58:08Z

Superceeded with the V2 design.

first implementation of a quantity_point's point_origin - no conversi…

209e1fc

…ons yet

JohelEGP reviewed Feb 21, 2021

View reviewed changes

example/temperature.cpp Outdated Show resolved Hide resolved

fix non-ASCII symbol text in temperature example

ee459d3

JohelEGP reviewed Feb 21, 2021

View reviewed changes

example/temperature.cpp Outdated Show resolved Hide resolved

JohelEGP reviewed Feb 21, 2021

View reviewed changes

src/include/units/origin.h Outdated Show resolved Hide resolved

Update src/include/units/origin.h

f7998c9

Fix downcast facility for point origin, thanks to @JohelEGP. Co-authored-by: Johel Ernesto Guerrero Peña <johelegp@gmail.com>

mpusz requested changes Feb 21, 2021

View reviewed changes

JohelEGP reviewed Feb 22, 2021

View reviewed changes

burnpanck added 4 commits February 23, 2021 08:10

fixed sign-conversion warning in doubtful conversion between std::clo…

8f93aaf

…ck_t and std::time_t

Merge branch 'bugfix/sign-conversion-in-timer-example' into feature/q…

fa84dbd

…uantity-point-origin

consistently named PointOrigin template parameter Orig & more careful…

f2d041b

… formatting

Merge branch 'feature/quantity-point-origin' of github.com:burnpanck/…

c969d30

…units into feature/quantity-point-origin

mpusz requested changes Feb 23, 2021

View reviewed changes

burnpanck and others added 4 commits February 23, 2021 19:23

Apply suggestions from code review

8444539

Co-authored-by: Mateusz Pusz <mateusz.pusz@gmail.com>

implemented DerivedPointOrigin and reference_origin properly

e22f362

Merge branch 'feature/quantity-point-origin' of github.com:burnpanck/…

69653dd

…units into feature/quantity-point-origin

small fix, C++20 rules for typename disambiguator

5d9e478

burnpanck commented Feb 23, 2021

View reviewed changes

added definitions for quantities and quantity points of units mK, K, …

3af07de

…°C and °F

JohelEGP reviewed Feb 23, 2021

View reviewed changes

quantity_point_cast and quantity_point_kind_cast are now able to conv…

b869a0c

…ert between related origins

JohelEGP reviewed Feb 23, 2021

View reviewed changes

src/include/units/physical/si/base/thermodynamic_temperature.h Outdated Show resolved Hide resolved

JohelEGP reviewed Feb 23, 2021

View reviewed changes

src/include/units/physical/si/imperial/base/thermodynamic_temperature.h Outdated Show resolved Hide resolved

mpusz reviewed Feb 23, 2021

View reviewed changes

compilation succeeds with Clang 12 and fails with GCC 11. Need to rev…

5d16497

…isit that 'rebind' business.

JohelEGP reviewed Sep 17, 2021

View reviewed changes

test/unit_test/static/quantity_point_test.cpp Outdated Show resolved Hide resolved

fixed static compilation tests, added temperature example back - stil…

9c8b734

…l no manual review done yet though

JohelEGP requested changes Sep 18, 2021

View reviewed changes

fixed a small number of @JohelEGP's comments, added in a lengthy disc…

e1f5e74

…ussion about the design choices

kammce mentioned this pull request Apr 24, 2022

hal::temperature_sensor libhal-google/libhal#137

Closed

JohelEGP reviewed Jul 20, 2022

View reviewed changes

docs/framework/quantity_points.rst Show resolved Hide resolved

JohelEGP mentioned this pull request Nov 29, 2022

feat: raise level of abstraction from dimension to structure describing a quantity #405

Closed

Merge branch 'master' into feature/quantity-point-origin

d100d77

burnpanck force-pushed the feature/quantity-point-origin branch from 919e847 to d100d77 Compare December 13, 2022 16:36

burnpanck added 4 commits December 13, 2022 17:45

recfactor: use FMT_THROW inside fmt.h to make code embedded-friendly

4603d24

Merge branch 'feature/make-fmt-more-embedded-friendly' into feature/q…

eeea806

…uantity-point-origin

add missing throws in format.h; undo change to core.h

c346ee1

Merge branch 'feature/make-fmt-more-embedded-friendly' into feature/q…

a5324b1

…uantity-point-origin

mpusz mentioned this pull request Dec 20, 2022

V2 framework #391

Merged

18 tasks

mpusz mentioned this pull request Jan 3, 2023

V2 quantity_point #414

Closed

mpusz closed this Sep 18, 2023

		template<UnitOf<dim_thermodynamic_temperature> U = degree_celsius, QuantityValue Rep = double>
		using celsius_temperature_point = quantity_point<si::dim_thermodynamic_temperature, U, Rep, celsius_temperature_origin>;

		template <PointOrigin Orig>
		[[nodiscard]] constexpr quantity_point<dimension, unit, rep, Orig> absolute() const noexcept;

		* auto q1 = units::quantity_point_cast<decltype(0_qp_deg_F)>(1_qp_deg_C);
		* auto q1 = units::quantity_point_cast<units::physical::si::celsius_temperature_origin>(1_qp_deg_F);

	the corresponding `customaery_origin_for_unit<Unit>`. Otherwise it will select the `unspecified_origin<Dimension>`.
	the corresponding `customary_origin_for_unit<Unit>`. Otherwise it will select the `unspecified_origin<Dimension>`.

	of it's arguments. Arithmetic summation for exampel will produce the same physically correct result (up to rounding),
	of it's arguments. Arithmetic summation for example will produce the same physically correct result (up to rounding),

	* unless `referende_point_origin` is the same type as the parent origin.
	* unless `reference_point_origin` is the same type as the parent origin.

	inline constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::imperial::degree_fahrenheit, Rep> &t) {
	[[nodiscard]] constexpr auto interpret_as_temperature_point(const units::isq::si::thermodynamic_temperature<units::isq::si::imperial::degree_fahrenheit, Rep> &t) {

	* this library, where unit conversions. For example, for the following holds:
	* this library, where unit converts. For example, for the following holds:

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

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

Conversation

burnpanck commented Feb 21, 2021 • edited Loading

Overview

Changes to quantity_point

PointOrigin concept

std::chrono::time_point

Compatibility between quantity points measured against different references

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

Side remark (rant?) on quantity_cast

Tests

Documentation

Bike-shedding

mpusz left a comment

Choose a reason for hiding this comment

JohelEGP commented Feb 21, 2021

Compatibility between quantity points measured against different references

Tests

Documentation

mpusz commented Feb 21, 2021 • edited Loading

burnpanck commented Feb 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohelEGP Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

burnpanck commented Feb 23, 2021 • edited Loading

mpusz commented Feb 23, 2021

JohelEGP commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpusz Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

burnpanck Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpusz Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpusz Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burnpanck commented Sep 17, 2021

burnpanck commented Sep 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burnpanck commented Feb 21, 2021 •

edited

Loading

Changes to `quantity_point`

`PointOrigin` concept

`std::chrono::time_point`

Side remark (rant?) on `quantity_cast`

mpusz commented Feb 21, 2021 •

edited

Loading

JohelEGP Feb 23, 2021 •

edited

Loading

burnpanck commented Feb 23, 2021 •

edited

Loading

mpusz Feb 25, 2021 •

edited

Loading

burnpanck Feb 25, 2021 •

edited

Loading

mpusz Feb 25, 2021 •

edited

Loading

mpusz Feb 25, 2021 •

edited

Loading